-
-
Notifications
You must be signed in to change notification settings - Fork 34
Adding support for build_from and build_from_clone builder methods.
#313
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
base: master
Are you sure you want to change the base?
Conversation
…ibutes - Introduces two new builder methods: `build_from` (by reference) and `build_from_clone` (by value) - Both methods fill unset builder fields from an existing instance of the target type - Supports both struct and function builders - Includes integration tests and IDE completions
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.
There are a bunch of (quite a lot actually) edge cases that this needs to handle. Not saying we should handle them all right now, we can start with a good enough MVP and put it under the experimental-build-from flag for now and iterate further.
I'm still not feeling in love with the attribute names and syntax. I also considered nesting them under finish_fn like #[builder(finish_fn(build_from))] to reduce noise in the site docs, but this syntax doesn't look intuitive (seems like we are overriding the finish_fn instead of adding one more method. Btw. we should also update them (they are in the website directory in this repo). But IDK, I may change the naming if some great idea comes to mind a bit later
Could also be done via #[builder(derive(BuildFrom))] like it's a "derive" for a builder that adds one more method. Still not sure
| pub(crate) derive: DerivesConfig, | ||
|
|
||
| #[darling(default)] | ||
| pub(crate) build_from: bool, |
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.
These should use Option<Option<ItemSigConfig>> to make their names, visibility and docs configurable, plus inherit the visibility of the builder struct similar to how the regular finish_fn does that here:
| vis: finish_fn.vis.unwrap_or_else(|| builder_type.vis.clone()), |
(^ this is the place where a bunch of complex inter-field-dependent defaults are resolved)
Otherwise the docs would be incorrectly claiming that these attributes support vis, name, doc attributes.
I think this may require writing a custom with parser function that handles a syn::Meta::Path (in which case it returns None (i.e. Some(None))), and otherwise delegates to ItemSigConfig.
| quote! { | ||
| let #ident: #ty = match self.__unsafe_private_named.#index { | ||
| Some(value) => value, | ||
| None => from.#ident.clone(), |
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.
The idea is that build_from accepts T by value and does not call clone() at all (that's why this method's name is a bit shorter since it is less expensive and should be the preferred thing), but build_from_clone accepts &T and calls .clone() if needed (that's why it has "clone" in its name).
| .ident | ||
| .clone(); | ||
|
|
||
| Ok(quote_spanned! { span => #ident }) |
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.
This is a known footgun of quote_spanned!. It doesn't overwrite the span of interpolated tokens such as #ident - only the bare tokens created as part of that quasi-quotation syntax. But (1), otherwise, I don't see a need to override the span here. The span is preserved from the input by default, and that's desired. But (2) see my comment below about the body
| #ctor_path { | ||
| #( #ctor_args, )* | ||
| } |
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.
Unfortunatelly, this approach won't work in case if the builder is generated with the #[builder] macro on top of a function. We have the body.generate() in finish_fn.rs here that generates the proper body:
| let body = &self.finish_fn.body.generate(self); |
It's only requirement is that members are assigned to variables of the same name in scope. Plus we need to inherit the potential async / unsafe / const modifiers and maybe handle the case of the Result from the original function
| use crate::prelude::*; | ||
|
|
||
| #[derive(Builder, Clone)] | ||
| #[builder(build_from, build_from_clone)] |
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 think we should support method/function syntax too, unless you'd like to keep it supported only on struct derives initially.
For that, we should have tests for methods/functions too. In fact, bon's tests are quite repetitive (unfortunately) - we test all 3 cases of the syntax - structs, methods and free functions. I recommend following the naming conventions and patterns like here. E.g. test functions are called test_{struct,method,function}. Different test suites are separated into different test modules (inline modules can be used if tests are small). You may notice that sometimes, not all three cases are covered (if as a white-box we know the code under test is agnostic to the kind of syntax - struct or function - doesn't matter to it).
- Put build_from behind a feature flag - Added support for method syntax - Started on adding ItemSigConfig Pending Changes - Figure out how to reuse or integrate features from finish_fn - Complete ItemSigConfig support - Add unit tests
Add
build_fromandbuild_from_clonebuilder methodsCloses #310
This PR adds support for two new optional top-level builder attributes:
#[builder(build_from)]– Adds a.build_from(&T)method#[builder(build_from_clone)]– Adds a.build_from_clone(T)methodThese methods allow partially configured builders to fill in missing fields from an existing instance of the target type (
T) before finalizing the build. This is useful when:Default, making field reuse non-trivial.Example
Implementation notes
The new code lives in
builder_gen/build_from.rsand is only emitted if the attributes are enabled.Each field is handled according to its member kind:
Namedfields fallback tofrom.field.clone()if not explicitly set.FinishFnfields always defer tofrom.field.clone().Skipfields useDefault::default().Integration tests verify both methods.
Next steps
src/__/ide.rs; we may want to flesh out the corresponding links on the site.@Veetaha This implements the optional build_from and build_from_clone methods discussed in #310 — feedback welcome!