Support custom fields in the builder struct #245
Conversation
|
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. |
There was a problem hiding this comment.
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 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. |
As per colin-kiegel#245 (comment) This may yet go away (or something), apropos further review comments.
|
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. |
| .as_expressed_vis() | ||
| .or_else(|| self.parent.field.as_expressed_vis()) | ||
| .unwrap_or(Visibility::Inherited) | ||
| if !self.field_enabled() { |
There was a problem hiding this comment.
Why does this now preempt the #[builder(field(...))] visibility settings?
There was a problem hiding this comment.
I have added a comment about this. This preserves the behaviour from before my MR. Previously this private visibility was dealt with in ad-hoc code in the builder struct code generator. Now, the field type comes from BuilderFieldType, and the code generator always uses the visibility which comes from field_vis, so field_vis must return the desired answer.
There was a problem hiding this comment.
First: Great comment.
Second: I agree with removing the ad-hoc hiding in codegen, so let's keep the logic for visibility all here.
Third: With #247, I feel like explicit field-level visibility should overrule the inferred visibility from being a phantom. How would you feel about this checking self.field.field.as_expressed_vis(), then checking if the field is enabled, then checking the struct-level? Or should it check those first with the normal inheritance chain and finally fall back to inferring Inherited vis if nothing is pushing to make it public?
There was a problem hiding this comment.
I feel like explicit field-level visibility should overrule the inferred visibility
Good point. I agree completely. about explicitly-specified field-level visibility, But, I think, only field-level, not struct-level visibility.
Ie, abolish the nesting in custom(). Incidentally, this now makes it possible to specify `build` without `type`. As per colin-kiegel#245 (comment)
And justify why we don't just put in Option<T>. Prompted by colin-kiegel#245 (comment)
|
I am going to rebase this onto master now, to resolve the conflict. I won't make other changes as part of the rebase. |
As per colin-kiegel#245 (comment) This may yet go away (or something), apropos further review comments.
Ie, abolish the nesting in custom(). Incidentally, this now makes it possible to specify `build` without `type`. As per colin-kiegel#245 (comment)
And justify why we don't just put in Option<T>. Prompted by colin-kiegel#245 (comment)
Make the test one which compiles if the default= is removed. As per colin-kiegel#245 (comment)
Co-authored-by: Ted Driggs <ted.driggs@outlook.com>
Not sure where this came from.
|
Thanks for the detailed review! I think I have dealt with all your comments. |
Co-authored-by: Ted Driggs <ted.driggs@outlook.com>
Co-authored-by: Ted Driggs <ted.driggs@outlook.com>
Co-authored-by: Ted Driggs <ted.driggs@outlook.com>
Cool, thanks. Your suggestions looked good to me so I just committed them via the github UI. |
There was a problem hiding this comment.
@ijackson what a heroic effort, and a great result.
I appreciate that you split your work out by commit, but I'd rather not add all 72 as individual commits to master. Would you like to do an interactive rebase locally to organize them, or would you prefer I squash and merge?
|
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. |
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]serializeof 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).