Added TryFrom impl generation for target types behind feature flag #77

TedDriggs · 2017-04-12T19:03:00Z

This addresses #76. The generation is automatic but depends on crate feature try_from.

…uilder into try_from

Accidentally had try_from on unconditionally. Now gated on nightlytests crate feature

TedDriggs · 2017-04-13T19:55:54Z

@colin-kiegel The failed test looks to be related to a failure in rustup; don't know what to make of that.

colin-kiegel · 2017-04-13T20:45:01Z

That's rust-lang/rustup#1062 and should be gone with the next nightly release. I can hit restart over the weekend and also have a look at the PR. :-)

colin-kiegel

Thx - good start!

Please merge (or rebase to) current master, because I basically removed the derive_builder_test crate. All integration tests in derive_builder/tests/ will then need this line:

#![cfg_attr(feature = "try_from", feature(try_from))]

colin-kiegel · 2017-04-15T11:10:18Z

derive_builder/Cargo.toml

@@ -23,6 +23,7 @@ proc-macro = true

 [features]
 logging = [ "log", "env_logger", "derive_builder_core/logging" ]
+try_from = []
 struct_default = []
 skeptic_tests = ["skeptic"]
 nightlytests = []


let's change this to nightlytests = ["try_from"] - this will in turn activate it in travis and dev/nightlytests.sh / dev/githook.sh.

colin-kiegel · 2017-04-15T11:24:00Z

derive_builder_core/src/build_method.rs

-            BuilderPattern::Mutable |
-            BuilderPattern::Immutable => quote!(&self),
-        };
+        let self_param = self.pattern.to_build_method_tokens();


I'd like to keep this match statement in BuildMethod for the moment. I feel like from the perspective of unit testing this gives clearer boundaries of responsibility. Of course this makes the BuildMethod type a little more complex..

colin-kiegel · 2017-04-15T11:24:29Z

derive_builder_core/src/options.rs

+        }
+    }
+}
+


please move this back to BuildMethod for the moment.

colin-kiegel · 2017-04-15T11:36:11Z

derive_builder_core/src/try_from.rs

