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

Allow collection setters to be generic over Into #214

Closed
wants to merge 4 commits into from

Conversation

uint
Copy link
Contributor

@uint uint commented Jul 19, 2021

Apparently this works without doing any special trickery, at least on my version of the compiler.

Closes #209

@TedDriggs
Copy link
Collaborator

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?

Copy link
Contributor

@andy128k andy128k left a 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.

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());
Copy link
Contributor

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));

@uint
Copy link
Contributor Author

uint commented Aug 1, 2021

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.

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 into_pair syntax, like so:

#[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.

@TedDriggs
Copy link
Collaborator

TedDriggs commented Mar 1, 2022

I'm going to rebase this and address the conflicts. Given that each has been in the wild for some time now, I'm inclined to create a wrapper for Each that will enable it to read the current each = "..." form so there's no breaking change; #[builder(setter(each = "..."))] would be the same as #[builder(setter(each(name = "....")))]. @uint / @andy128k thoughts on that?

@andy128k
Copy link
Contributor

andy128k commented Mar 1, 2022

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.

@TedDriggs
Copy link
Collaborator

I've updated the branch here, but had some difficulty with strip_option and Option wrappers around collections.

@TedDriggs
Copy link
Collaborator

This merged as #234

@TedDriggs TedDriggs closed this Mar 3, 2022
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.

Allowing each and into on a vec
3 participants