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

impl a custom builder for Node #274

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

dan-da
Copy link

@dan-da dan-da commented Dec 24, 2022

addresses #247. (more to come)

This PR is intended to replace #265, or at least provide an alternative to it, for comparison by reviewer(s). I personally consider this version nicer/cleaner.

Here we impl a custom NodeBuilder that is more ergonomic for the caller than the version derived by derive_builder.

The builder pattern by its very nature involves some boilerplate with the setters, so there's no getting around that, but I've tried to keep it as simple as possible.

I considered putting NodeBuilder inside node.rs, but my reasoning is that node.rs is already quite large, and using a dedicated builder.rs we are able to remove a bit of code from it, rather than add to.

I included the previous commit that uses derive_builder to generate NodeBuilder, because I figure we might want to go back to it at some point, if derive_builder crate ever supports custom/transient properties as I requested.

I provided some basic doc-comments for the builder. Those more familiar with the codebase may wish to flesh the docs out more.

If the builder approach is deemed too heavy, then I guess the alternative is to use the pre-existing Node::new() except it must return a Result. I am willing to do that, but I do find this approach more flexible and pleasing. :-)


commit message:

Implements NodeBuilder which provides more ergonomic usage for callers
than the derive_builder version, and also keeps Node clean without
a lot of derive_builder annotations.

builder.rs: new file added

node.rs:
* remove derive_builder annotations
* remove NodeBuilder impl
* remove ParseError variant from BlockBodyError
* add comment that Node should be built with NodeBuilder

util.rs
* make EpochError pub. It can bereturned by NodeBuilder::build()

lib.rs:
* add builder mod

tests, bench, kindelia/src/main:
* adjust usage of NodeBuilder

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()
Implements NodeBuilder which provides more ergonomic usage for callers
than the derive_builder version, and also keeps Node clean without
a lot of derive_builder annotations.

builder.rs: new file added

node.rs:
 * remove derive_builder annotations
 * remove NodeBuilder impl
 * remove ParseError variant from BlockBodyError
 * add comment that Node should be built with NodeBuilder

util.rs
 * make EpochError pub. It can bereturned by NodeBuilder::build()

lib.rs:
 * add builder mod

tests, bench, kindelia/src/main:
 * adjust usage of NodeBuilder
@dan-da dan-da marked this pull request as draft December 24, 2022 22:31
@dan-da dan-da marked this pull request as ready for review December 24, 2022 22:31
@racs4 racs4 self-requested a review January 2, 2023 12:43
Copy link
Contributor

@racs4 racs4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way looks better than using derive_builder, the code is clearer in this case, for some new user and it removes the helper functions that were created earlier for some properties. Although the boilerplate remains, it seems more self-contained and simpler to understand in comparison to #265.

kindelia_core/src/builder.rs Show resolved Hide resolved
@racs4 racs4 requested a review from steinerkelvin January 2, 2023 13:19
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.

2 participants