+        let string_ty = self.bindings.string_ty();
+
+        let immutable_borrow = quote!(
+            impl #impl_generics #try_from<&'unused #builder_ty #ty_generics> for #target_ty #ty_generics 


What happens if we elide the lifetime parameter 'unused and write & #builder_ty #ty_generics instead?
Would that fail if #ty_generics contained other lifetime parameters? Did you test that?

If we really need to have an explicit lifetime parameter, I'd suggest to call it 'try_from just in case so the user would get nicer error messages - while still reducing the risk of name collisions.

colin-kiegel · 2017-04-15T11:37:10Z

derive_builder_core/src/try_from.rs

+                #where_clause {
+                type Error = #string_ty;
+
+                fn try_from(v: &#builder_ty #ty_generics) -> #result_ty<Self, Self::Error> {


you don't have a lifetime parameter here, that makes me suspicious that we might not need it. :-)

colin-kiegel · 2017-04-15T11:55:13Z

derive_builder_core/src/try_from.rs

+
+            #mutable_borrow
+
+            #owned            


I'd like to start with only one impl depending on the builder pattern

Variant 1: Mimic build() method

Pseudo code for demonstration:

Owned => /* ... impl TryFrom<FooBuilder> for Foo ... */, Mutable | Immutable => /* ... impl TryFrom<&FooBuilder> for Foo ... */,

So despite having three variants of BuilderPattern there would only be two variants of TryFrom implementations - in analogy to the BuildMethod.

Variant 2: Always take Self

The logic behind changing the signature of the build method was to enable method chaining FooBuilder::default().foo(1).build(). But this would most likely not be the primary use case of TryFrom - although it could be used in this position.

Alternatively we could talk about always implementing TryFrom<FooBuilder> for Foo and never implementing TryFrom<&FooBuilder> for Foo.

Discussion

We should probably discuss the primary use cases for having TryFrom implementations. What signature would they require? Would autoderef or other coercions help to pick the right TryFrom implementation?

colin-kiegel · 2017-04-15T11:56:14Z

derive_builder_test/Cargo.toml

@@ -9,7 +9,7 @@ publish = false
 path = "src/lib.rs"

 [features]
-nightlytests = ["compiletest_rs", "derive_builder/nightlytests"]
+nightlytests = ["compiletest_rs", "derive_builder/nightlytests", "derive_builder/try_from"]


I removed the derive_builder_test crate in latest master. :-)

PS: More correctly - I merged it into the derive_builder crate and left a tiny derive_builder/tests/dummy-crate for doc-inspection.

colin-kiegel · 2017-04-15T11:59:43Z

derive_builder_test/tests/try_from.rs

@@ -0,0 +1,37 @@
+#![cfg(feature = "nightlytests")]
+#![cfg_attr(feature = "nightlytests", feature(try_from))]


please merge (or rebase to) current master. Then depend on feature = "try_from" instead.

TedDriggs · 2017-04-17T21:43:04Z

@colin-kiegel are the run-pass tests not passed the flags from the crate?

colin-kiegel · 2017-04-18T21:16:37Z

@TedDriggs Hm, it would seem so. You could try something like this for the compiletests.

-#![cfg_attr(feature = "nightlytests", feature(try_from))]
+// Compiletests seem to _not_ have access to feature flags. We need to set `#![feature(try_from)]` unconditionally.
+#![allow(unused_features)]
+#![feature(try_from)]

…n current

TedDriggs · 2017-04-19T03:05:39Z

Now it's complaining about the example in readme.md. I'm stumped on how to get this working without a separate feature flag gating the generation of the TryFrom blocks.

includes a workaround for rust-lang/rust#41401

we should not silently implement the `TryFrom` trait if the build function is skipped, because the current `TryFrom` implementation delegates to a build function.

colin-kiegel · 2017-04-21T14:52:35Z

I imagine the final TryFrom implementation like this:

the build-method will only be a shallow wrapper around the TryFrom implementation
- all logic (like custom validation) should be implemented in the TryFrom implementation
- the build method will require the TryFrom implementation - not the other way around, like now
- the build_fn will follow the builder pattern and clone when necessary, before delegating to TryFrom.
- the TryFrom implementation always consumes FooBuilder and avoids cloning. (I believe this would be the most idiomatic implementation)

It's not a problem to start with TryFrom delegating to the build fn (i.e. in the other direction), before we remove the feature gate. However IMO we should already do the following change in this PR:

TryFrom should always consume FooBuilder (i.e. not take references)

What do you think about this?

colin-kiegel · 2017-04-21T14:56:04Z

PS: If you happen to agree to this change than IMO we would be ready to merge. Otherwise we can of course discuss this more deeply. :-)

TedDriggs · 2017-04-21T21:57:34Z

the build-method will only be a shallow wrapper around the TryFrom implementation

My impression after #54 was that logic should live in inherent methods, with the trait impl being the shallow wrapper.

TryFrom should always consume FooBuilder (i.e. not take references)

Without the version that takes a reference, using this in conjunction with #72 gets more verbose. The setter methods are returning &mut Self by default, so not having impl<'a> TryFrom<&'a mut FooBuilder> for Foo means you can't write:

let bar = BarBuilder::default()
    .try_baz("1.2.3.4")?
    .try_foo(FooBuilder::default()
        .lorem("Hello")
        .ipsum("world"))?    
    .build()?;

I'm not opposed to having an owned, non-cloning implementation, as long as Rust is smart enough to avoid an extra clone to push into that implementation if someone writes the code above.

colin-kiegel · 2017-04-22T22:37:01Z

My impression after #54 was that logic should live in inherent methods, with the trait impl being the shallow wrapper.

Hm, but it does not make a difference with regard to inference whether a trait impl delegates to an inherent method or vice-versa. The main issue in #54 was that things like .try_into().try_into().try_into() have inference problems. But inherent methods .foo().bar().baz() do not have this problem, even if each one of them delegates to a trait impl.

The setter methods are returning &mut Self by default

I'm actually thinking about changing that default to Self. I started a discussion #88 about this - but I don't intend to rush anything there, because we already did a flip in the past.

