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
Extend prost-build to generate message builders #901
base: master
Are you sure you want to change the base?
Conversation
Notes on the present implementation:
|
a38d703
to
1fb7926
Compare
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.
I think this PR is difficult to review. Could you do these things to make it easier to review?
-
Can you explain why this APi is better then a generic builder? For example: https://crates.io/crates/derive_builder
-
Add documentation about how to use this builder.
-
Reorganize the commits so that they change a single thing with a description of what and why you do that. If that commit generally improves the codebase, consider opening a separate PR for just that commit. This allows me to review each commit individually.
-
I am not sure about this one: Is it beneficial to split the build code generator into a separate source file?
Thanks for the feedback @caspermeijn! I have rebased the branch and squashed the changes into a few topical commits.
I have added an explanation in the description comment.
I mean to do it, but first I'd like to settle on a more flexible way to configure the builders. prost_buid::Config::new()
// .mypackage.Foo gets foo::Builder and the associated Foo::builder() function
.builders(".mypackage", "Builder")
// ...
I was hesitant to make a separate module for this, as hardly any functionality used in Also, I feel that the builders should be a core feature, because as discussed in #399, this together with |
I could try to split off a PR with the refactoring changes in |
Done in #1011 and rebased on top. |
For each message, optionally generate a struct to provide the forward-compatible builder API, with inherent methods for field setters and the .build() method to produce the fully initialized message struct. If generation of builders is enabled with a name `Builder` for a message named `Foo`, an impl block with the associated fn `Foo::builder()` is appended to the top-level file scope, and the definition of the builder struct `Builder` and its methods is appended to the module `foo`. Reuse the helpers that generate field definitions for a message, but use a mode to output code for the builder fields, which must be private and devoid of docs and prost annotations. The added Config::builder method populates a PathMap with values given in the second argument to configure the builder name.
Adorn a struct with #[non_exhaustive] to showcase and test how the builder API (and Default) can be used to forward-compatibly construct values of generated message types, while the struct initializer syntax is forbidden. Some of the doc tests showcase working around name conflicts between the associated function generated for the builder API and a getter method generated by the Message derive for a field named `builder`. Also verify the no-conflict case where the field does not have a getter generated for it, so the `builder` associated fn is fine. The test crates in tests-2015 and tests-no-std are uniformly named "tests" to simplify boilerplate in the doc tests.
This allows plugging a message builder into other builders' setters for fields of the message type without the need to call .build().
The setter for field named `test` in proto must be named `r#test`.
First crack at #399
Based on #1011
Advantages over generic macro-generated builders like derive_builder:
Foo
does not need to be specially configured to not useOption<Foo>
as the argument type, we assume the setter is only used when the field is meant to be set so it'simpl Into<Foo>
.Option
, but no generics with theInto
bound here as that could obfuscate call sites more than necessary (maybe it's a wrong assumption and the argument conventions should be made uniform).impl IntoIterator
argument.i32
type that the corresponding struct field is generated with.