-
Notifications
You must be signed in to change notification settings - Fork 89
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
Allow collection setters to be generic over Into
#214
Conversation
Thanks for submitting this; I haven't had the time to review it yet. @andy128k I believe you were the original author; can you take a look at these changes please? |
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 will be a breaking change, so a minor version bump is needed.
I would also like to see how/if this works with maps (HashMap
/BTreeMan
). I suppose maps with a String
key are pretty common and being able to use str
literals would be a huge improvement.
derive_builder_core/src/setter.rs
Outdated
if each.into { | ||
ty_params = quote!(<VALUE, FROM_VALUE: ::derive_builder::export::core::convert::Into<VALUE>>); | ||
param_ty = quote!(FROM_VALUE); | ||
into_item = quote!(item.into()); |
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.
into_item = quote!(::derive_builder::export::core::convert::Into::into(item));
I think for this to Just Work there would have to be some generic thing of this sort in the std: impl<T, U, FromT: Into<T>, FromU: Into<U>> From<(FromT, FromU)> for (T, U) { ... } Or maybe something like this? impl<T, U, FromT: Into<T>, FromU: Into<U>> Extend<(FromT, FromU)> for HashMap<T, U> { ... } But there is not. The best I can think of is adding a separate #[builder(setter(each(name = "bar", into_pair)))] On the bright side, this would let us avoid having to pass a tuple. We could make the function accept two arguments - key and value. |
I'm going to rebase this and address the conflicts. Given that |
Frankly, I don't like how it becomes lisp-ish with all those nested constructs, so I welcome idea to continue to support current (relatively) flat syntax. |
I've updated the branch here, but had some difficulty with |
This merged as #234 |
Apparently this works without doing any special trickery, at least on my version of the compiler.
Closes #209