colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.32k stars 88 forks source link

Allow collection setters to be generic over `Into` #214

Closed uint closed 2 years ago

uint commented 3 years ago

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

Closes #209

TedDriggs commented 3 years ago

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?

uint commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

TedDriggs commented 2 years ago

This merged as #234