feat: add completions for `--lockfile-path` by Gmin2 · Pull Request #15238 · rust-lang/cargo

Gmin2 · 2025-02-27T13:11:54Z

What does this PR try to resolve?

Add auto completion for cargo build --lockfile-path <TAB>

Related to #14520

How should we test and review this PR?

rustbot · 2025-02-27T13:11:59Z

rustbot has assigned @epage.
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

epage · 2025-02-27T13:48:44Z

Feel free to squash your commits for how your PR should be merged

epage · 2025-02-27T13:50:10Z

src/cargo/util/command_prelude.rs

+                            None => return false,
+                        };
+
+                        // includes "." and ".." entries as they are required for directory navigation


If these dir part are needed for files, then thats a bug in clap_complete. They shouldn't be needed though since all dirs should always be included, just filtered ones get but last

see #15238 (comment), the directories are already part of the is_dir, removing this check

and also do we need to include hidden folders , i think we need to put a check for it

weihanglo · 2025-02-27T13:48:26Z

src/cargo/util/command_prelude.rs

+                        // includes "." and ".." entries as they are required for directory navigation
+                        if file_name == OsStr::new(".") || file_name == OsStr::new("..") {
+                            return true;
+                        }
+
+                        // allow directories
+                        if path.is_dir() {
+                            return true;
+                        }


Directories are already potential implicit candidates. Wonder why this is needed.

ref: https://github.com/clap-rs/clap/blob/f2cb42a4538cf5fb2a64536464040a7e10987692/clap_complete/src/engine/custom.rs#L325-L334

Thanks for pointing that out, yu are right about the directories part . and .. are already part of is_dir()

weihanglo · 2025-02-27T13:53:32Z

src/cargo/util/command_prelude.rs

+                        };
+
+                        // includes "." and ".." entries as they are required for directory navigation
+                        if file_name == OsStr::new(".") || file_name == OsStr::new("..") {


Do we intentionally make . and .. as candidates to display? I don't think without we cannot do directory navigation. And --manifest-path doesn't have them either.

in my opinion we should include directory navigation(should modify the manifest-path flag also )

As mentioned, this is inconsistent with --manifest-path

As mentioned, this is something that should be handled in clap_complete as a general policy and not one off in each filter

removed from this pr to keep the consistency with --manifest-path and should i open a issue in clap-rs/clap regarding this?

should i open a issue in clap-rs/clap regarding this?

If you can name a concrete problem with a small reproduction case, feel free.

src/cargo/util/command_prelude.rs

epage · 2025-02-28T18:49:16Z

Thanks!

Update cargo 22 commits in 2622e844bc1e2e6123e54e94e4706f7b6195ce3d..ab1463d632528e39daf35f263e10c14cbe590ce8 2025-02-28 12:33:57 +0000 to 2025-03-08 01:45:05 +0000 - test: redact host target when comparing CARGO_ENV path (rust-lang/cargo#15279) - feat: add completions for install --path (rust-lang/cargo#15266) - fix(package): report lockfile / workspace manifest is dirty (rust-lang/cargo#15276) - feat(tree): Add `--depth public` behind `-Zunstable-options` (rust-lang/cargo#15243) - Don't use `$CARGO_BUILD_TARGET` in `cargo metadata` (rust-lang/cargo#15271) - feat: show extra build description from bootstrap (rust-lang/cargo#15269) - Upgrade to `rustc-stable-hash v0.1.2` (rust-lang/cargo#15268) - fix: Respect --frozen everywhere --offline or --locked is accepted (rust-lang/cargo#15263) - feat(tree): Color the output (rust-lang/cargo#15242) - fix(vendor): dont remove non-cached source (rust-lang/cargo#15260) - docs: lockfile is always included since 1.84 (rust-lang/cargo#15257) - Remove `Cargo.toml` from `package.include` in example (rust-lang/cargo#15253) - Small cleanup: remove unneeded result (rust-lang/cargo#15256) - Fix typo in build-scripts.md (rust-lang/cargo#15254) - chore(deps): update rust crate pulldown-cmark to 0.13.0 (rust-lang/cargo#15250) - chore(deps): update compatible (rust-lang/cargo#15249) - feat(cli): forward bash completions of third party subcommands (rust-lang/cargo#15247) - feat: add completions for `--lockfile-path` (rust-lang/cargo#15238) - fix: reset $CARGO if the running program is real `cargo[.exe]` (rust-lang/cargo#15208) - Get all members as `available targets` even though default-members was specified. (rust-lang/cargo#15199) - refactor: control byte display precision with std::fmt options (rust-lang/cargo#15246) - fix(package): Ensure we can package directories ending with '.rs' (rust-lang/cargo#15240)

rustbot assigned epage Feb 27, 2025

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 Feb 27, 2025

epage reviewed Feb 27, 2025

View reviewed changes

weihanglo reviewed Feb 27, 2025

View reviewed changes

Gmin2 force-pushed the lockfile-path-completion branch from ce3ba53 to 30a1eab Compare February 28, 2025 08:39

Gmin2 requested review from epage and weihanglo February 28, 2025 08:42

feat: add completions for --lockfile-path

ad2b1ee

Gmin2 force-pushed the lockfile-path-completion branch from 30a1eab to ad2b1ee Compare February 28, 2025 08:43

epage reviewed Feb 28, 2025

View reviewed changes

src/cargo/util/command_prelude.rs Outdated Show resolved Hide resolved

fix: remove reduntant is_dir() check

4afa6d7

epage approved these changes Feb 28, 2025

View reviewed changes

epage added this pull request to the merge queue Feb 28, 2025

epage mentioned this pull request Feb 28, 2025

Tracking Issue for native-completion #14520

Open

30 tasks

Merged via the queue into rust-lang:master with commit e79bac1 Feb 28, 2025
21 checks passed

weihanglo mentioned this pull request Mar 8, 2025

Update cargo rust-lang/rust#138200

Merged

rustbot added this to the 1.87.0 milestone Mar 10, 2025

Conversation

Gmin2 commented Feb 27, 2025

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented Feb 27, 2025

Uh oh!

epage commented Feb 27, 2025

Uh oh!

epage Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Gmin2 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Gmin2 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gmin2 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

epage Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Gmin2 Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

epage Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epage commented Feb 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Gmin2 Feb 27, 2025 •

edited

Loading

weihanglo Feb 27, 2025 •

edited

Loading