general QGB cosmetics #576
Conversation
Codecov Report
@@ Coverage Diff @@
## qgb-integration #576 +/- ##
===============================================
Coverage 9.76% 9.76%
===============================================
Files 55 55
Lines 9036 9036
===============================================
Hits 882 882
Misses 8075 8075
Partials 79 79
Help us with your feedback. Take ten seconds to tell us how you rate us. |
e2e/qgb/errors.go
Outdated
| @@ -2,4 +2,14 @@ package e2e | |||
|
|
|||
| import "errors" | |||
|
|
|||
| var ErrNetworkStopped = errors.New("network is stopping") | |||
| var ( | |||
| ErrNetworkStopped = errors.New("network is stopping") | |||
There was a problem hiding this comment.
nit: we should probably match the tense of the name of the variable with the error message. So like, "network has stopped" or ErrNetworkingStopping. Where is this used btw?
e2e/qgb/errors.go
Outdated
| var ErrNetworkStopped = errors.New("network is stopping") | ||
| var ( | ||
| ErrNetworkStopped = errors.New("network is stopping") | ||
| ErrRelayerStart = errors.New("relayer didn't start correctly") |
There was a problem hiding this comment.
nit: perhaps we could be more specific, like since we use this when we reach a deadline, we could simply reflect that
e2e/qgb/errors.go
Outdated
| ErrValsetNotFound = errors.New("couldn't find valset") | ||
| ErrHeightNotReached = errors.New("couldn't reach wanted heigh") | ||
| ErrNodeStart = errors.New("node didn't start correctly") | ||
| ErrEmpty = errors.New("empty") |
There was a problem hiding this comment.
nit: this error message might be too abstract considering we have to wrap it each time we use it. empty what? If we don't want to create a flag for each thing like EmptyFlag or EmptyService, which might be going overboard, then we can just do what we're doing now but keep not wrap this error. pls see other comments for examples
e2e/qgb/qgb_network.go
Outdated
| return fmt.Errorf("empty list of services provided") | ||
| return fmt.Errorf("list of services provided: %w", ErrEmpty) |
There was a problem hiding this comment.
nit: I actually think we don't need this change as the first is more descriptive and wrapping the word empty doesn't really give us anything. see other comment above
x/qgb/orchestrator/config.go
Outdated
| return orchestratorConfig{}, errors.New("private key flag required") | ||
| return orchestratorConfig{}, fmt.Errorf("%s: %w", privateKeyFlag, ErrEmpty) |
There was a problem hiding this comment.
nit: similar to the other comments, I'm not sure this change is actually better from both from a user's point of view and a reader of the code. The previous error message is really good cause it tells the user exactly what they need to do to fix the issue while also not making the readers of the code lookup what ErrEmpty says/means.
|
Thanks for the comments. Probably, it would be best to remove the errors from this PR and handle them separately. What do you think? @evan-forbes @rootulp |
sounds good to me 👍 @sweexordious |
Description
Cosmetics
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes