-
Notifications
You must be signed in to change notification settings - Fork 52
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
Introduce base builder type-alias #55
base: master
Are you sure you want to change the base?
Conversation
|
|
@@ -126,6 +126,13 @@ impl<'a> StructInfo<'a> { | |||
quote!(#[doc(hidden)]) | |||
}; | |||
|
|||
let mut b_decl_generics = b_generics.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.
Why is this required? b_generics
is only used when declaring the builder type (and you want the default there) and when splitting it for the impl
block (where defaults should not be looked at anyway). So why clone it instead of modifying the original?
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.
That's redundant, indeed.
@@ -126,6 +126,13 @@ impl<'a> StructInfo<'a> { | |||
quote!(#[doc(hidden)]) | |||
}; | |||
|
|||
let mut b_decl_generics = b_generics.clone(); | |||
for param in &mut b_decl_generics.params { | |||
if let syn::GenericParam::Type(ref mut param) = param { |
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.
Wouldn't this match all type parameters? The one you want to add a default to is always the first one, so you can just modify that.
Or even better - just add the default when creating b_generics
.
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.
Yes, that would match all parameters. I guess that barely make sense.
Though unfortunately rust doesn't allow to specify defaults for non-trailing generics and the TypedBuilderFields
is the first one. I'm not sure whether it's fine to set it as the last one, perhaps it might break some code.
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 half minded to just go with your original design as a temporary solution. Once const generics support strings I plan on doing a great refactor that'll break anything (other than regular by-the-book usage) anyway, and once that come I may remove it in favor of something else.
It's quite handy to have an alias rather than manually recreating the internal type.