-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvement ideas #595
Comments
The comment is valid, but syntax-wise we we wrap errors (they ported much of that from weave), so it would be Whether we want to wrap on every level is a question for Alex |
Good suggestions. Sorry for my late response
Unless we fail the build it is hard to enforce this. My IDE handles/hides imports for me. I will check if there is a way to configure this.
We avoid "naked" errors mostly. Let's have an 👁️ on this and improve the code as we go. As Ethan wrote:
This was one of the test patterns that evolved. They are effective but I see your point that this is hard to read. 🤔 Let's find some better template as we go and discuss.
This is some cosmos module "standard" to provide them in the module root. This avoids some imports in the "/app" . The alias generator does not work on my box and seems not to be maintained. I had already stopped adding more to 'alias.go`. ;-) |
This issue is a container for ideas on how the application and the code could be improved. Not everything might be valid as I am learning the code base.
use
goimport
as your code formatter 😉Always wrap returned errors. Doing
if err != nil { return err }
does not include each callers' context. Pushing errors up the stack without context makes it harder to test and debug. Additionally, a short context description makes it easier for the reader to understand the code. Example:wasmd/x/wasm/keeper/keeper.go
Lines 334 to 338 in 93e2e66
It would be an improvement to return
Please notice that
fmt.Errorf
is not used, because the error handling predatesfmt.Errorf
anderrors.Is
Update tests so that mock objects own the data they modify. To describe the case, let's focus on a single test:
wasmd/x/wasm/keeper/handler_plugin_test.go
Lines 72 to 75 in 93e2e66
gotMsgs
is a reference to a slice that is carried and modified byNewCapturingMessageHandler
. Because it is used in all test cases, internal state of the mock requires resetting.I think it would be cleaner to apply two modifications to such test:
1. make each mock own its internal state. In this case, instead of passing
gotMsgs
reference, let the mock own the slice and expose it via an attribute or a method.2. where can be done, move mock creation inside of each test case. Sharing mutable mocks breaks the islocation and tests can influence one another. When mocks are isolated, there is no need to reset them.
Limit the use of aliases, when not used during the refactoring process.
I only briefly know the alias functionality story in Go. Apparently in 1.9 the current implementation fianlly landed (issue and proposal).
My understanding of alias functionality is that this functionality is meant as a temporary mean during big code base refactoring. Using an alias allows gradually introducing an otherwise breaking change.
x/wasmd
has a dedicatedalias.go
file. It seems like this file is a conscious decision rather than an in process refactoring state. Not sure what are consequences of this approach, maybe it is something worth looking deeper into.The text was updated successfully, but these errors were encountered: