-
Notifications
You must be signed in to change notification settings - Fork 12
Simplify exports in Hermes SDK crates #595
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
Conversation
| tendermint-proto = { workspace = true } | ||
| hermes-error = { workspace = true } | ||
| hermes-relayer-components = { workspace = true } | ||
| hermes-core = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the purpose of hermes-core here.
Wouldn't it allow to reduce the number of dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it is a meta-crate to reduce the number of dependencies, so that high level crates can include just one crate instead of ten. Many popular crates like tokio are in fact meta crates that just re-export other smaller crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But those this mean we can remove the hermes-encoding-components and use hermes_core::encoding_components instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can. I could miss some of the renaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I commented before seeing the cli crates which use the new renaming/dependencies
ljoss17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I think the full renaming/replacing with hermes-core and hermes-cosmos-core can be postponed as there are some examples with the CLI crates.
Do you agree @rnbguy ?
Summary
This PR attempts to simplify the crate exports to reduce the code bloat in import statements. This is done mainly by removing the nested module namespaces, by re-exporting everything from the inner modules.
The PR also introduces the meta-crates
hermes-coreandhermes-cosmos-core, that can be included as a single package inside the Cargo.toml dependencies. This simplifies the logistics of adding Hermes SDK as a dependency, as developers have less need to remember the exact name of the smaller core crates.Evaluation
Looking at the changes summary, it looks like we only managed to save less than 500 lines of code from the massive changes on the import statements.
Although the impact is not large, I think it is still useful to have this PR and use the new convention moving forward. Hopefully, this can at least reduce some negative perception that CGP code requires overly excessive imports. The exercise shows that even if we don't split code into smaller modules, the effective lines of code savings from the "verbose" import statements (which are done mechanically by Rust Analyzer) is not actually that much.