doc: update error-compare and define unwrapped-error #162
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
Pull Request Test Coverage Report for Build 9732834377Details
💛 - Coveralls |
mmorel-35
changed the title
mmorel-35
changed the title
mmorel-35
changed the title
Pull Request Test Coverage Report for Build 9732886433Details
💛 - Coveralls |
|
ErrorContains is anti-pattern and, I think, must be flagged by default. We can consider an option (config) to turn off it or vice versa |
|
Do you agree that Shall be converted to Then there can be another |
Pull Request Test Coverage Report for Build 9733131811Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9733147710Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9733219903Details
💛 - Coveralls |
|
@ccoVeille , |
ccoVeille
reviewed
Jul 1, 2024
CONTRIBUTING.md
Outdated
|
|
||
| **Autofix**: true. <br> | ||
| **Enabled by default**: true. <br> | ||
| **Reason**: The `Error()` method on the `error` interface exists for humans, not code. <br> |
There was a problem hiding this comment.
Using testify methods are error prone as they avoid nil pointer exception.
Pull Request Test Coverage Report for Build 9748055669Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9748065672Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
ccoVeille
reviewed
Jul 1, 2024
Pull Request Test Coverage Report for Build 9748225491Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
mmorel-35
force-pushed
the
contributing/error-compare
branch
from
9d3bc59
to
9e6e06b
Compare
Pull Request Test Coverage Report for Build 9748271183Details
💛 - Coveralls |
mmorel-35
changed the title
ccoVeille
approved these changes
Jul 1, 2024
| assert.Equal(t, err.Error(), "user not found") | ||
| assert.Equal(t, err, errSentinel) // Through `reflect.DeepEqual` causes error strings to be compared. | ||
| assert.NotEqual(t, err, errSentinel) | ||
| // etc. | ||
|
|
||
| ✅ assert.ErrorContains(t, err, "not found") | ||
| assert.EqualError(t, err, "user not found") |
There was a problem hiding this comment.
I'm a bit afraid this one is valid
There was a problem hiding this comment.
You probably need a rule that says
If I have
var ErrUserNotFound = errors.New("user not found")
...
assert.EqualError(t, err, "user not found")Then you shall replace it with
var ErrUserNotFound = errors.New("user not found")
...
assert.ErrorIs(t, err, ErrUserNotFound)There was a problem hiding this comment.
My point is that ErrorEqual should be part of the same checker than ErrorContains.
It should be discouraged
There was a problem hiding this comment.
Ok, we probably need to change namings then . Maybe error-compare for those two and something else for the use of Error()
There was a problem hiding this comment.
Maybe, let's wait for Anton's feedback
There was a problem hiding this comment.
@Antonboom maybe you missed this old thread
You said ErrorContains is an antipattern, what is your point of view about ErrorEqual ?
Pull Request Test Coverage Report for Build 9759437884Details
💛 - Coveralls |
mmorel-35
changed the title
Pull Request Test Coverage Report for Build 9759450840Details
💛 - Coveralls |
|
These names make more sense 👍 |
mmorel-35
force-pushed
the
contributing/error-compare
branch
from
9ae9d09
to
a209c77
Compare
Pull Request Test Coverage Report for Build 9826241707Details
💛 - Coveralls |
ccoVeille
approved these changes
Jul 7, 2024
mmorel-35
force-pushed
the
contributing/error-compare
branch
from
a209c77
to
e1e5f54
Compare
Pull Request Test Coverage Report for Build 10704183307Details
💛 - Coveralls |
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
mmorel-35
force-pushed
the
contributing/error-compare
branch
from
e1e5f54
to
4cb531e
Compare
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.
Following the different exchanges about error compare, I believe testifylint shall be allowed to recommend the use of ErrorContains, ErrorEqual, ErrorIs and NotErrorIs.
See #47 (comment) for more context
I believe ErrorIs is only applicable to wrapped errors but I might be missing something.