fix errorlint linter #50158
fix errorlint linter #50158
Conversation
37680b9 to
266e00f
Compare
|
CI looks happy, so it's probably fine to bring this in; my only concern is that changing @robmry @vvoland any thoughts / concerns? (such cases of course should've been caught in CI, but not sure if there's always cases to check for "unknown" situations) |
|
I don't know of anywhere the underlying error is deliberately masked ... but that just means I don't know, I wouldn't bet on CI catching that sort of thing. We don't know of any issues caused by the masking. There's also a lot of change, too much to review in any detail. So, yes - the The |
|
To reduce risk (for now), could you move the second commit (for |
|
@thaJeztah , |
| # Check for plain type assertions and type switches. | ||
| asserts: false |
There was a problem hiding this comment.
This one looks enabled by default, and I think this PR should fix these, or are there more remaining that are not yet fixed? If all of them are fixed, we can remove these lines.
There was a problem hiding this comment.
As I said initially in the description of the PR, I did not treated asserts because this needs a lot of change.
I believe it is better to have a dedicated one for that purpose.
There was a problem hiding this comment.
oh! I mis-interpreted assert as the ones that were fixed in this PR (use errors.Is / errors.As) 🙈
f54f7d2 to
ac8ee2d
Compare
313c4d9 to
ed30ee4
Compare
|
Probably unrelated, but in case it isn't; |
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com> Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
- What I did
Enables errorlint linter with
comparisonrule and fixes issues. Theerrorf,errorf-multiandassertrules are disabled because it requires a number of changes that deserves a dedicated PR- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)