Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(yank): Use '--version' like install #10575

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Apr 18, 2022

What does this PR try to resolve?

During the design conversations on cargo-add, we noticed:

  • cargo-install has a public flag --version and an invisible alias --vers
  • cargo-yank has a public flag --vers

This switches cargo-yank to publicly use --version and have an invisible alias
--vers, making it consistent with cargo-install.

How should we test and review this PR?

This updated all tests to use the "recommended" flag.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2022
During the design conversations on cargo-add, we noticed that
`cargo-install` has a public flag `--version` and an invisible alias
`--vers` while `cargo-yank` has a public flag `--vers`.  This switches
`cargo-yank` to publicly use `--version` and have an invisible alias
`--vers`, making them consistent.

Completions are a best guess.
@epage epage force-pushed the version branch 2 times, most recently from f0998dc to 2313edd Compare April 20, 2022 14:54
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable!

For completion, it makes sense that --vers is not there, since it's not recommended any more. For doc, it should be there for people to lookup. I believe cargo will continue supporting --vers for a long time. Does these answer your uncertainties?

@epage
Copy link
Contributor Author

epage commented Apr 23, 2022

For completion, it makes sense that --vers is not there, since it's not recommended any more. For doc, it should be there for people to lookup. I believe cargo will continue supporting --vers for a long time. Does these answer your uncertainties?

I actually don't remember why I listed those areas of uncertainty as I had just copied what cargo install did :)

@weihanglo

This comment was marked as duplicate.

@weihanglo weihanglo added the T-cargo Team: Cargo label Apr 23, 2022
@weihanglo
Copy link
Member

@rfcbot merge

It's a user-facing change, so I'd like to get feedback from the team.

Generally it seems no harm and more consistent.

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2022

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 23, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@joshtriplett
Copy link
Member

Seems reasonable. We already have other commands accepting --version, and only the top-level cargo --version has the traditional meaning of "print the version of the program".

@weihanglo
Copy link
Member

Thanks everyone!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2022

📌 Commit 2313edd has been approved by weihanglo

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@bors
Copy link
Contributor

bors commented Apr 24, 2022

⌛ Testing commit 2313edd with merge f3a4448...

@bors
Copy link
Contributor

bors commented Apr 25, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing f3a4448 to master...

@bors bors merged commit f3a4448 into rust-lang:master Apr 25, 2022
@epage epage deleted the version branch April 25, 2022 13:40
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2022
Update cargo

8 commits in edffc4ada3d77799e5a04eeafd9b2f843d29fc23..f63f23ff1f1a12ede8585bbd1bbf0c536e50293d
2022-04-19 17:38:29 +0000 to 2022-04-28 03:15:50 +0000
- move workspace inheritance untable docs to the correct place (rust-lang/cargo#10609)
- Cargo add support for workspace inheritance (rust-lang/cargo#10606)
- chore: Upgrade toml_edit (rust-lang/cargo#10603)
- Mark .cargo/git and .cargo/registry as cache dirs (rust-lang/cargo#10553)
- fix(yank): Use '--version' like install (rust-lang/cargo#10575)
- Disallow setting registry tokens with --config (rust-lang/cargo#10580)
- Set cargo --version git hash length to 9 (rust-lang/cargo#10579)
- Prefer `key.workspace = true` to `key = { workspace = true }` (rust-lang/cargo#10584)
@ehuss ehuss added this to the 1.62.0 milestone Apr 29, 2022
@rfcbot rfcbot added to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants