Skip to content
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

Use encoder/registry to initialize encoder types #2359

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

weiihann
Copy link
Contributor

@weiihann weiihann commented Jan 6, 2025

init() should be avoided as much as possible, but we should make an exception for initializing encoder types.

The problem with the old approach of registering types to encoder in the blockchain package is that, any other packages may need to explicitly call RegisterCoreTypesToEncoder() if the blockchain object isn't created anywhere. This doesn't help with code readability. Moreover, in the tests, as the types aren't registered automatically, we either have to register types individually, call RegisterCoreTypesToEncoder() or do something like TestMain() as shown in state_test.go.

The new approach creates a new package encoder/registry with an init() function that does the type registration. This package is blank imported in the main package, so the types are initialized before main() runs. For the test files, init_test.go are created for each package, where all it does is blank import.

Another alternative to this approach of test files is to create a new testing package (i.e. github.com/NethermindEth/juno/testing) with the blank import and encapsulate the built-in testing package. All tests should use this new testing package so that the types are initialized properly. I feel that this approach is slightly overengineering so I went with the former approach.

Uber's go style guide also mentioned that we can make an exception for encoder type registry.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.63%. Comparing base (6b4d132) to head (b68f718).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2359      +/-   ##
==========================================
+ Coverage   74.56%   74.63%   +0.06%     
==========================================
  Files         110      110              
  Lines       11895    11893       -2     
==========================================
+ Hits         8870     8876       +6     
+ Misses       2337     2331       -6     
+ Partials      688      686       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@weiihann weiihann force-pushed the weiihann/init-encoder branch from 5c2a241 to b68f718 Compare January 6, 2025 09:43
@weiihann weiihann merged commit 8a9b746 into main Jan 6, 2025
13 checks passed
@weiihann weiihann deleted the weiihann/init-encoder branch January 6, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants