Skip to content

feat(add/install): check if given crate argument would be valid with inserted @ symbol #15441

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

Merged
merged 4 commits into from
Apr 28, 2025

Conversation

dawedawe
Copy link
Contributor

Suggest to user to use a crate name with an inserted @ before the first invalid package name character

Fixes #15318

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-add Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2025
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.

Thanks for the patch!

@weihanglo weihanglo self-requested a review April 21, 2025 00:25
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.

Not a blocker. Would you mind following the atomic commit pattern described here. That is,

As part of your atomic commits, prefer adding tests as their own commit before any functionality changes. The tests should pass in each commit, demonstrating the behavior before your change and how each commit affects behavior.

@ehuss ehuss assigned weihanglo and unassigned ehuss Apr 21, 2025
@dawedawe
Copy link
Contributor Author

Not a blocker. Would you mind following the atomic commit pattern described here. That is,

As part of your atomic commits, prefer adding tests as their own commit before any functionality changes. The tests should pass in each commit, demonstrating the behavior before your change and how each commit affects behavior.

Oops, sorry. Should have seen that.
Thanks for the review!

@epage
Copy link
Contributor

epage commented Apr 21, 2025

As a suggestion to clarify what is being asked by the link to the contributor guide, including the tip. The commits could be organized as:

  1. A commit that adds all of the tests, with them passing (or separate commits for cargo add and cargo install)
  2. A commit that changes cargo install, and updates the tests to pass
  3. A commit that changes cargo add, and updates the tests to pass

We're fine with your rewriting your commits during the review process.

@dawedawe dawedawe force-pushed the suggest_crate_name_fix branch 3 times, most recently from c46ba9c to 8c9b373 Compare April 21, 2025 16:31
@dawedawe
Copy link
Contributor Author

Okay, I hope the history is acceptable now.

@epage
Copy link
Contributor

epage commented Apr 21, 2025

Okay, I hope the history is acceptable now.

Thanks, this helps a lot!

@dawedawe dawedawe force-pushed the suggest_crate_name_fix branch 2 times, most recently from 55042a0 to a8010c2 Compare April 26, 2025 21:29
@dawedawe dawedawe force-pushed the suggest_crate_name_fix branch from a8010c2 to 0df8d68 Compare April 26, 2025 23:25
@epage
Copy link
Contributor

epage commented Apr 28, 2025

Thanks!

@epage epage added this pull request to the merge queue Apr 28, 2025
Merged via the queue into rust-lang:master with commit 538c163 Apr 28, 2025
25 checks passed
@dawedawe dawedawe deleted the suggest_crate_name_fix branch April 28, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. Command-add Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo install foo=1.2.3 does not detect invalid character
5 participants