-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Display builtin aliases with cargo --list
#8542
Conversation
Update fork
As stated in rust-lang#8486 it would help to the discovery of the builtin aliases the facto of printing them with the `cargo --list` command. - Extracted the builtin aliases currently implemented to a sepparated `const`. - Make all of the functions that interact with these aliases point to that function. - Refactored the `list_commands` fn in order to include with the builtin and external commands, the builtin aliases that come with cargo by defaut.
Added a test that checks that the aliases that currently are builtin with cargo are indeed being printed with the rest of the commands when `cargo --list` is called. Closes rust-lang#8486
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
📌 Commit 7b16c7c has been approved by |
src/bin/cargo/main.rs
Outdated
/// corresponding execs represented as &str. | ||
fn builtin_aliases_execs(cmd: &str) -> Option<&str> { | ||
match cmd { | ||
"b" => Some("build"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of defining the set of aliases twice, perhaps just place them all in a single table? Like a 3-element tuple (alias, command, description)
, and then builtin_aliases_execs
could just iterate over the array and find a match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's brilliant!! Will modify it!!
As @ehuss correctly suggested, we could just declare in one `const` structure for every builtin alias the following: `(alias, aliased_command, description)`. Therefore, the suggestion has been applied and the `BUILTIN_ALIASES` const has been refactored. Also, the `builtin_aliases_execs` now parses the `BUILTIN_ALIASES` const searching for a "possible alias command" returning an option with the previous info structure or `None`.
I think this should do it. I'm considering to open a followup issue in respect to that and move all of the "aliased" stuff to a separate file so it's easier to upgrade on the future. Suggestions are welcomed @alexcrichton @ehuss |
Thanks! 😄 @bors r+ |
📌 Commit 2ae8df6 has been approved by |
☀️ Test successful - checks-actions |
Update cargo 14 commits in aa6872140ab0fa10f641ab0b981d5330d419e927..974eb438da8ced6e3becda2bbf63d9b643eacdeb 2020-07-23 13:46:27 +0000 to 2020-07-29 16:15:05 +0000 - Fix O0 build scripts by default without `[profile.release]` (rust-lang/cargo#8560) - Emphasize git dependency version locking behavior. (rust-lang/cargo#8561) - Update lock file encodings on changes (rust-lang/cargo#8554) - Fix sporadic lto test failures. (rust-lang/cargo#8559) - build-std: Fix libraries paths following upstream (rust-lang/cargo#8558) - Flag git http errors as maybe spurious (rust-lang/cargo#8553) - Display builtin aliases with `cargo --list` (rust-lang/cargo#8542) - Check manifest for requiring nonexistent features (rust-lang/cargo#7950) - Clarify test name filter usage (rust-lang/cargo#8552) - Revert Cargo Book changes for default edition (rust-lang/cargo#8551) - Prepare for not defaulting to master branch for git deps (rust-lang/cargo#8522) - Include `+` for crates.io feature requirements in the Cargo Book section on features (rust-lang/cargo#8547) - Update termcolor and fwdansi versions (rust-lang/cargo#8540) - Cargo book nitpick in Manifest section (rust-lang/cargo#8543)
@CPerezz I just noticed, I had intended for this to also include user aliases. Would you be willing to do a follow-up PR to add those, too? |
Reopened #8486, let me know if you have any questions on it. |
As stated in #8486 it would help to the discovery of the
builtin aliases the facto of printing them with the
cargo --list
command.separated
const
.point to that function.
list_commands
fn in order to include with thebuiltin and external commands, the builtin aliases that come with
cargo by default.
are builtin with cargo are indeed being printed with the rest
of the commands when
cargo --list
is called.The output on my machine looks like this:
As discussed with @ehuss the
BTreeSet
enforcesOrd
therefore, the aliases get mixed with the commands since they're passed through the same function.It can be refactored to appear separately, but, the code will be more spread and now it's all in just one file (which I believe is easier to maintain and review).
Closes #8486