Added #[builder(derive(...)] for #43 #84

TedDriggs · 2017-04-17T21:45:37Z

Addresses #43; enables builder to have its own additional derive statements. This PR does not include attribute forwarding on fields; there are still good questions about the future of that in #79. This does add immediate value for debugging and testing, as it allows generation of equality tests and debug output.

colin-kiegel · 2017-04-18T21:05:56Z

Cool, will have a look the next days (hopefully soon)! :-)

colin-kiegel

Looks good.

However, I'd like to discuss, how we want to treat the auto-derivation of Debug and Clone. I feel like we can improve the story a little here. :-)

colin-kiegel · 2017-04-21T06:02:25Z

derive_builder/README.md

@@ -93,6 +93,7 @@ with [cargo-edit](https://github.com/killercup/cargo-edit):
 * **Setter type conversions**: With `#[builder(setter(into))]`, setter methods will be generic over the input types – you can then supply every argument that implements the [`Into`][into] trait for the field type.
 * **Generic structs**: Are also supported, but you **must not** use a type parameter named `VALUE`, if you also activate setter type conversions.
 * **Default values**: You can use `#[builder(default)]` to delegate to the `Default` implementation or any explicit value via `=".."`. This works both on the struct and field level.
+* **Builder derivations**: You can have the builder derive additonal traits using `#[builder(derive(Trait1, Trait2, ...))]`.


in README.md I prefer example style, e.g.
You can use `#[builder(derive(Copy, PartialEq, ...))]` to derive additional traits on the builder struct.

colin-kiegel · 2017-04-21T06:07:27Z

derive_builder/tests/derive_trait.rs

+struct NotPartialEq(String);
+
+#[derive(Debug, Clone, Builder)]
+#[builder(derive(Debug, PartialEq, Eq))]


please add tests that (one of these) is actually implemented.

I actually have that; the defaults test down below will fail if LoremBuilder doesn't implement PartialEq and Debug. I've made this more explicit with a comment in the test.

colin-kiegel · 2017-04-21T06:14:48Z

derive_builder/src/lib.rs

+//! }
+//! ```
+//!
+//! Attributes declared for those crates are _not_ forwarded to the fields on the builder.


What do you mean by crates here? Do you mean struct instead?

Please add another note, that we derive Default and Clone on the builder unconditionally. Well, unless we change that ^^Alternatively this would be our chance to say Default and Clone are only derived automatically if the user didn't specify #[builder(derive)]. This would allow to opt-out of these derivations - but I feel like they don't do any harm. I lean towards unconditionally deriving them. What do you think?

That should be "traits", not "crates" - I'll fix that. I agree with unconditional derivation for the Debug and Clone traits.

colin-kiegel · 2017-04-21T06:17:10Z

derive_builder_core/src/builder.rs

@@ -80,7 +83,7 @@ impl<'a> ToTokens for Builder<'a> {
                   impl_generics);

            tokens.append(quote!(
-                #[derive(Default, Clone)]
+                #[derive(Default, Clone #( , #derives)* )]


I think it would be cleaner, if we move Default and Clone into the derive options - even, if we set it unconditionally.

PS: In that case also remove Default and Clone from the unit tests - since we already have the testcase add_derives.

I thought about this, but as long as these are going to be unconditionally present it felt strange to hide them far from the actual rendering of the builder.

TL;DR: Ok,let's just keep it as is :-)

My line of thinking was this:

derive_builder_core is just a toolkit for code generation and should make minimal assumptions

derive_builder populates the settings and sets all defaults - i.e. most decisions should be here

But I just realized that derive_builder_core assumes the builder to implement Clone for immutable/mutable setters. So we already know in the scope of derive_builder_core that we need Clone for consistency. With this minimal reasoning rule of thumb the decision about deriving Clone should in fact stay in derive_builder_core. Now if only had Default, we could move at least that to derive_builder. But since we have both - let's just keep it as is. :-)

colin-kiegel · 2017-04-21T06:24:11Z

derive_builder/src/options/struct_mode.rs

@@ -97,6 +124,7 @@ impl From<OptionsBuilder<StructMode>> for (StructOptions, OptionsBuilder<FieldMo
            builder_visibility: m.builder_vis.unwrap_or(m.build_target_vis),
            builder_pattern: b.builder_pattern.unwrap_or_default(),
            build_target_ident: syn::Ident::new(m.build_target_name),
+            derives: m.derive_traits.unwrap_or_default(),


Depending on which behaviour we want, I'd suggest either

unconditionally push Debug and Clone onto the derives vector

only push Debug and Clone if they are not yet in the vector

or default to [`Debug`, `Clone`], i.e. unwrap_or(vec![syn::Ident::new("Debug"), syn::Ident::new("Clone")])

PS: I lean towards 1 or 2.

If we pick 2, we could emit a (deprecation) warning which informs the user that Debug and Clone derives are already implicit and that deriving them explicitly is redundant? (deprecations and panics are the only ways we can currently communicate with the user).

colin-kiegel · 2017-04-21T06:35:57Z

derive_builder/src/options/struct_mode.rs

+                // We don't allow name-value pairs or further nesting here, so
+                // only look for words.
+                syn::NestedMetaItem::MetaItem(syn::MetaItem::Word(ref tr)) => {
+                    traits.push(tr.clone())


question is, whether we want to do any checks like tr == "Debug" | "Clone" at this place. But I guess it would feel a bit hacky to have the information about Debug and Clone being automatically derived scattered about multiple places. It's probably ok, to not have any checks here.

Note: It's worth looking at how the Rust compiler complains about this

IN

#[derive(Builder)] #[builder(derive(Default))] struct Lorem {

OUT

error[E0119]: conflicting implementations of trait `std::default::Default` for type `LoremBuilder`: --> derive_builder/tests/derive_trait.rs:7:10 | 7 | #[derive(Builder)] | ------- | | | conflicting implementation for `LoremBuilder` | first implementation here

So the error message is ok-ish, but not perfect. I think it would be nicer, if this would either just work without error, or if the builder would panic with a custom message.

I've added a warning at the location in the code review. At the moment it also suppresses forwarding the trait, but I can remove that so we push the warning and then let compilation fail.

* Reword README.md * Filter Default and Clone * Clarify how test works

colin-kiegel · 2017-04-25T08:39:22Z

Hm, there is again a duplicate commit.

This makes it harder to review.

According to git log, the commit 19523fb has a strange parent (e48b24d Colin Kiegel 2017-04-15 clippy). But in github it looks like the parent of 19523fb was 287877a Ted Driggs 2017-04-17 Merge remote-tracking branch 'upstream/master' into build_fn.

I'm no expert with github pull requests, but I would guess that you made a rebase at some point, either directly git rebase or maybe indirectly git pull --rebase? In that case I would prefer if you did rebases in pull requests only if the changes are so radical, that it's cleaner to start from scratch. If you didn't do a rebase, than I don't understand where this redundant comes from.

colin-kiegel

Looks good - going to merge. :-)

Thank you! 🎉

colin-kiegel · 2017-04-25T08:45:01Z

derive_builder/tests/derive_trait.rs

@@ -17,5 +17,7 @@ struct Lorem {

 #[test]
 fn defaults() {
+    // This macro requires that the two sides implement `PartialEq` AND `Debug`,
+    // so this one line is testing that the requested traits were in fact generated.


colin-kiegel · 2017-04-25T08:45:20Z

derive_builder/src/options/struct_mode.rs

+                            format!("The `Default` and `Clone` traits are automatically added to all \
+                            builders; explicitly deriving them is unnecessary ({})", where_diag)); 
+                        },
+                        _ => traits.push(tr.clone())


TedDriggs · 2017-04-25T15:06:39Z

@colin-kiegel you're right, I tried to rebase to clean up the commit history. Thanks for the merge!

Added #[builder(derive(...)]

19523fb

Merge remote-tracking branch 'upstream/master' into derive

cd37230

colin-kiegel reviewed Apr 21, 2017

View reviewed changes

TedDriggs added 3 commits April 24, 2017 10:24

Added #[builder(derive(...)]

e70d8bb

Address feedback

c3ccd38

* Reword README.md * Filter Default and Clone * Clarify how test works

Trying to push

a000df5

Merge branch 'master' into derive

21d81d4

colin-kiegel approved these changes Apr 25, 2017

View reviewed changes

colin-kiegel merged commit 5fe8c53 into colin-kiegel:master Apr 25, 2017

TedDriggs deleted the derive branch April 25, 2017 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added #[builder(derive(...)] for #43 #84

Added #[builder(derive(...)] for #43 #84

TedDriggs commented Apr 17, 2017

colin-kiegel commented Apr 18, 2017

colin-kiegel left a comment

colin-kiegel Apr 21, 2017

colin-kiegel Apr 21, 2017

TedDriggs Apr 24, 2017

colin-kiegel Apr 21, 2017

TedDriggs Apr 21, 2017

colin-kiegel Apr 21, 2017

colin-kiegel Apr 21, 2017

TedDriggs Apr 21, 2017

colin-kiegel Apr 22, 2017

colin-kiegel Apr 21, 2017

colin-kiegel Apr 21, 2017

colin-kiegel Apr 21, 2017

colin-kiegel Apr 21, 2017

TedDriggs Apr 24, 2017

colin-kiegel commented Apr 25, 2017

colin-kiegel left a comment

colin-kiegel Apr 25, 2017

colin-kiegel Apr 25, 2017

TedDriggs commented Apr 25, 2017

Added #[builder(derive(...)] for #43 #84

Added #[builder(derive(...)] for #43 #84

Conversation

TedDriggs commented Apr 17, 2017

colin-kiegel commented Apr 18, 2017

colin-kiegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IN

OUT

Choose a reason for hiding this comment

colin-kiegel commented Apr 25, 2017

colin-kiegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedDriggs commented Apr 25, 2017