search

colin-kiegel / rust-derive-builder

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

Support custom fields in the builder struct #245

Closed ijackson closed 2 years ago

ijackson commented 2 years ago

In Arti, we want our builder structs to sometimes contain particular types that we specify, other than Option<TargetFieldType>. This will allow us to control what the [De]serialize of the builder looks like.

There are a number of other conceivable uses for this escape hatch. The internal machinery to implement this could also be used for other feature(s).

TedDriggs commented 2 years ago

FYI, given the size of this PR I probably won't be able to get to it until next week.

ijackson commented 2 years ago

FYI, given the size of this PR I probably won't be able to get to it until next week.

Sure, thanks for the update. FYI I will be away next week, so I will hope to read your thoughts when I get back. I hope you find my structuring into individual commits helpful.

ijackson commented 2 years ago

This is quality work. I'm on the fence about whether this pushes derive_builder too far into the realm of arbitrary code generation - or if there's a way to unbundle what derive_builder does such that these advanced scenarios feel like composition rather than ever-deeper modification.

Thanks for the review. I need to read the details again properly to get to grips with things (and remind myself of the structure...)

I too find the shape of this escape hatch rather uncomfortable. It's not super convenient and the syntax is awkward. But I think it is necessary precisely to enable composition: allowing the user to insert arbitrary fields into the builder structure, thus letting the user compose autogenerated code from derive_builder with their own handling of special cases. The new plumbing in derive_builder is close to the minimum necessary to make that work.

I considered whether it woudl be possible to allow adding fields to the builder without any corresponding field in the output structure; but that would involve smuggling the whole definition of a field through magic attributes. Or providing an attribute macro, which can of course filter the input arbitrarily. I didn't like either of those ideas.

ijackson commented 2 years ago

I think I've dealt with all your comments now. There are several where I think your further opinion is needed; the others I have marked "resolved". Thanks.

ijackson commented 2 years ago

I am going to rebase this onto master now, to resolve the conflict. I won't make other changes as part of the rebase.

ijackson commented 2 years ago

Hi. Just got back to this after the long weekend here for Easter. Thanks for your comments, which I'll work on now.

ijackson commented 2 years ago

Rebased onto #245, sorted out textual conflicts, with a final commit at the end to fix two semantic conflicts.

ijackson commented 2 years ago

Thanks for the detailed review! I think I have dealt with all your comments.

ijackson commented 2 years ago

All substantive feedback is addressed; there are a couple minor changes and then we'll be ready to merge.

Cool, thanks. Your suggestions looked good to me so I just committed them via the github UI.

ijackson commented 2 years ago

Thanks :-). I think it's probably best to squash, unless you would prefer to retain a more detailed history. If the latter, let me know.

TedDriggs commented 2 years ago

Squashing now