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

Nested builder support #253

Closed
wants to merge 9 commits into from
Closed

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented Apr 21, 2022

This builds on the codegen #245 to provide the ability to sensibly nest structs with builders. With this, our code in Arti can do this:

#[derive(Clone, Builder, Debug, Eq, PartialEq, AsRef)]
#[builder(build_fn(error = "ConfigBuildError"))]
#[builder(derive(Deserialize))]
pub struct TorClientConfig {
    /// Information about the Tor network we want to connect to.
    #[builder(sub_builder)]
    tor_network: dir::NetworkConfig,

with the result that:

  • TorClientConfigBuilder contains a NetworkConfigBuilder.
  • Callers can do things like tor_client_config_builder.tor_network().fallback_caches(...)
  • There is no need for anyone to write out the .build() calls for the sub-structures. tor_client_config_builder.build() does it.
  • impl TorClientConfigBuilder { pub fn tor_network(&mut self) -> &mut NetworkConfigBuilder { ... } } can now be autogenerated rather than our current handwritten methods

sub_builder changes defaults for the contained field type and how to construct the target field, but you can still override them with the features from #245

I guess you will have some opinions about the shape of the field attribute syntax...

Thanks again for your attention!

This evaluation is in a priority order.  IMO this new approach is
clearer, and we're going to add another branch which doesn't fit into
the match pattern.
Everything here *could* be done with `#[builder(field(type,build)]]`
and `#[builder(setter(custom))]` and handwriting the accessor
methods.  But that's very clumsy.
Use this when custom conversions fail.  Provide a conversion to the
autogenerated field type.  Abolish the ad-hoc error in the test.
@TedDriggs
Copy link
Collaborator

Can you file an issue for this proposal?

I'd like to better understand the use-case and available alternatives before reviewing a particular implementation.

@ijackson ijackson mentioned this pull request Apr 22, 2022
fabricedesre pushed a commit to capyloon/arti that referenced this pull request May 7, 2022
This commitid is the current head of my MR branch
  colin-kiegel/rust-derive-builder#253
  https://github.com/ijackson/rust-derive-builder/tree/field-builder
Using the commitid prevents surprises if that branch is updated.

We will require this newer version of derive_builder.  The version
will need to be bumped again later, assuming the upstream MR is merged
and upstream do a release containing the needed changes.

We will need the new version of not only `derive_builder_core` (the
main macro implementation) but also`derive_builder` for a new error
type.
@ijackson ijackson closed this Aug 17, 2022
@ijackson ijackson deleted the field-builder branch August 17, 2022 15:13
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.

None yet

2 participants