-
Notifications
You must be signed in to change notification settings - Fork 38
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
refactor(node): add builder for Node #265
base: dev
Are you sure you want to change the base?
Conversation
Accomplishes three related goals: 1. remove unwraps() that were in Node::new() 2. requires that a Node be instantiated infallibly 3. improves ergonomics for instantiating a Node node.rs: * add BlockBodyError::ParseError variant * derive Builder for Node * refactor Node::new() into: + NodeBuilder::genesis_code() and + NodeBuilder::build() bench.rs, tests/network.rs, kindelia/src/main.rs: * use NodeBuilder::build() instead of Node::new()
hmm, CI is getting clippy errors that I don't get locally. Maybe it is using the new rust toolchain released a day or two ago?
There are a bunch of these type of errors unrelated to this PR, so I think they should be fixed in a separate PR. |
I think this is a nice refactoring, but I'm also scared by the size of "boilerplate" code (I don't know if that would be the most correct term) that was needed to create this builder. I think it definitely seems simpler to build a node using this method, even more so when default values are expected, but all the "boilerplate" (?) needed to create this builder is overcomplicating the node construction code. Of course, the code was not the simplest before, but now, in addition to understanding what is happening in the construction of the node, it would also be necessary to understand the macros in the properties and the new functions created for the builder to be possible (although it is all very well documented, I know some people who would criticize for the "issues" I just described). For me this PR would be approved, but this time I think I need a third opinion to evaluate these points. |
Yeah, I don't like adding any unnecessary code either. In this case, I figure that a builder is warranted. First, I always consider there to be some code-smell if a Thing::new() returns a Result. I find it much cleaner if Thing::new() is infallible, or indeed ::new() does not need to exist at all. So I didn't want to add a Result to Node::new(), and yet it was doing fallible things that had to go somewhere. So those fallible things either had remain in new(), move to caller(s), or go into a builder of some type. Second, any time there is a struct with a lot of properties, including some that are optional/defaultable, a builder can be considered. It can make usage much more ergonomic for the caller. Given that Node is a huge struct and is central to everything, I think it is worth making usage as ergonomic and idiomatic as possible. An indicator of the code-smell in this area is the Now, in the past, I've made my own builders, so this is the first time I've worked with derive_builder. I found it quite nice except for the limitation that the builder cannot have any properties not in the target struct, so I opened an issue about that. If they were to add the requested feature, I could make the builder API cleaner for callers. Of course, I could also do it right now by manually implementing a builder, and perhaps that is the best thing here. That would remove all the Builder annotations from Node and might result in less total compiled code. |
Yes, that was embarrassing.
It's worth noting that at the moment the node is only actually created in our code in the |
So I'm not totally happy, with this derive_builder impl either. Example usage in this PR:
What I would like to see instead is:
Notice that data_path() and genesis_code() are now separate items that do not return any result and that build() accepts no params. So all fallible work is delayed until ::build() when all necessary inputs have been set. The whole thing is just cleaner. Since derive_builder() doesn't support this usage yet, I think I will add a new commit to this PR with a custom builder that works per above -- for your consideration. It might take a day or two though given the holidays.
This is exactly where I'm coming from. I think Kindelia is and will be an important project/ecosystem and that lots of people will be using it in the future after mainnet, so I consider these crates to be libraries and not just internal components of kindelia cli. Thus it's good to to polish and clearly doc the public API. |
marking this as draft. #274 is preferred. |
Accomplishes three related goals:
node.rs:
bench.rs, tests/network.rs, kindelia/src/main.rs: