Addressing issues raised by staticcheck #2267
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2267 +/- ##
==========================================
- Coverage 97.81% 97.78% -0.03%
==========================================
Files 114 114
Lines 10266 10269 +3
==========================================
Hits 10042 10042
- Misses 156 158 +2
- Partials 68 69 +1
Continue to review full report at Codecov.
|
| @@ -89,7 +89,7 @@ func (s *IssueImportService) Create(ctx context.Context, owner, repo string, iss | |||
| if ok { | |||
| decErr := json.Unmarshal(aerr.Raw, i) | |||
| if decErr != nil { | |||
| err = decErr | |||
| return nil, resp, decErr | |||
There was a problem hiding this comment.
I'm concerned about this change in behavior... it looks to me like what we had before was most likely broken (because i should not have been returned), but I'm thinking that the original AcceptedError should be returned here instead of the error caused by attempting to unmarshal the JSON.
Because if we return the json.Unmarshal error, the client will have no way of knowing that this is actually an AcceptedError meaning that GitHub accepted the call but decided to not yet return a value.
Thoughts?
There was a problem hiding this comment.
yeah it's unusual here because the err was never used in this can, so we could simply drop the decErr which will be the current behavior
There was a problem hiding this comment.
Except that if decErr is nil then we do want to actually return the value of i, so we cannot drop it entirely. I'm just saying we should return aerr here instead.
| @@ -30,3 +30,15 @@ issues: | |||
| - linters: | |||
| - dupl | |||
| path: _test\.go | |||
| - linters: | |||
There was a problem hiding this comment.
it's probably worth adding at least a one-line comment explaining what each of these rules are that are being excluded
|
Closing abandoned PR. |
I've disabled the checks for switch from go's openpgp implementation to a community fork because it seems like it's not a drop in replacement and needs some extra work.
Original issue: #2260