fix(catalog,manifest): handle some ignored errors #670
Conversation
Also enable errcheck linter for future. Ignore table printer errors for cli which are self observable and mostly fine. Ignoring it in tests. There are many violations, could be enabled separately if needed. Signed-off-by: ferhat elmas <[email protected]>
ferhatelmas
force-pushed
the
handle-more-errors
branch
from
eb82257 to
9ddcab7
Compare
ferhatelmas
changed the title
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
just a couple questions, but otherwise looks good
| dec := json.NewDecoder(rsp.Body) | ||
| dec.Decode(&struct { | ||
| Error *errorResponse `json:"error"` | ||
| }{Error: &e}) |
There was a problem hiding this comment.
was this erroring or causing an issue somewhere? I assumed that if the body was empty then this would essentially be a no-op and nothing would happen.
There was a problem hiding this comment.
I was reviewing errors in the context of #667 and thought enabling errcheck would be better. This is one of the reported failures so trying to address
if the body was empty then this would essentially be a no-op and nothing would happen
that's correct but if there is a body and it fails to decode, it would be ignored, now it's more explicit
| err = db.RunInTx(ctx, &sql.TxOptions{ReadOnly: true}, func(ctx context.Context, tx bun.Tx) error { | ||
| result, err = fn(ctx, tx) | ||
|
|
||
| return err | ||
| }) |
There was a problem hiding this comment.
would this err = conflict with the fact that we set err inside of the anonymous function?
There was a problem hiding this comment.
it won't because it will be set at last with correct error (i.e. commit)
2 participants
Also enable errcheck linter for future.
Ignore table printer errors for cli which are
self observable and mostly fine. Ignoring it
in tests. There are many violations, could be
enabled separately if needed.