sql/*: fix improperly wrapped errors#72350
Conversation
|
This change is |
ajwerner
left a comment
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
rafiss
left a comment
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.
rafiss
left a comment
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)
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
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 <rafi@cockroachlabs.com>
refs #42510
I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.
Release note: None