Disable derive clone on owned pattern #111

azriel91 · 2018-03-05T23:39:45Z

Attempts to address #97, here's the quick summary:

Made Clone optionally derived on the Builder, depending on the BuilderPattern on the struct as well as on each of the fields. I'm not sure if the way it calculates and passes the "mark this as should derive Clone" is a good way — it was the quickest way to get most of the tests to pass.
Ran current Rustfmt over the repository.
Updated top level docs (minor).
Updated changelog.

The ./dev/checkfeatures.sh run fails, but I couldn't figure out how to make it pass:

getopts 0.2.17 fails to compile:

$ ./dev/checkfeatures.sh

Running `cd derive_builder && rustup run 1.15.0 cargo test --all --color always --features "skeptic_tests"`
   Compiling getopts v0.2.17
error[E0301]: cannot mutably borrow in a pattern guard
   --> /home/azriel/.cargo/registry/src/github.com-1ecc6299db9ec823/getopts-0.2.17/src/lib.rs:405:73
    |
405 |                         } else if was_long || name_pos < names.len() || args.peek().map_or(true, |n| is_arg(&n)) {
    |                                                                         ^^^^ borrowed mutably in pattern guard

error: aborting due to previous error

error: Could not compile `getopts`.

Cargo.lock says compiletest_rs 0.3.7 depends on getopts 0.2.17.

Cargo.toml in derive_builder says:

[dependencies]
# ... omitted
compiletest_rs = { version = "0.3.3", optional = true }

Which, to me says "use version 0.3.3 of compiletest_rs" but cargo is disobeying that. Then I became sad because I tried munging dependencies, but cargo always chose 0.3.7 of compiletest_rs. I did delete Cargo.lock and try again + some other variations. Maybe this issue would solve that: rust-lang/cargo#4100

Re-enactment of how I did this PR

Ripgrep: rg -F owned.
Hack code.
cargo test
rg -F '::Owned'.
Fix compile errors.
Read CONTRIBUTING.md
Do what it says, ish 😀.

Issue colin-kiegel#97

fluffysquirrels · 2018-03-12T11:42:37Z

Thanks for contributing to the crate. I can take a look at this

fluffysquirrels · 2018-03-12T11:59:21Z

Regarding cargo choosing 0.3.3, I believe cargo.lock overrides cargo.toml and you can update cargo.lock with cargo update.

dev/checkfeatures.sh seems to choose v1.15.0, which probably needs updating. Can you test running your changes with latest stable here instead of v1.15.0?

In my other projects I have a parameter or environment variable in scripts to select the toolchain, then call those scripts from Travis. Would you be willing to try this? If not I'll have a go.

azriel91 · 2018-03-12T18:13:17Z

Regarding cargo choosing 0.3.3, I believe cargo.lock overrides cargo.toml and you can update cargo.lock with cargo update.

So turns out it was this niggly point: some_crate = "0.1.2" means some_crate = "^0.1.2" instead of some_crate = "=0.1.2" 🤦‍♂️.

I've locked down getopts to 0.2.15 to maintain compatibility with Rust 1.15, as 0.2.16 and later require Rust 1.18 (see next point).

dev/checkfeatures.sh seems to choose v1.15.0, which probably needs updating. Can you test running your changes with latest stable here instead of v1.15.0?

I think that's intended — this crate maintains compatibility with Rust 1.15, so it must always compile with that as part of testing. I have tested on stable (1.24.1), beta (1.25.0-beta.9), and nightly 2018-03-06, the handy githook does that for free 🆒:

II: Checking clean working directory... ✓ 
II: Running tests on stable... ✓ 
II: Running tests on beta... ✓ 
II: Running tests on nightly... ✓ 
II: Running dev/nightlytests.sh... ✓ 
II: Running dev/checkfeatures.sh... ✓ 
OK: All checks passed!

Pushed!

colin-kiegel · 2018-03-12T18:57:27Z

Yes, tests for 1.15 were intentional, but they don't need to stay forever.

You can bump the minimum Rust version, if you think there is good reason to do so. Stale dependencies can be a very good reason of course. Just keep in mind, that this is a breaking change and implies a "major" release of derive-builder. :-)

But this just reminds me that travis was recently failing and I changed travis.yml in a glorious series of 3 commits ^^. In essence I disabled the offending tests and dev-dependencies for Rust 1.15. However I forgot to fix the dev/-scripts accordingly, because they try to mimic travis.... Yep, it's fragile and quirky as we can see...

