Ensure consistent errors #147
Conversation
bznein
left a comment
bznein
left a comment
There was a problem hiding this comment.
LGTM but unless I misunderstood something we can achieve even more consistency in our error handling
| Password: password, | ||
| })); err != nil { | ||
| t.Fatal(fmt.Sprintf("Error connecting to MongoDB deployment: %+v", err)) | ||
| t.Fatal(fmt.Sprintf("Error connecting to MongoDB deployment: %s", err)) |
There was a problem hiding this comment.
[q] is there a specific reason here we use fmt.Sprintf? A few lines below we have the same structure (t.Fatal) with inside a errors.Errorf
| case <-time.After(interval): | ||
| if err := connectFunc(); err != nil { | ||
| t.Fatal(fmt.Sprintf("error reaching MongoDB deployment: %+v", err)) | ||
| t.Fatal(fmt.Sprintf("error reaching MongoDB deployment: %s", err)) |
There was a problem hiding this comment.
Same as the previous comment
|
|
||
| if err := setup.CreateTLSResources(mdb.Namespace, ctx); err != nil { | ||
| t.Fatalf("Failed to set up TLS resources: %+v", err) | ||
| t.Fatalf("Failed to set up TLS resources: %s", err) |
There was a problem hiding this comment.
Seeing this and referring to my other two comments, maybe using t.Fatalf every time would be better? I feel we have a mixture of three different approaches at the moment:
t.Fatalf
t.Fatal(fmt.Sprintf(...))
t.Fatal(errors.Errorf(...))
There was a problem hiding this comment.
yes I agree! I will update to use one. I think t.Fatalf is the most straightfoward, what do you think?
There was a problem hiding this comment.
I think so! No need to nest function calls!
All Submissions:
This PR updates the codebase to use
pkg/errors.Errorfin place offmt.Errorfthe format strings have been updated to use
%s(the intended mechanism for handling errors in format strings)