make IsNil guard against nil underlying errors #64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frankban
reviewed
Mar 11, 2020
checker.go
Outdated
| @@ -73,6 +73,34 @@ func (c *equalsChecker) Check(got interface{}, args []interface{}, note func(key | |||
| return nil | |||
| } | |||
|
|
|||
| // NilError is a Checker that checks whether an error value is nil. | |||
There was a problem hiding this comment.
I'd call it IsNilError:
- it is similar to IsNil that we already have;
- it will show up in autocompletion as an alternative to IsNil;
- all other checkers are 3rd person verbs.
checker.go
Outdated
| } | ||
| return errors.New("got non-nil error") | ||
| } | ||
|
|
There was a problem hiding this comment.
What do you think about hinting using this in the Equals checker when got is an error and want is nil?
We could add a hint with a note() for example.
checker_test.go
Outdated
| expectedNegateFailure: ` | ||
| error: | ||
| bad check: value is not an error | ||
| `, |
There was a problem hiding this comment.
Add a check with a nil concrete error type?
rogpeppe
changed the title
frankban
approved these changes
Mar 12, 2020
doc.go
Outdated
| @@ -214,6 +222,18 @@ For instance: | |||
| c.Assert("these are the voyages", qt.Matches, `these are .*`) | |||
| c.Assert(net.ParseIP("1.2.3.4"), qt.Matches, `1.*`) | |||
|
|
|||
| NilError | |||
There was a problem hiding this comment.
Please remove this and modify IsNil doc.
Almost everyone that uses quicktest for the first time uses IsNil to check for errors, but there's a subtle mistake in doing so because an error with a nil underlying value is not considered to be nil for the usual `err != nil` error checking. This change alters the semantics of `IsNil` to guard against this case. If you really do want to check that a error-implementing value is nil, you can still use qt.Equals. Also add package-level docs for ContentEquals.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Almost everyone that uses quicktest for the first time uses
IsNil to check for errors, but there's a subtle mistake in doing so
because an error with a nil underlying value is not considered
to be nil for the usual
err != nilerror checking.This change alters the semantics of
IsNilto guard againstthis case. If you really do want to check that a error-implementing
value is nil, you can still use qt.Equals.
Also add package-level docs for ContentEquals.