Now a quick fix would be to remove the "skeptic_tests" feature in this line. And replace /checkfeatures/checkminimumrust/ in all dev/* file(name)s.

Then you don't need to stick with old dependencies or bump the minimum rust version. Because it's good enough when skeptic tests only run on nightly.

colin-kiegel · 2018-03-12T19:05:05Z

@azriel91 Let me know if you need further advice.

Regarding the PR: As of Jan 3rd @TedDriggs volunteered to lead this crate development together with @Dylan-DPC @fluffysquirrels. We still have to see how this works out. This is like the first test case. ;-)

azriel91 · 2018-03-12T22:20:43Z

Then you don't need to stick with old dependencies or bump the minimum rust version. Because it's good enough when skeptic tests only run on nightly.

Zing!

$ ./dev/githook.sh
II: Checking clean working directory... ✓ 
II: Running tests on stable... ✓ 
II: Running tests on beta... ✓ 
II: Running tests on nightly... ✓ 
II: Running dev/nightlytests.sh... ✓ 
II: Running dev/checkminimumrust.sh... ✓ 
OK: All checks passed!

I also did this, because I like good dev user experience:

$ ./dev/githook.sh
EE: git flag missing: hooks.checkminimumrust
    you can set it like
  $ git config hooks.checkminimumrust true # (or false)

II: `hooks.checkfeatures` has been replaced by `hooks.checkminimumrust`
II: obsolete git flag found: hooks.checkfeatures
    you can remove it like
  $ git config --unset hooks.checkfeatures

EE: Invalid git configuration. Aborting checks.

Dylan-DPC-zz · 2018-03-12T22:29:26Z

Which, to me says "use version 0.3.3 of compiletest_rs" but cargo is disobeying that. Then I became sad because I tried munging dependencies, but cargo always chose 0.3.7 ...

it is better to mention "0.3" in Cargo.toml instead of 0.3.3. 0.3 implies ^0.3 which means it will pick any version 0.3.x and thus we will not end up with such issues.

Dylan-DPC-zz · 2018-03-22T19:15:04Z

@colin-kiegel Kindly review this. I see no issue with it. There are a few caveats (like having a minimum requirement of Rust 1.15 which could mess up some dependencies) but we can solve them as separate issues.

azriel91 · 2018-03-27T22:06:13Z

@TedDriggs Hiya, do you have time to review this in the near future?
Happiness shall ensue if you do 🙃

azriel91 · 2018-04-16T00:55:27Z

Hiya again! I'd like to push for this to be merged / released, because it helps me remove one of the "bad contagion" tech debts before it starts to grow in my code base.

TedDriggs · 2018-05-01T17:35:27Z

@azriel91 reviewing now

TedDriggs · 2018-05-01T17:36:59Z

derive_builder/src/options/mod.rs

-//!    options specified at the struct level. This creates one `OptionsBuilder<FieldMode>` per
-//!    struct field on the input/target type. Once complete, these get converted into
-//!    `FieldOptions` instances.
+//! 1. Builder options on the struct are parsed into


@azriel91 please undo the formatting on comments; this has broken the ordered list rendering.

TedDriggs · 2018-05-01T17:39:49Z

Will merge after fix to doc comments

This aligns the development git hook with the Travis CI build. The skeptic tests are still run on `nightly`, so we don't lose that level of testing.

This also tells them that the flag has been updated to `hooks.checkminimumrust`.

azriel91 · 2018-05-01T20:21:35Z

Sweet thank you! I've pushed the branch minus the Rustfmt commit, but the nightly build fails due to rust-lang/cargo#5364

TedDriggs · 2018-05-01T21:05:47Z

@azriel91 well, that's not very helpful of them. I'll check back in a couple days to see if the nightlies have it fixed; I'm more comfortable merging when there's a passing Travis run.

TedDriggs · 2018-05-17T17:13:26Z

@azriel91 I've finally had some time to come back to this crate, and CI is now fixed on master. I've got a PR in (#120) that updates our version of syn to support new Rust syntax and tries to simplify our options parsing. As soon as that gets resolved, I'm going to loop back around to this and get it updated and merged.

Builders using the owned pattern for all fields don't need Clone derived for them, and therefore can be used with generics that don't support cloning. Fixes #97, and obviates #111

TedDriggs · 2018-05-30T15:48:10Z

derive_builder_core/src/builder.rs

@@ -76,6 +81,11 @@ impl<'a> ToTokens for Builder<'a> {
            let builder_vis = self.visibility;
            let builder_ident = self.ident;
            let derives = self.derives;
+            let clone_derive = if self.pattern.requires_clone() || self.initializer_requires_clone {


Why is this an 'or' here?

It's because the builder needs to derive Clone when using the mutable or immutable patterns;

When the builder pattern is owned, but at least one field overrides the pattern to be mutable/immutable, the builder itself doesn't have to. At the time the PR was made, I ~~was a noob~~ just catered for making the builder not derive Clone when no fields override the pattern and didn't work through the code to make the builder also not derive Clone

Okay, I think my version handles this correctly then; it requires clone if any field requires it, whether via explicit or implicitly getting the mutable or immutable pattern.

Builders using the owned pattern for all fields don't need Clone derived for them, and therefore can be used with generics that don't support cloning. Fixes #97, and obviates #111

Removing approval because of larger crate changes

azriel91 · 2018-06-05T00:37:47Z

Closing in favour of #122

* Only derive Clone on builders when necessary Builders using the owned pattern for all fields don't need Clone derived for them, and therefore can be used with generics that don't support cloning. Fixes #97, and obviates #111 * Improve handling for non-owned builders with zero non-owned fields

Remove Clone derive from builder when using "owned" pattern.

e0b3dcc

Issue colin-kiegel#97

azriel91 mentioned this pull request Mar 5, 2018

Owned builders should not #[derive(Clone)] #97

Closed

azriel91 changed the title ~~work in progress (WIP): Disable derive clone on owned pattern~~ Disable derive clone on owned pattern Mar 9, 2018

colin-kiegel requested a review from TedDriggs March 12, 2018 19:05

Dylan-DPC-zz approved these changes Mar 22, 2018

View reviewed changes

TedDriggs previously approved these changes May 1, 2018

View reviewed changes

azriel91 added 6 commits May 2, 2018 08:09

Updated top level module documentation.

a10f8b7

Updated README.md about no longer deriving Clone for owned builders.

9a62c88

Lock version of getopts to remain compatible with Rust 1.15.

9346cef

Renamed checkfeatures with checkminimumrust

239f187

This aligns the development git hook with the Travis CI build. The skeptic tests are still run on `nightly`, so we don't lose that level of testing.

Inform user if their git config contains hooks.checkfeatures

f15745c

This also tells them that the flag has been updated to `hooks.checkminimumrust`.

Added development tip regarding stale compilation artifacts

c72a3c5

azriel91 force-pushed the issue-97/disable-derive-clone-on-owned-pattern branch from dfc1e69 to c72a3c5 Compare May 1, 2018 20:10

TedDriggs mentioned this pull request May 30, 2018

Only derive Clone on builders when necessary #122

Merged

TedDriggs reviewed May 30, 2018

View reviewed changes

TedDriggs mentioned this pull request May 30, 2018

dev/checkfeatures.sh should not rely on rust v1.15 #123

Closed

azriel91 closed this Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable derive clone on owned pattern #111

Disable derive clone on owned pattern #111

azriel91 commented Mar 5, 2018 •

edited

Loading

fluffysquirrels commented Mar 12, 2018

fluffysquirrels commented Mar 12, 2018

azriel91 commented Mar 12, 2018

colin-kiegel commented Mar 12, 2018

colin-kiegel commented Mar 12, 2018

azriel91 commented Mar 12, 2018

Dylan-DPC-zz commented Mar 12, 2018

Dylan-DPC-zz commented Mar 22, 2018

azriel91 commented Mar 27, 2018

azriel91 commented Apr 16, 2018

TedDriggs commented May 1, 2018

TedDriggs May 1, 2018

azriel91 May 1, 2018

TedDriggs commented May 1, 2018

azriel91 commented May 1, 2018 •

edited

Loading

TedDriggs commented May 1, 2018

TedDriggs commented May 17, 2018

TedDriggs May 30, 2018

azriel91 May 30, 2018

TedDriggs May 30, 2018

azriel91 commented Jun 5, 2018

Disable derive clone on owned pattern #111

Disable derive clone on owned pattern #111

Conversation

azriel91 commented Mar 5, 2018 • edited Loading

fluffysquirrels commented Mar 12, 2018

fluffysquirrels commented Mar 12, 2018

azriel91 commented Mar 12, 2018

colin-kiegel commented Mar 12, 2018

colin-kiegel commented Mar 12, 2018

azriel91 commented Mar 12, 2018

Dylan-DPC-zz commented Mar 12, 2018

Dylan-DPC-zz commented Mar 22, 2018

azriel91 commented Mar 27, 2018

azriel91 commented Apr 16, 2018

TedDriggs commented May 1, 2018

TedDriggs May 1, 2018

Choose a reason for hiding this comment

azriel91 May 1, 2018

Choose a reason for hiding this comment

TedDriggs commented May 1, 2018

azriel91 commented May 1, 2018 • edited Loading

TedDriggs commented May 1, 2018

TedDriggs commented May 17, 2018

TedDriggs May 30, 2018

Choose a reason for hiding this comment

azriel91 May 30, 2018

Choose a reason for hiding this comment

TedDriggs May 30, 2018

Choose a reason for hiding this comment

azriel91 commented Jun 5, 2018

azriel91 commented Mar 5, 2018 •

edited

Loading

azriel91 commented May 1, 2018 •

edited

Loading