colin-kiegel · 2017-04-24T09:14:32Z

It's already possible to use nested builders even with the mutable pattern. You only need to write .foo(foo_builder.bar(v).build()?) instead of .try_foo(foo_builder.bar(v))?.

I think we can still introduce the TryFrom<&FooBuilder> implementation later. It would be less of a breaking change if we introduce it later rather than remove it later.

Which reminds me that we should raise the minor version with this feature. EDIT: As soon as we remove the feature gate of course. ^^

PS: I'd also welcome more independent opinions on this topic.

TedDriggs added 4 commits April 12, 2017 11:59

Added TryFrom impl generation, tests, and changelog

b87d3bd

Added TryFrom impl generation, tests, and changelog

303b275

Merge branch 'try_from' of https://github.com/TedDriggs/rust-derive-b…

386b46d

…uilder into try_from

Fixed tests on stable and beta

3dd8631

Accidentally had try_from on unconditionally. Now gated on nightlytests crate feature

colin-kiegel requested changes Apr 15, 2017

View reviewed changes

TedDriggs added 4 commits April 17, 2017 09:50

Merge with master

445e555

Removed a file

74b291a

Addressed review feedback

e5b951b

Added try_from to compile tests

165f614

TedDriggs added 2 commits April 18, 2017 19:53

Changed attribute declaration for compiletests to pass CI

7360bae

Merge remote-tracking branch 'upstream/master' into try_from to remai…

c89af56

…n current

colin-kiegel and others added 5 commits April 21, 2017 14:42

fix skeptic tests for try_from feature

e136b7e

fix doc tests

db10f0a

includes a workaround for rust-lang/rust#41401

Merge branch 'master' into try_from

26e6422

fix regression from merge

6027872

fix TryFrom requires build function

4768be4

we should not silently implement the `TryFrom` trait if the build function is skipped, because the current `TryFrom` implementation delegates to a build function.

cleanup

d023758

colin-kiegel mentioned this pull request Apr 22, 2017

default builder pattern #88

Closed

TedDriggs closed this May 30, 2018

TedDriggs deleted the try_from branch February 5, 2019 15:21

TedDriggs mentioned this pull request Feb 28, 2022

Update documentation, TryFrom is stable #230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TryFrom impl generation for target types behind feature flag #77

Added TryFrom impl generation for target types behind feature flag #77

TedDriggs commented Apr 12, 2017

TedDriggs commented Apr 13, 2017

colin-kiegel commented Apr 13, 2017

colin-kiegel left a comment

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

TedDriggs commented Apr 17, 2017

colin-kiegel commented Apr 18, 2017

TedDriggs commented Apr 19, 2017

colin-kiegel commented Apr 21, 2017

colin-kiegel commented Apr 21, 2017

TedDriggs commented Apr 21, 2017

colin-kiegel commented Apr 22, 2017 •

edited

Loading

colin-kiegel commented Apr 24, 2017 •

edited

Loading

		@@ -0,0 +1,37 @@
		#![cfg(feature = "nightlytests")]
		#![cfg_attr(feature = "nightlytests", feature(try_from))]

Added TryFrom impl generation for target types behind feature flag #77

Added TryFrom impl generation for target types behind feature flag #77

Conversation

TedDriggs commented Apr 12, 2017

TedDriggs commented Apr 13, 2017

colin-kiegel commented Apr 13, 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

Variant 1: Mimic build() method

Variant 2: Always take Self

Discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedDriggs commented Apr 17, 2017

colin-kiegel commented Apr 18, 2017

TedDriggs commented Apr 19, 2017

colin-kiegel commented Apr 21, 2017

colin-kiegel commented Apr 21, 2017

TedDriggs commented Apr 21, 2017

colin-kiegel commented Apr 22, 2017 • edited Loading

colin-kiegel commented Apr 24, 2017 • edited Loading

Variant 1: Mimic `build()` method

Variant 2: Always take `Self`

colin-kiegel commented Apr 22, 2017 •

edited

Loading

colin-kiegel commented Apr 24, 2017 •

edited

Loading