Add --lib and status message for completion #2921

Merged
merged 2 commits into from Jul 30, 2016

Conversation

Projects
None yet
9 participants
@jonathandturner
Contributor

jonathandturner commented Jul 26, 2016

This PR adds a --lib flag for creating a library with new/init. It also adds a status message for a successful completion.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 26, 2016

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jul 26, 2016

Member

Hm, when we made cargo, we assumed the opposite. What's the basis here?

On Jul 26, 2016, 15:02 -0700, Rust highfive robot notifications@github.com, wrote:

r? @alexcrichton (https://github.com/alexcrichton)

(rust_highfive has picked a reviewer for you, use r? to override)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub (#2921 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AABsioh44d3w_CYU7KUZbRZ6DMx5kkVJks5qZoPfgaJpZM4JVo20).

Member

steveklabnik commented Jul 26, 2016

Hm, when we made cargo, we assumed the opposite. What's the basis here?

On Jul 26, 2016, 15:02 -0700, Rust highfive robot notifications@github.com, wrote:

r? @alexcrichton (https://github.com/alexcrichton)

(rust_highfive has picked a reviewer for you, use r? to override)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub (#2921 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AABsioh44d3w_CYU7KUZbRZ6DMx5kkVJks5qZoPfgaJpZM4JVo20).

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jul 26, 2016

Member

Also, while Cargo is technically nightly, we've considered the CLI pretty
stable, this is a pretty big change in behavior.

On Tue, Jul 26, 2016 at 6:29 PM, Steve Klabnik steve@steveklabnik.com
wrote:

Hm, when we made cargo, we assumed the opposite. What's the basis here?

On Jul 26, 2016, 15:02 -0700, Rust highfive robot <
notifications@github.com>, wrote:

r? @alexcrichton https://github.com/alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2921 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABsioh44d3w_CYU7KUZbRZ6DMx5kkVJks5qZoPfgaJpZM4JVo20
.

Member

steveklabnik commented Jul 26, 2016

Also, while Cargo is technically nightly, we've considered the CLI pretty
stable, this is a pretty big change in behavior.

On Tue, Jul 26, 2016 at 6:29 PM, Steve Klabnik steve@steveklabnik.com
wrote:

Hm, when we made cargo, we assumed the opposite. What's the basis here?

On Jul 26, 2016, 15:02 -0700, Rust highfive robot <
notifications@github.com>, wrote:

r? @alexcrichton https://github.com/alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2921 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABsioh44d3w_CYU7KUZbRZ6DMx5kkVJks5qZoPfgaJpZM4JVo20
.

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 26, 2016

Contributor

@steveklabnik - Cargo is at "0.13.0-nightly (dc12479 2016-07-22)", so we're still pre-1.0. I agree that we don't want to go breaking things willy-nilly...

It looks like the hunch that most people would make libraries doesn't seem to play out from the results we're seeing. The 2016 survey puts "contributes to crates.io" as a minority of our users (roughly 550 of the 2000 active Rust users in the survey).

There's another intuition that if you're trying to make a library, you already have a bit of Rust experience under your belt. You know how to look up documentation and how to set up additional settings. I'd argue even into your first few days as a Rust developer you may not have created a library yet, but likely would have created a few small apps.

If the minority of users create libraries, and we're making users go down less ergonomic paths because of it, this seems like a good reason to change the default.

Contributor

jonathandturner commented Jul 26, 2016

@steveklabnik - Cargo is at "0.13.0-nightly (dc12479 2016-07-22)", so we're still pre-1.0. I agree that we don't want to go breaking things willy-nilly...

It looks like the hunch that most people would make libraries doesn't seem to play out from the results we're seeing. The 2016 survey puts "contributes to crates.io" as a minority of our users (roughly 550 of the 2000 active Rust users in the survey).

There's another intuition that if you're trying to make a library, you already have a bit of Rust experience under your belt. You know how to look up documentation and how to set up additional settings. I'd argue even into your first few days as a Rust developer you may not have created a library yet, but likely would have created a few small apps.

If the minority of users create libraries, and we're making users go down less ergonomic paths because of it, this seems like a good reason to change the default.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 27, 2016

Member

One possibility for sequencing this as well is to add the --lib flag, wait for it to hit stable, then switch the defaults. (if we're worried about breakage).

I agree with @steveklabnik that despite cargo being "pre 1.0" it's basically defacto stable in many respects. I'm not too worried about breakage here, but it may be best to head it off by moving slowly.

I don't personally recall the exact reasons why we chose libraries as the default, but I'm sympathetic to the data just being the other way around so don't mind changing on that basis.

Member

alexcrichton commented Jul 27, 2016

One possibility for sequencing this as well is to add the --lib flag, wait for it to hit stable, then switch the defaults. (if we're worried about breakage).

I agree with @steveklabnik that despite cargo being "pre 1.0" it's basically defacto stable in many respects. I'm not too worried about breakage here, but it may be best to head it off by moving slowly.

I don't personally recall the exact reasons why we chose libraries as the default, but I'm sympathetic to the data just being the other way around so don't mind changing on that basis.

src/bin/new.rs
+ }
+ else {
+ flag_bin
+ };

This comment has been minimized.

@alexcrichton

alexcrichton Jul 27, 2016

Member

Here we should just pass through flag_bin, right?

@alexcrichton

alexcrichton Jul 27, 2016

Member

Here we should just pass through flag_bin, right?

This comment has been minimized.

@alexcrichton

alexcrichton Jul 27, 2016

Member

Oh I see, yeah let's just pass through these flags as-is and have the "default logic" kick in during the implementation below

@alexcrichton

alexcrichton Jul 27, 2016

Member

Oh I see, yeah let's just pass through these flags as-is and have the "default logic" kick in during the implementation below

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 27, 2016

Member

cc @rust-lang/tools, thoughts about changing the default?

Member

alexcrichton commented Jul 27, 2016

cc @rust-lang/tools, thoughts about changing the default?

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Jul 27, 2016

Member

+1 on adding --lib, +0.5 on changing the default - It annoys me personally, I nearly always want --bin, but Cargo is de facto pretty stable, so we should do this gently rather than flicking the switch. In addition to Alex's suggestion, could we warn when doing cargo new with neither --bin nor --lib?

Member

nrc commented Jul 27, 2016

+1 on adding --lib, +0.5 on changing the default - It annoys me personally, I nearly always want --bin, but Cargo is de facto pretty stable, so we should do this gently rather than flicking the switch. In addition to Alex's suggestion, could we warn when doing cargo new with neither --bin nor --lib?

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 27, 2016

Contributor

I think I agree that --bin is a better default purely for the new user experience, regardless of what people do most often, and also that switching it without warning would be undesirable. I also know that I just never remember which is the default and am sometimes surprised that I have a library after creating a cargo project (particularly for the numerous one-off experiments that just get thrown away).

I'm unsure the best way to proceed, or if the breakage is worth it.

Another way we could transition is by temporarily changing cargo new to produce a crate that makes both a lib and a bin and in the lib.rs source write a comment explaining what's going on.

Contributor

brson commented Jul 27, 2016

I think I agree that --bin is a better default purely for the new user experience, regardless of what people do most often, and also that switching it without warning would be undesirable. I also know that I just never remember which is the default and am sometimes surprised that I have a library after creating a cargo project (particularly for the numerous one-off experiments that just get thrown away).

I'm unsure the best way to proceed, or if the breakage is worth it.

Another way we could transition is by temporarily changing cargo new to produce a crate that makes both a lib and a bin and in the lib.rs source write a comment explaining what's going on.

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 28, 2016

Contributor

So it sounds like we're leaning towards making it the default but with caution.

Maybe we can switch the default but also just add a general result message for any new/init? Something like:

> cargo new foobar
  Created binary (application) `foobar` template
Contributor

jonathandturner commented Jul 28, 2016

So it sounds like we're leaning towards making it the default but with caution.

Maybe we can switch the default but also just add a general result message for any new/init? Something like:

> cargo new foobar
  Created binary (application) `foobar` template
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 28, 2016

Member

@nrc

could we warn when doing cargo new with neither --bin nor --lib?

I think the motivation for this PR from @jonathandturner was to simplify the newbie experience with just cargo new foo being much simpler than cargo new foo --bin in terms of aesthetics.

Another way we could transition is by temporarily changing cargo new to produce a crate that makes both a lib and a bin and in the lib.rs source write a comment explaining what's going on.

This... seems like a great idea! @jonathandturner what do you think of this?

The idea of a status message also seems fine to me, it's arguably just missing today.

Member

alexcrichton commented Jul 28, 2016

@nrc

could we warn when doing cargo new with neither --bin nor --lib?

I think the motivation for this PR from @jonathandturner was to simplify the newbie experience with just cargo new foo being much simpler than cargo new foo --bin in terms of aesthetics.

Another way we could transition is by temporarily changing cargo new to produce a crate that makes both a lib and a bin and in the lib.rs source write a comment explaining what's going on.

This... seems like a great idea! @jonathandturner what do you think of this?

The idea of a status message also seems fine to me, it's arguably just missing today.

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 28, 2016

Contributor

I lean towards the status message.

Putting both in works on one level, since it doesn't have a preference. But what I'm trying to fix is that there's a happy path for users, especially new users. If a new user types "cargo new foo", they'll end up getting more than they have today that they need to read and understand. I'd rather have a simple starter template.

Contributor

jonathandturner commented Jul 28, 2016

I lean towards the status message.

Putting both in works on one level, since it doesn't have a preference. But what I'm trying to fix is that there's a happy path for users, especially new users. If a new user types "cargo new foo", they'll end up getting more than they have today that they need to read and understand. I'd rather have a simple starter template.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jul 28, 2016

Contributor

I'm comfortable with adding --lib now and printing a warning (for now) when people use bare cargo new (similar to the warnings about git changing the behavior of git push). After some time, we can change the behavior to default to --bin and remove the warning.

Contributor

wycats commented Jul 28, 2016

I'm comfortable with adding --lib now and printing a warning (for now) when people use bare cargo new (similar to the warnings about git changing the behavior of git push). After some time, we can change the behavior to default to --bin and remove the warning.

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 28, 2016

Contributor

Switched to using @wycats/@nrc's suggestion, so I now warn if you do a cargo new without a target.

Default is back to --lib.

I also added a status message for successful completion of a new/init.

Contributor

jonathandturner commented Jul 28, 2016

Switched to using @wycats/@nrc's suggestion, so I now warn if you do a cargo new without a target.

Default is back to --lib.

I also added a status message for successful completion of a new/init.

@jonathandturner jonathandturner changed the title from Add --lib and default new/init to --bin (apps) to Add --lib and status message for completion Jul 29, 2016

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 29, 2016

Contributor

Started a thread here: https://internals.rust-lang.org/t/should-cargo-new-default-to-applications/3772 to see if we should change the default. For the time being, leaving the PR as a simple "add --lib and add a status message".

Contributor

jonathandturner commented Jul 29, 2016

Started a thread here: https://internals.rust-lang.org/t/should-cargo-new-default-to-applications/3772 to see if we should change the default. For the time being, leaving the PR as a simple "add --lib and add a status message".

src/bin/init.rs
+
+ try!(config.shell().status("Created", format!("{} project",
+ if is_lib { "library" }
+ else {"binary (application)"})));

This comment has been minimized.

@alexcrichton

alexcrichton Jul 29, 2016

Member

Can this be pushed into ops to deduplicate betwen here and new? (same with the if block above

@alexcrichton

alexcrichton Jul 29, 2016

Member

Can this be pushed into ops to deduplicate betwen here and new? (same with the if block above

This comment has been minimized.

@jonathandturner

jonathandturner Jul 29, 2016

Contributor

The messages are different between init and new, so I'm not sure how much that would buy us.

@jonathandturner

jonathandturner Jul 29, 2016

Contributor

The messages are different between init and new, so I'm not sure how much that would buy us.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 29, 2016

Member

Oh ok, in that case maybe it could be pushed down into the init and new functions? (would be good to deduplicate the if above in any case)

@alexcrichton

alexcrichton Jul 29, 2016

Member

Oh ok, in that case maybe it could be pushed down into the init and new functions? (would be good to deduplicate the if above in any case)

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Jul 29, 2016

Contributor

@alexcrichton Fixed and squished. Defaulting now happens in a constructor that's shared by both functions.

Contributor

jonathandturner commented Jul 29, 2016

@alexcrichton Fixed and squished. Defaulting now happens in a constructor that's shared by both functions.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 29, 2016

Member

Ah right one last thing is could you add a test where both --lib and --bin are passed? Other than that lgtm

Member

alexcrichton commented Jul 29, 2016

Ah right one last thing is could you add a test where both --lib and --bin are passed? Other than that lgtm

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
Contributor

jonathandturner commented Jul 29, 2016

@alexcrichton - done.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Jul 29, 2016

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 29, 2016

Contributor

⌛️ Testing commit a0a7752 with merge f1bbc08...

Contributor

bors commented Jul 29, 2016

⌛️ Testing commit a0a7752 with merge f1bbc08...

bors added a commit that referenced this pull request Jul 29, 2016

Auto merge of #2921 - jonathandturner:bin_by_default, r=alexcrichton
Add --lib and status message for completion

This PR adds a --lib flag for creating a library with new/init.  It also adds a status message for a successful completion.
@bors

This comment has been minimized.

Show comment
Hide comment
Contributor

bors commented Jul 30, 2016

@bors bors merged commit a0a7752 into rust-lang:master Jul 30, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@sanxiyn sanxiyn referenced this pull request in rust-lang/book Aug 16, 2016

Closed

Synchronize with Cargo output changes #195

@SoniEx2

This comment has been minimized.

Show comment
Hide comment
@SoniEx2

SoniEx2 Oct 4, 2016

Why can't specify both bin and lib? You /can/ have both lib.rs and main.rs... (Should I be opening a new issue instead of hijacking a merged PR?)

SoniEx2 commented Oct 4, 2016

Why can't specify both bin and lib? You /can/ have both lib.rs and main.rs... (Should I be opening a new issue instead of hijacking a merged PR?)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 4, 2016

Member

@SoniEx2 ah yeah in general you should be able to combine any number of --bin, --example, --lib, etc flags. Right now the option parser doesn't quite handle all the cases but it's indeed an issue that should be tracked on this repo most likely

Member

alexcrichton commented Oct 4, 2016

@SoniEx2 ah yeah in general you should be able to combine any number of --bin, --example, --lib, etc flags. Right now the option parser doesn't quite handle all the cases but it's indeed an issue that should be tracked on this repo most likely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment