Skip to content

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 --features <TAB>, cargo run --features <TAB>

It loads all the features that are there in the profile section of Cargo.toml

Screenshot from 2025-03-14 03-36-53

Related to #14520

How should we test and review this PR?

by running cargo build --features <TAB>, cargo run --features <TAB>

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2025
@Gmin2
Copy link
Contributor Author

Gmin2 commented Mar 13, 2025

@epage suggestion will be appreciated on how can i improve this like should i include some description ? or something like that, as i have not discussed with yu earlier and pick directly from the parent issue

Comment on lines 1183 to 1186
Ok(features
.into_iter()
.map(|name| clap_complete::CompletionCandidate::new(name))
.collect())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we sort or de-duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have sorted it in b615137

Copy link
Contributor

Choose a reason for hiding this comment

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

What about de-duplicate?

Copy link
Contributor

Choose a reason for hiding this comment

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

So we'll show a feature multiple times if the name is used in different packages? In some workspaces, that could really bloat the output.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, we can always iterate on this in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we'll show a feature multiple times if the name is used in different packages? In some workspaces, that could really bloat the output.

ahh, we should have a check ig if the package already exists in the completer, that sounds good ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would assume that we put all features into a BTree<String, Vec<String>> that maps feature names to packages. We then generate the candidates from this so you get one feature name no matter how many packages use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should i implement in this pr ? or in the feature iteration ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My assumption would be this PR.

Is there a reason to push it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, we can always iterate on this in a future PR.

i thought you wanted to iterate on future pr

@ehuss ehuss assigned epage and unassigned ehuss Mar 15, 2025
@Gmin2 Gmin2 requested a review from epage March 22, 2025 09:25
@Gmin2 Gmin2 force-pushed the completer-features branch from 9674bef to b615137 Compare March 22, 2025 09:31
@rustbot

This comment has been minimized.

@epage
Copy link
Contributor

epage commented Mar 27, 2025

FYI I'm fine with you cleaning up your commits through the review process. This is small and easy to track the differences between each posting.

@Gmin2 Gmin2 force-pushed the completer-features branch from a0315e2 to 1f18cdf Compare March 29, 2025 05:05
@Gmin2 Gmin2 requested a review from epage March 29, 2025 05:35
@epage
Copy link
Contributor

epage commented Mar 31, 2025

Please clean up the commits for how they should be merged.

@Gmin2 Gmin2 force-pushed the completer-features branch from 1fbc09a to 64b6a59 Compare April 1, 2025 05:31
- Added completion for --features flag
- Collect features from all workspaces
- TODO: To remove a feature multiple times if the name is used in different packages
@Gmin2 Gmin2 force-pushed the completer-features branch from 64b6a59 to c5d4950 Compare April 1, 2025 05:37
@epage
Copy link
Contributor

epage commented Sep 12, 2025

I think I missed that the push happened

@epage epage added this pull request to the merge queue Sep 12, 2025
Merged via the queue into rust-lang:master with commit 00c7c80 Sep 12, 2025
21 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2025
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2025
### What does this PR try to resolve?

Our options are:
- Not offer these completions
- Hide them
- De-prioritize them

Since someone could be using `--package`, I felt de-prioritizing would
give the best experience.

This is a follow up to #15309

### How to test and review this PR?

Along the way, I made things more consistent across completions
bors added a commit to rust-lang/rust that referenced this pull request Sep 17, 2025
Update cargo submodule

19 commits in 24bb93c388fb8c211a37986539f24a819dc669d3..966f94733bbc94ca51ff9f1e4c49ad250ebbdc50
2025-09-10 23:16:07 +0000 to 2025-09-16 17:24:45 +0000
- fix(frontmatter): Improve error quality (rust-lang/cargo#15972)
- fix: wrong variable name in documentation (rust-lang/cargo#15968)
- Add "Optimizing Build Performance" section to the Cargo book (rust-lang/cargo#15924)
- Remove extra apostrophe in environment-variables.md (rust-lang/cargo#15963)
- Clarify warning for using `features` or `default-features` in `patch` (rust-lang/cargo#15953)
- fix(frontmatter): Try alternative len code fences (rust-lang/cargo#15952)
- feat(cli): Allow completions for third-party subcommand names (rust-lang/cargo#15961)
- docs(index): Clarify what we mean by omitting features (rust-lang/cargo#15957)
- fix(future): Report all content as a single Report (rust-lang/cargo#15943)
- fix(complete): Show local crates/features over other members  (rust-lang/cargo#15956)
- docs(resolver): Describe the role of the lockfile  (rust-lang/cargo#15958)
- chore: Skip check-version-bump ci job in forks (rust-lang/cargo#15959)
- Eliminate the last three "did you mean" warning phrasings (rust-lang/cargo#15356)
- fix(info): Suggest a more universal `cargo tree` command (rust-lang/cargo#15954)
- feat(cli): Use ellipses when truncating progress (rust-lang/cargo#15955)
- feat(completer): Added completion for `--features` flag (rust-lang/cargo#15309)
- fix(publish): Switch the 'ctrl-c on wait' line to a help message (rust-lang/cargo#15942)
- docs: move docs building process to contributor guide (rust-lang/cargo#15854)
- fix(manifest): Show error source to users (rust-lang/cargo#15939)

r? ghost
@rustbot rustbot added this to the 1.92.0 milestone Sep 17, 2025
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants