Return nil explicitly when there is no error. #550

Merged
merged 5 commits into from Feb 15, 2017

Projects

None yet

3 participants

@nspragg
Contributor
nspragg commented Feb 15, 2017 edited

Resolves #536. Explicitly return nil when there is no error. Hopefully, this should address outstanding instances of the following pattern:

if err != nil {
  return ..., err
}
return ..., err
@nspragg nspragg Return nil error explicitly when there is no error.
f090c92
@nspragg nspragg changed the title from Return nil error explicitly when there is no error. to Return nil explicitly when there is no error. Feb 15, 2017
@shurcooL

One issue to resolve, everything else looks good.

I've also confirmed that after this PR, I don't see any remaining issues, so feel free to update the PR description and/or commit message to say "Resolves #536.", because it'll be fully resolved once this is merged.

Thanks!

github/github.go
@@ -739,7 +739,7 @@ func (c *Client) RateLimit() (*Rate, *Response, error) {
return nil, nil, err
}
- return limits.Core, resp, err
+ return limits.Core, resp, nil
@shurcooL
shurcooL Feb 15, 2017 edited Collaborator

Hmm, this one won't work, it changes behavior. But I think the problem lies above.

Currently, it checks if limits is nil, and always returns err. In the (possibly impossible) path where limits is not nil, but err is also not nil, this change would incorrectly return nil error.

However, I think you should change the checks above to be like this:

limits, resp, err := c.RateLimits()
if err != nil {
	return nil, resp, err
}
if limits == nil {
	return nil, resp, fmt.Errorf("RateLimits() returned nil error and nil limits, cannot extract Core rate limit")
}

Then it's fine to do return limits.Core, resp, nil here.

We could also leave this as is, but I think it's unfriendly API to return nil, nil, nil, so changing it to always return non-nil error on failure would be better.

@nspragg
nspragg Feb 15, 2017 Contributor

Well spotted. I agree with checking both limit and error in this case (better than returning nil, nil, nil)

Incidentally, no tests failed. Is there any motivation to increase the test coverage in the project in the future?

@shurcooL
shurcooL Feb 15, 2017 Collaborator

Having high but practical test coverage is always a goal, and I think we're doing pretty well there (look at all the _test.go files). This finding doesn't change that. This is a particular rare edge case that I wouldn't expect to be fully caught by unit tests.

Also note that this is a deprecated method, so it's not worth spending a lot of time/effort on.

@nspragg nspragg nil check for both limits and error.
6c75e8c
github/github.go
return nil, nil, err
}
+ if limits == nil {
+ return nil, nil, fmt.Errorf("RateLimits returned nil limits and error. Unable to extract Core rate limit")
@shurcooL
shurcooL Feb 15, 2017 Collaborator

I think we should return original resp instead of nil here. It's either going to be nil or some valid response. If there's an error, and a valid response, it's better to return that to the caller so they can look into why the error happened, rather than dropping the response information.

Here, and in the if check above.

@shurcooL
shurcooL Feb 15, 2017 edited Collaborator

About the error message, see https://github.com/golang/go/wiki/CodeReviewComments#error-strings.

I don't think you should separate it into multiple sentences and capitalize the letters. First letter happens to be capital because it refers to RateLimits the function name.

Maybe use semicolon if you don't like comma:

"RateLimits returned nil limits and error; unable to extract Core rate limit"
@nspragg nspragg Retain original response information and update error string.
912e0d1
@shurcooL

LGTM. Thank you.

The nitpicky comment is optional, for your consideration.

github/github.go
if limits == nil {
- return nil, nil, err
+ return nil, resp, fmt.Errorf("RateLimits returned nil limits and error; unable to extract Core rate limit")
}
@shurcooL
shurcooL Feb 15, 2017 Collaborator

Nitpick, but I would remove this blank line now. It does not provide value at this point IMO.

Without the blank line separating them, it would be a little more visible that limits is checked for being non-nil before dereferencing limits.Core.

@nspragg nspragg Removing blank line to aid readability.
0026dfd
@gmlewis

Small Go-style nit-pick. Otherwise, LGTM.

github/github.go
if limits == nil {
- return nil, nil, err
+ return nil, resp, fmt.Errorf("RateLimits returned nil limits and error; unable to extract Core rate limit")
@gmlewis
gmlewis Feb 15, 2017 Member

s/fmt.Errorf/errors.New/

@nspragg nspragg Create error using errors package
c209660
@gmlewis

Thank you, @nspragg!
LGTM.

@shurcooL shurcooL merged commit dfd20fd into google:master Feb 15, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment