Skip to content

feat(complete): Added completion for --profile #15308

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

Gmin2
Copy link
Contributor

@Gmin2 Gmin2 commented Mar 13, 2025

What does this PR try to resolve?

This attempts to complete the autocompleter for cargo build --profile <TAB>, it loads the built in profiles and the custom profile present in Cargo.toml and .cargo/config.toml if present when pressing TAB key, in the completion value it also adds an description inspired by this

Screenshot from 2025-03-14 02-17-40

Related to #14520

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
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. A-profiles Area: profiles S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2025
}
}

Ok(vec![
Copy link
Member

Choose a reason for hiding this comment

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

This is also profile in configuration file, though we might not want to go that far in completion code I guess?

/// Helper for fetching a profile from config.
fn get_config_profile(ws: &Workspace<'_>, name: &str) -> CargoResult<Option<TomlProfile>> {
let profile: Option<context::Value<TomlProfile>> =
ws.gctx().get(&format!("profile.{}", name))?;

Copy link
Contributor

Choose a reason for hiding this comment

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

In the main flow, I believe we do load [profile] from both config and workspace because we do Profile::new().get_profile_names which looked like the main code path for doing this.

This code path is for when that fails. We could try to make it smarter and give people as much information as can successfully run but that ends up duplicating a lot more of Profiles. My assumption is that it will be fine with the hard coded list.

@Gmin2 Gmin2 requested review from epage and weihanglo March 22, 2025 08:23
@Gmin2 Gmin2 force-pushed the completer-profile branch from ab24dd3 to ed48f5f Compare March 23, 2025 09:26
@epage
Copy link
Contributor

epage commented Mar 24, 2025

Could you clean up the commits for how they should be merged?

@epage
Copy link
Contributor

epage commented Mar 24, 2025

btw the CI failure is being tracked in rust-lang/rust#138863

@Gmin2 Gmin2 force-pushed the completer-profile branch from ed48f5f to b355a81 Compare March 26, 2025 03:33
@Gmin2
Copy link
Contributor Author

Gmin2 commented Mar 26, 2025

Could you clean up the commits for how they should be merged?

Done


fn default_profile_candidates() -> Vec<clap_complete::CompletionCandidate> {
vec![
clap_complete::CompletionCandidate::new("dev".to_string())
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary allocation

Suggested change
clap_complete::CompletionCandidate::new("dev".to_string())
clap_complete::CompletionCandidate::new("dev")

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI it will allocate either way; its just not needed to be explicit (and I prefer limiting to_string() to only when doing conversions)


let mut candidates = Vec::new();
for name in profiles.profile_names() {
if let Ok(profile_instance) = Profiles::new(&ws, name) {
Copy link
Member

Choose a reason for hiding this comment

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

We are recreating Profiles struct here, which doesn't really make sense, as Profiles already has every profile. But for completion, this seems acceptable?

BTW we could use let-else so one less identation level.

Suggested change
if let Ok(profile_instance) = Profiles::new(&ws, name) {
let Ok(profile_instance) = Profiles::new(&ws, name) else {
continue;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW we could use let-else so one less identation level.

done

We are recreating Profiles struct here, which doesn't really make sense, as Profiles already has every profile. But for completion, this seems acceptable?

you are right that recreating the Profiles struct for each profile name is inefficient, the original profiles object should already contain all the information, I kept this approach for consistency with the existing implementation, but if you think it would be better to refactor this to avoid the redundant Profiles creation i would be happy to refactor it

@Gmin2 Gmin2 force-pushed the completer-profile branch from b355a81 to 56a587e Compare March 26, 2025 06:56
 - Added completion for `--profile` in `cargo build`
 - Loads the built in profile and custom profiles present in `Cargo.toml` and `.cargo/config.toml`
@Gmin2 Gmin2 force-pushed the completer-profile branch from 56a587e to feb0e50 Compare March 26, 2025 07:01
@Gmin2 Gmin2 requested a review from weihanglo March 26, 2025 07:04
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.

Thanks for your work on this!

@epage epage added this pull request to the merge queue Mar 26, 2025
Merged via the queue into rust-lang:master with commit 521bd76 Mar 26, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2025
Update cargo

10 commits in 307cbfda3119f06600e43cd38283f4a746fe1f8b..a6c604d1b8a2f2a8ff1f3ba6092f9fda42f4b7e9
2025-03-20 20:00:39 +0000 to 2025-03-26 18:11:00 +0000
- fix(package): update tracking issue for `--message-format` (rust-lang/cargo#15354)
- docs(contrib): Expand the description of team meetings (rust-lang/cargo#15349)
- feat(package): add unstable `--message-format` flag  (rust-lang/cargo#15311)
- feat(complete): Added completion for `--profile` (rust-lang/cargo#15308)
- Uplift windows Cygwin DLL import libraries (rust-lang/cargo#15193)
- do not pass cdylib link args to test (rust-lang/cargo#15317)
- fix: revert the behavior checking lockfile's VCS status (rust-lang/cargo#15341)
- Temporarily ignore cargo_test_doctest_xcompile_ignores (rust-lang/cargo#15348)
- docs: fix typo in the "Shared cache" section (rust-lang/cargo#15346)
- Fix some issues with future-incompat report generation (rust-lang/cargo#15345)

r? ghost
@rustbot rustbot added this to the 1.87.0 milestone Mar 27, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-profiles Area: profiles 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.

4 participants