server,cli: fix improperly wrapped errors#72352
server,cli: fix improperly wrapped errors#72352craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
knz
left a comment
There was a problem hiding this comment.
very nice. I already love your linter.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/cli/clisqlclient/conn_test.go, line 68 at r1 (raw file):
testutils.SucceedsSoon(t, func() error { if sqlRows, err := conn.Query(`SELECT 1`, nil); !errors.Is(err, driver.ErrBadConn) { return errors.AssertionFailedf("expected ErrBadConn, got %v", err)
NewAssertionErrorWithWrappedErrf? same below
pkg/server/drain_test.go, line 90 at r1 (raw file):
return nil } return errors.AssertionFailedf("server not yet refusing RPC, got %v", err)
assertion with wrap
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlclient/conn_test.go, line 68 at r1 (raw file):
Previously, knz (kena) wrote…
NewAssertionErrorWithWrappedErrf? same below
Done.
pkg/server/drain_test.go, line 90 at r1 (raw file):
Previously, knz (kena) wrote…
assertion with wrap
Done.
330685b to
b12219b
Compare
|
tftr! bors r=knz |
|
Build failed: |
|
bors r=knz |
|
Build failed: |
|
bors r=knz |
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
|
made a mistake in review |
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/drain_test.go, line 90 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
Done.
please revert this one. we want an error to be returned by this function when Dial returns a nil error.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/drain_test.go, line 90 at r1 (raw file):
Previously, knz (kena) wrote…
please revert this one. we want an error to be returned by this function when Dial returns a nil error.
not sure what the best way to do this is while still keeping the forthcoming linter happy. i could do
errS := "<nil>"
if err != nil {
errS = err.Error()
}
return errors.Newf("server not yet refusing RPC, got %s", errS)
which seems gross.
should i keep it as is and instead allow a //nolint directive similar to
|
yes a nolint comment in this case seems appropriate. if err != nil {
if grpc.XXX(err) {
return nil
}
return errors.NewAssertion...(err, "...")
}
// nil err
return errors.AssertionFailed("dial did not fail") |
|
thanks, i'll add |
I'm working on a linter that detects errors that are not wrapped correctly, and it discovered these. Release note: None
b12219b to
2407d3f
Compare
|
tftr! bors r=knz |
|
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