sql/*: fix improperly wrapped errors #72350
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/alter_column_type.go, line 410 at r1 (raw file):
var x = `se `
what's this about?
pkg/sql/type_change_test.go, line 585 at r1 (raw file):
} if !testutils.IsError(err, "invalid input value for enum") { return errors.AssertionFailedf("expected invalid input for enum error, found %v", err)
I think you've got another case to add to your linter. Shouldn't this use errors.NewAssertionErrorWithWrappedErrf
There was a problem hiding this comment.
Reviewable status:
pkg/sql/alter_column_type.go, line 410 at r1 (raw file):
Previously, ajwerner wrote…
var x = `se `what's this about?
no idea; nice catch
pkg/sql/type_change_test.go, line 585 at r1 (raw file):
Previously, ajwerner wrote…
I think you've got another case to add to your linter. Shouldn't this use
errors.NewAssertionErrorWithWrappedErrf
hm so this one i wasn't quite sure what to do. not wrapping the error here made sense to me since the problem in this case is that an error was expected, but it was the wrong type. but NewAssertionErrorWithWrappedErrf seems like the best of both worlds; didn't know about that.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/type_change_test.go, line 585 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
hm so this one i wasn't quite sure what to do. not wrapping the error here made sense to me since the problem in this case is that an error was expected, but it was the wrong type. but
NewAssertionErrorWithWrappedErrfseems like the best of both worlds; didn't know about that.
and i'll make sure my linter looks for this function too. (will send that PR up once these fix PRs are in)
rafiss
force-pushed
the
fix-sql-errwrap
branch
3 times, most recently
from
1ef44e0 to
26e23af
Compare
I'm working on a linter that detects errors that are not wrapped correctly, and it discovered these. Release note: None
rafiss
force-pushed
the
fix-sql-errwrap
branch
from
26e23af to
2ccf86b
Compare
|
bors r=ajwerner |
|
Build failed: |
|
bors r=ajwerner |
|
Build failed: |
|
bors r=ajwerner |
|
Build failed (retrying...): |
|
Build succeeded: |
71877: lint: add new errwrap linter r=ajwerner,knz a=rafiss fixes #42510 This linter checks if we don't correctly wrap errors. The `/* nolint:errwrap */` comment can be used to disable the linter inline. See individual commits for mistakes this linter caught. It had already caught a few in #72353, #72352, #72351, #72350, and #72349. Release note: None Co-authored-by: Rafi Shamim <[email protected]>
refs #42510
I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.
Release note: None