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.

Sign up for GitHub

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

Jump to bottom

refactor: Use clap to suggest alternative argument for unsupported arguments #12529

Merged
merged 4 commits into from
Aug 22, 2023
Merged

refactor: Use clap to suggest alternative argument for unsupported arguments #12529

merged 4 commits into from
Aug 22, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Aug 19, 2023 •
edited
Loading

What does this PR try to resolve?

Part of #11702.

This pull request replaces manual errors with clap native UnknownArgumentValueParser::suggest to give uses alternative arugments, including

  • Suggest --no-fail-fast for --keep-going in cargo test and cargo bench.
  • Suggest cargo-vendor vendor for unsupported cargo tree arguments
    • --no-merge-sources
    • --relative-path
    • --only-git-deps
    • --disallow-duplicates

This PR also renames fn arg_jobs(…) and friends to more reasonable names.

How should we test and review this PR?

cargo t --keep-going
cargo vendor --no-merge-sources
cargo vendor --relative-path
cargo vendor --only-git-deps
cargo vendor --disallow-duplicates

Before

$ cargo vendor --no-merge-sources
error: the crates.io `cargo vendor` command has now been merged into Cargo itself
and does not support the flag `--no-merge-sources` currently; to continue using the flag you
can execute `cargo-vendor vendor ...`, and if you would like to see this flag
supported in Cargo itself please feel free to file an issue at
https://github.com/rust-lang/cargo/issues/new

After

$ cargo vendor --no-merge-sources
error: unexpected argument '--no-merge-sources' found

  tip: the crates.io `cargo vendor` command has been merged into Cargo
  tip: and the flag `--no-merge-sources` isn't supported currently
  tip: to continue using the flag, execute `cargo-vendor vendor ...`
  tip: to suggest this flag supported in Cargo, file an issue at <https://github.com/rust-lang/cargo/issues/new>

Usage: cargo vendor [OPTIONS] [path]

For more information, try '--help'.

@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

r? @epage

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

@weihanglo weihanglo mentioned this pull request Aug 19, 2023
Open
2 tasks
@weihanglo
Copy link
Member Author

This is current blocked on clap-rs/clap#5081.

@epage
Copy link
Contributor

epage commented Aug 21, 2023

This is current blocked on clap-rs/clap#5081.

This can be worked around on the clap side. We could add to the prelude a maybe_flag function that calls try_get_one and filters out the type-discrepancy error.

@weihanglo weihanglo marked this pull request as ready for review August 22, 2023 06:53
@weihanglo
Copy link
Member Author

@epage this is ready for review. Moved some functional tests to UI tests since it makes more sense matching the entire clap output there.

@weihanglo
Copy link
Member Author

Blocked on #12539 bumping the version of cargo.

epage
epage reviewed Aug 22, 2023
View reviewed changes
tests/testsuite/bench.rs Outdated Show resolved Hide resolved
@epage epage changed the title refactor: suggest alternative argument for well-known unknown argument fix: suggest alternative argument for well-known unknown argument Aug 22, 2023
@epage
Copy link
Contributor

epage commented Aug 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit a31aaac has been approved by epage

It is now in the queue for this repository.

@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 Aug 22, 2023
@epage epage changed the title fix: suggest alternative argument for well-known unknown argument refactor: suggest alternative argument for well-known unknown argument Aug 22, 2023
@epage epage changed the title refactor: suggest alternative argument for well-known unknown argument refactor: Use clap to suggest alternative argument for unsupported arguments Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit a31aaac with merge a4c5f11...

bors added a commit that referenced this pull request Aug 22, 2023
@bors
Auto merge of #12529 - weihanglo:clap-suggestions, r=epage
a4c5f11 
refactor: Use clap to suggest alternative argument for unsupported arguments
@bors
Copy link
Contributor

bors commented Aug 22, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2023
@weihanglo
test: move --keep-going suggestion match tests to UI tests
77da514 
We don't want to overly match to suggestion help from clap.
@epage
Copy link
Contributor

epage commented Aug 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit 77da514 has been approved by epage

It is now in the queue for this repository.

@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 Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit 77da514 with merge 50ba4bf...

@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 50ba4bf to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 50ba4bf to master...

@bors
Copy link
Contributor

bors commented Aug 22, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit 50ba4bf into rust-lang:master Aug 22, 2023
@weihanglo weihanglo deleted the clap-suggestions branch August 22, 2023 20:06
@epage epage mentioned this pull request Aug 23, 2023
Closed
10 tasks
@weihanglo weihanglo mentioned this pull request Aug 23, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2023
@bors
Auto merge of rust-lang#115121 - weihanglo:update-cargo, r=weihanglo
e3a0e38 
Update cargo

3 commits in 80eca0e58fb2ff52c1e94fc191b55b37ed73e0e4..2cc50bc0b63ad20da193e002ba11d391af0104b7
2023-08-19 00:52:06 +0000 to 2023-08-22 22:43:08 +0000
- config: merge lists in precedence order (rust-lang/cargo#12515)
- ci: test `resolver-tests` in a separate job (rust-lang/cargo#12540)
- refactor: Use clap to suggest alternative argument for unsupported arguments (rust-lang/cargo#12529)

r? ghost
@ehuss ehuss added this to the 1.74.0 milestone Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@epage epage

Labels
A-cli Area: Command-line interface, option parsing, etc. Command-bench Command-build Command-check Command-doc Command-fix Command-install Command-package Command-publish Command-run Command-rustc Command-rustdoc Command-test Command-vendor S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
5 participants
@weihanglo @rustbot @epage @bors @ehuss