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

improve error message for no such subcommand #10924

Merged
merged 16 commits into from
Aug 3, 2022

Conversation

bindsdev
Copy link
Contributor

@bindsdev bindsdev commented Aug 2, 2022

Closes #10900

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2022
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

Thanks!

src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Aug 2, 2022

Based on my different pieces of feedback, here is a rough sketch of how I think this should behave

if cmd.starts_with('+') {
    anyhow::format_err!("no such subcommand: `{}`{}\n\n\tCargo does not handle `+toolchain` directives itself.\n\tDid you mean to run `cargo` through `rustup` instead?", cmd, did_you_mean)
} else {
    let suggestions = list_commands(config);
    let did_you_mean = closest_msg(cmd, suggestions.keys(), |c| c);
    anyhow::format_err!("no such subcommand: `{}`{}\nView all installed commands with `cargo --list`", cmd, did_you_mean)
}

Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

There are a few places where this update is not being checked. It might be good to update a few of the tests from with_stderr_contains to use with_stderr i.e.

        .with_stderr(
            "\
error: no such subcommand: `C`

<tab>Did you mean `c`?

<tab>View all installed commands with `cargo --list`
",
        )

@epage do you have any thoughts on this?

src/bin/cargo/main.rs Outdated Show resolved Hide resolved
src/bin/cargo/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Overall, it looks good.

Probably only thing remaining is ensuring the message for + and suggestions looks good.

@bindsdev
Copy link
Contributor Author

bindsdev commented Aug 3, 2022

Overall, it looks good.

Probably only thing remaining is ensuring the message for + and suggestions looks good.

Thank you. But, I am a bit confused on what you mean by "ensuring the message for +".

@epage
Copy link
Contributor

epage commented Aug 3, 2022

I am a bit confused on what you mean by "ensuring the message for +".

Testing the case for cmd.starts_with('+') where "Cargo does not handle +toolchain directives" shows up

@bindsdev
Copy link
Contributor Author

bindsdev commented Aug 3, 2022

I am a bit confused on what you mean by "ensuring the message for +".

Testing the case for cmd.starts_with('+') where "Cargo does not handle +toolchain directives" shows up

ah ok. Would I write the test for this in the cargo_command.rs file?

@epage
Copy link
Contributor

epage commented Aug 3, 2022

ah ok. Would I write the test for this in the cargo_command.rs file?

Seems like a good place

… message stating that the directive is not handled by Cargo
@epage
Copy link
Contributor

epage commented Aug 3, 2022

@bors r+

Thanks for all your work on this!

@bors
Copy link
Contributor

bors commented Aug 3, 2022

📌 Commit 222d90b 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 3, 2022
@bors
Copy link
Contributor

bors commented Aug 3, 2022

⌛ Testing commit 222d90b with merge 471b80d...

@bindsdev
Copy link
Contributor Author

bindsdev commented Aug 3, 2022

@bors r+

Thanks for all your work on this!

No problem! Looking forward to making more contributions to Rust repositories.

@bors
Copy link
Contributor

bors commented Aug 3, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing 471b80d to master...

@bors bors merged commit 471b80d into rust-lang:master Aug 3, 2022
@bindsdev bindsdev deleted the better-no-such-subcommand branch August 3, 2022 21:12
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 10, 2022
Update cargo

7 commits in 4fd148c47e733770c537efac5220744945d572ef..ce40690a5e4e315d3dab0aae1eae69d0252c52ac
2022-08-03 15:03:52 +0000 to 2022-08-09 22:32:17 +0000
- Make the `rust-version` error recommend `cargo update --precise -p crate@ver` (rust-lang/cargo#10891)
- resolver docs: link to version requirements syntax full explanation (rust-lang/cargo#10946)
- Bump os_info to 3.5.0 (rust-lang/cargo#10943)
- Mark --timings=html unstable in the document (rust-lang/cargo#10941)
- Mention that aliases are recursive (rust-lang/cargo#10935)
- Test if reserved filenames are allowed in Windows (rust-lang/cargo#10322)
- improve error message for `no such subcommand` (rust-lang/cargo#10924)
@ehuss ehuss added this to the 1.65.0 milestone Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error message for no such subcommand
6 participants