Improved type param bound expressions (#91) #92

TedDriggs · 2017-04-25T22:44:38Z

This addresses #91 for the Clone trait using the same principles as the built-in #[derive(Clone)] implementation. It's sitting on top of #85, so a diff against that branch may be more useful for seeing the scope of this change.

@colin-kiegel, I'm not 100% happy with the split between derive_builder and derive_builder_core at the moment. The StructOptions has to pass some of the generics twice or else the generated code won't compile. At the moment that's easy to uphold, but I'm wondering if it would be better to pass additional_bounds to derive_builder_core::Builder and have it assemble the correct generics internally.

I deliberately didn't put anything about this in README.md - I think that the behavior after the PR is more "normal" than the current behavior, so trying to explain it would be confusing.

* Builder impl block is now bounded for when generics implement Clone, rather than requiring that declaration on base type * Updated unit tests in core * Added integration test in derive_builder/tests

TedDriggs · 2017-04-25T22:57:39Z

derive_builder/src/options/struct_mode.rs

@@ -233,7 +230,7 @@ impl From<OptionsBuilder<StructMode>> for (StructOptions, OptionsBuilder<FieldMo
                `field(<vis>)` to the builder attribute on the struct. (Found {})",
                where_diagnostics));
        }
-        
+


My editor keeps insisting on doing this, and I'm not sure why. I'll see if I can find a setting that will make it stop.

colin-kiegel

Nice work. 👍

Regarding your question about passing the impl_generics vs. additional_generics: I think derive_builder_core::Builder should instead know about BuilderPattern and generate the impl_generics in a private helper method.

I also left some comments.

colin-kiegel · 2017-04-26T13:47:49Z

derive_builder_core/src/builder.rs

+        for mut typ in generics.ty_params.iter_mut() {
+            typ.bounds.push(syn::TyParamBound::Trait(
+                syn::PolyTraitRef {
+                    trait_ref: syn::parse_path("::std::clone::Clone").unwrap(),


Can you cache this calculation? Ideally behind an early return (if loop will be empty). :-)

colin-kiegel · 2017-04-26T14:24:56Z

derive_builder/tests/bounds_generation.rs

+
+#[test]
+fn dolor_lorems() {
+    assert_eq!(make_default_lorem(), make_dolor_lorem(Dolor::default()));


Hm, in a test summary dolor_lorems will not be very informative. :-)

Please try to describe the test property in the function name.

If I understand this correctly, it's actually a compile-test in the sense that hey this compiles now, but it didn't before. This test is actually only asserting something (almost) arbitrary in order to show up in the test summary, right?

In that case I would prefer to turn it into something like this

#[test] fn generic_field_without_clone_compiles() { // This is actually a compile test, if we can _name_ `LoremBuilder<Dolor>`, we are already fine. // But we will not be able to call any setters or build method on it, because that would // require the field to implement `Clone`. let _: LoremBuilder<Dolor>; }

I think I'd also like another test - like this:

#[test] fn generic_field_without_clone_has_owned_builder() { assert_eq!(OwnedLoremBuilder::default() .ipsum(Dolor::default()) .build() .unwrap(), OwnedLorem { ipsum: Dolor::default() }); }

Tests have been cleaned up and expanded. As far as your second comment, you got a test that looks exactly like that 😛

colin-kiegel · 2017-04-26T14:30:44Z

derive_builder/tests/bounds_generation.rs

+
+#[test]
+fn u16_lorems() {
+    assert_eq!(make_u16_lorem(10), build_u16_lorem(10));


please use a more informative title (see below). Also since most methods are used only once, I think it's easier to read inlined, no?

E.g.

#[test] fn generic_field_with_clone_has_builder_impl() { assert_eq!(LoremBuilder::default() .ipsum(10) .build() .unwrap(), Lorem { ipsum: 10 }); }

colin-kiegel · 2017-04-26T14:36:05Z

derive_builder/tests/bounds_generation.rs

+#[test]
+fn type_inference() {
+    assert_eq!(LoremBuilder::<String>::default().ipsum("".to_string()).build().unwrap(), 
+               make_default_lorem());


title is fine, but please put the thing you want to test on the left side and try to keep the other side simple. E.g. something like this perhaps?

#[test] fn type_inference() { assert_eq!(make_default_lorem(), Lorem { ipsum: "".to_string() }); }

I prefer a pattern where the left side does all fragile computations and the right side is as robust/straight-forward as possible. If there is an error this will make it easier to trace it down, because you can concentrate on one arm of the equality. :-)

colin-kiegel · 2017-04-26T14:36:31Z

derive_builder/tests/bounds_generation.rs

+/// Notice that this type derives Builder without disallowing
+/// `Lorem<Dolor>`.
+#[derive(Debug, Clone, Builder, PartialEq, Eq)]
+#[builder(field(private), setter(into))]


please remove the attributes you don't need. Both field(private) and setter(into) are not necessary IMO - right?

field(private) was needed to squelch the warnings about public fields. You're right that I don't need setter(into); I'll remove that.

I know, we have deprecations all over the place. :-)

But we also run all tests a second time with feature = "private_fields". So I think it's best not to add #[builder(fields(private))] on every test, but rather live with the deprecations for one cycle.

I ended up keeping it, because I added a test that uses it.

colin-kiegel · 2017-04-26T15:19:08Z

PS: Yeah, I agree that README is only to showcase important stuff. But this sentence is not true anymore:

README.md:99:127 All builders derive Default and Clone, so you should not declare those in this attribute.

In lib.rs:491 this ~~can~~ should be removed: :-)

//! - Generic structs need the boundary where T: std::clone::Clone if
//! used in combination with the immutable/mutable pattern

TedDriggs · 2017-04-26T16:26:46Z

I'm partway through moving this logic over; turns out it also requires the derive_builder_core::Builder struct to know about the bindings. Not a big deal, but wanted to flag it for you.

README.md:99:127 is true; the builder derives Default and Clone, so you shouldn't declare those in the derive(...) attribute or we'll emit a warning. The built-in derives for those types already apply bounds checks like the ones I'm adding for the builder's inherent impl.

I'll remove the line in lib.rs.

* Moved trait bounding logic to derive_builder_core * Cache trait bound for use across all type params * Fixed tests * Removed "gotcha" from lib.rs doc comment

colin-kiegel

@TedDriggs Thank you for this great work!

FYI - I tweaked the changelog message just a little bit. :-)

colin-kiegel · 2017-04-27T08:00:53Z

derive_builder_core/src/builder.rs

+        } else {
+            Default::default()
+        }
+    }


