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

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits2 Checks21 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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?

r? @epage

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

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

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

None => return false,
};

// includes "." and ".." entries as they are required for directory navigation

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Comment on lines 356 to 359

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

// allow directories
if path.is_dir() {
return true;
}

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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

};

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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