-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix invalid JSON struct tag, and other linter issues #319
Conversation
This change is a pretty noisy change in that I went through and cleaned up all of the issues that my linters flagged. I wanted to get this done, so that we can add them as automatic CI checks for future PRs. In the process I did find one critical bug, where we had a malformed JSON struct tag: https://github.com/PagerDuty/go-pagerduty/blob/69ade4b95ef0ff1d582a7d9f226a98f9db886df1/webhook.go#L44 There are two linter issues we cannot fix until v1.5.0, because they are breaking changes. As such, the v1.5.0 development line would be the first place we can enforce those CI checks. Fixes #317
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments to indicate which linters / formatters caused some changes.
func (c *Client) TestAbility(ability string) error { | ||
return c.TestAbilityWithContext(context.Background(), ability) | ||
} | ||
|
||
// TestAbility Check if your account has the given ability. | ||
// TestAbilityWithContext checks if your account has the given ability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was flagged by golint
.
@@ -12,14 +12,13 @@ func TestAbility_ListAbilities(t *testing.T) { | |||
|
|||
mux.HandleFunc("/abilities", func(w http.ResponseWriter, r *http.Request) { | |||
testMethod(t, r, "GET") | |||
w.Write([]byte(`{"abilities": ["sso"]}`)) | |||
_, _ = w.Write([]byte(`{"abilities": ["sso"]}`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was flagged by errcheck
, which finds places where error checking was missed.
}) | ||
|
||
var client = &Client{apiEndpoint: server.URL, authToken: "foo", HTTPClient: defaultHTTPClient} | ||
client := &Client{apiEndpoint: server.URL, authToken: "foo", HTTPClient: defaultHTTPClient} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My editor uses gofumpt
instead of go fmt
for style*, and it's a bit more opinionated. This was changed automatically.
- Technically it's
gofumports
, which is meant to replacegoimports
.
@@ -30,14 +30,17 @@ func TestAnalytics_GetAggregatedIncidentData(t *testing.T) { | |||
} | |||
|
|||
bytesAnalyticsResponse, err := json.Marshal(analyticsResponse) | |||
testErrCheck(t, "json.Marshal()", "", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ineffective assignment linter caught that we weren't handling the err
value above; this is absolutely a bug.
@@ -54,6 +54,8 @@ type APIReference struct { | |||
Type string `json:"type,omitempty"` | |||
} | |||
|
|||
// APIDetails are the fields required to represent a details non-hydrated | |||
// object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was caught by golint
.
client := &Client{apiEndpoint: server.URL, | ||
authToken: "foo", | ||
HTTPClient: defaultHTTPClient, | ||
client := &Client{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was also gofumports
.
@stmcallister wanted to ping again to see if you may have a moment to 👀 this week. I apologize for it being such a large PR, thankfully the majority of changes were done by automatic tooling and not by hand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Reading through the lint changes was educational for me. 😁 Also, thanks for catching the webhook bug and bumping the version up to 1.4.0.
This change is a pretty noisy change in that I went through and cleaned up all
of the issues that my linters flagged. I wanted to get this done, so that we can
add them as automatic CI checks for future PRs.
In the process I did find one critical bug, where we had a malformed JSON struct
tag:
go-pagerduty/webhook.go
Line 44 in 69ade4b
There are two linter issues we cannot fix until v1.5.0, because they are
breaking changes. As such, the v1.5.0 development line would be the first place
we can enforce those CI checks.
Fixes #317