Rename --prepare-for to --edition, drop arg
#5845
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @killercup |
There was a problem hiding this comment.
Looks good in general, but I'd like to see a few more things. First, we should ensure that you can't pass both flags, and second, if using --edition, we shound print a message like
Upgrading to 2018 edition
so the user knows what they are getting.
I've also left a comment on how I'd write that next_edition method, but it's not required to refactor that now.
| // This'll probably be wrong in 2020, but that's future Cargo's | ||
| // problem. Eventually though we'll just add more editions here as | ||
| // necessary. | ||
| _ => "2018", |
There was a problem hiding this comment.
Hmmmm. Is it actually only future cargo's problem? What if I run cargo fix --edition on an 2018-edition project right now? I'd naively assume this method to deal with that. As it stands right now it sometimes returns a string we can't really work with, and instead assumes we call a function like verify_not_preparing_for_enabled_edition.
Maybe it's overkill, but I'd have this return a Result<&str, CantUpgradeEditionError>, with enum CantUpgradeEditionError { UnknownEdition, LatestKnownEdition }.
There was a problem hiding this comment.
Yeah I was wondering this as well, and it's true that the natural interperetation of this flag on the 2018 edition is to avoid doing anything. Currently though we have a good property where if you mix up the transition steps you'll get warnings and errors to nudge you in the right direction. If we take this approach, though, if you enable the edition to soon you won't get a helpful error but rather a deluge of resolution errors (due to crate::).
For that reason I figured it'd be best to stick to a policy of "--edition can't be used when you're already on the latest edition" and that way we should hopefully not surprise too many people and otherwise continue to help nudge towards the new edition along the prescribed path.
src/cargo/ops/fix.rs
Outdated
| opts.compile_opts.build_config.extra_rustc_env.push((key, "1".to_string())); | ||
| } | ||
|
|
||
| if let Some(edition) = opts.prepare_for { |
There was a problem hiding this comment.
else if let here to only ever use one of edition/prepare_for?
Alternatively, use clap's conflicts_with("prepare_for") on the edition arg
alexcrichton
force-pushed
the
fix-edition
branch
from
f65123b to
7e246be
Compare
|
Updated! |
|
Hm I haven't figured out a great place to print out the "Upgrading..." message, but @killercup did you have a particular location in mind? |
alexcrichton
force-pushed
the
fix-edition
branch
from
7e246be to
ec6d756
Compare
This commit tweaks the UI of `cargo fix` for the edition. Previously you'd execute `cargo fix --prepare-for 2018`, but that's a lot of typing! Plus it's some manual data that Cargo can already infer. Instead, after this commit, you now type `cargo fix --edition`, and that's it! The idea is that this'll tell Cargo to fix code for the *next* edition, inferring whatever edition is in use and figuring out what to pass to rustc. Functionality-wise this should be the exact same as `--prepare-for 2018` though If others agree w/ this change I'll send a PR to the edition guide after this merges!
alexcrichton
force-pushed
the
fix-edition
branch
from
ec6d756 to
b2b120e
Compare
|
@bors: r+ Ok I'm gonna go ahead and try to squeeze this in for the preview, but we can of course continue to iterate in-tree! |
|
📌 Commit b2b120e has been approved by |
Rename `--prepare-for` to `--edition`, drop arg This commit tweaks the UI of `cargo fix` for the edition. Previously you'd execute `cargo fix --prepare-for 2018`, but that's a lot of typing! Plus it's some manual data that Cargo can already infer. Instead, after this commit, you now type `cargo fix --edition`, and that's it! The idea is that this'll tell Cargo to fix code for the *next* edition, inferring whatever edition is in use and figuring out what to pass to rustc. Functionality-wise this should be the exact same as `--prepare-for 2018` though If others agree w/ this change I'll send a PR to the edition guide after this merges!
|
☀️ Test successful - status-appveyor, status-travis |
This commit tweaks the UI of
cargo fixfor the edition. Previously you'dexecute
cargo fix --prepare-for 2018, but that's a lot of typing! Plus it'ssome manual data that Cargo can already infer.
Instead, after this commit, you now type
cargo fix --edition, and that's it!The idea is that this'll tell Cargo to fix code for the next edition,
inferring whatever edition is in use and figuring out what to pass to rustc.
Functionality-wise this should be the exact same as
--prepare-for 2018thoughIf others agree w/ this change I'll send a PR to the edition guide after this
merges!