very nice! 👍

I like how Builder::compute_impl_bounds() turned out. :-)

Address colin-kiegel#91

ec81671

* Builder impl block is now bounded for when generics implement Clone, rather than requiring that declaration on base type * Updated unit tests in core * Added integration test in derive_builder/tests

TedDriggs commented Apr 25, 2017

View reviewed changes

TedDriggs changed the title ~~Improved bound expressions for #91~~ Improved type param bound expressions (#91) Apr 26, 2017

colin-kiegel requested changes Apr 26, 2017

View reviewed changes

TedDriggs added 2 commits April 26, 2017 09:52

Feedback updates for colin-kiegel#92

88bf11e

* Moved trait bounding logic to derive_builder_core * Cache trait bound for use across all type params * Fixed tests * Removed "gotcha" from lib.rs doc comment

Merge with master

421a96b

TedDriggs mentioned this pull request Apr 27, 2017

refactoring: re-export custom derive with additional items (WIP) #94

Closed

tweak changelog message

76fd5a5

colin-kiegel approved these changes Apr 27, 2017

View reviewed changes

colin-kiegel merged commit aa7e6e0 into colin-kiegel:master Apr 27, 2017

TedDriggs deleted the generic_bounds branch April 27, 2017 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved type param bound expressions (#91) #92

Improved type param bound expressions (#91) #92

TedDriggs commented Apr 25, 2017

TedDriggs Apr 25, 2017

colin-kiegel left a comment

colin-kiegel Apr 26, 2017

TedDriggs Apr 26, 2017

colin-kiegel Apr 26, 2017

colin-kiegel Apr 26, 2017

TedDriggs Apr 26, 2017

colin-kiegel Apr 26, 2017

colin-kiegel Apr 26, 2017

colin-kiegel Apr 26, 2017

TedDriggs Apr 26, 2017

colin-kiegel Apr 26, 2017

TedDriggs Apr 26, 2017

colin-kiegel commented Apr 26, 2017 •

edited

Loading

TedDriggs commented Apr 26, 2017

colin-kiegel left a comment

colin-kiegel Apr 27, 2017 •

edited

Loading

TedDriggs Apr 27, 2017

Improved type param bound expressions (#91) #92

Improved type param bound expressions (#91) #92

Conversation

TedDriggs commented Apr 25, 2017

Choose a reason for hiding this comment

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

colin-kiegel commented Apr 26, 2017 • edited Loading

TedDriggs commented Apr 26, 2017

colin-kiegel left a comment

Choose a reason for hiding this comment

colin-kiegel Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel commented Apr 26, 2017 •

edited

Loading

colin-kiegel Apr 27, 2017 •

edited

Loading