Skip to content
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

feat(clap_complete): Dynamic Completion: Support hiding possible values, flags, and subcommands by default #5283

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

siketyan
Copy link

@siketyan siketyan commented Jan 4, 2024

Closes #3951

For example, --output is hidden on listing all possible flags...

image

And it is visible when I type --o.

image

Signed-off-by: Natsuki Ikeguchi <me@s6n.jp>
Signed-off-by: Natsuki Ikeguchi <me@s6n.jp>
Signed-off-by: Natsuki Ikeguchi <me@s6n.jp>
@epage
Copy link
Member

epage commented Jan 4, 2024

Thanks for the work on this!

There is one point I wasn't clear about in the expectations for this: we should probably keep a completion hidden unless there are no visible choices.

In reviewing this, I also noticed

  • I didn't mention hidden aliases
  • We aren't showing visible subcommand aliases

I do find it less than ideal how inconsistent we are with where we check is_hide_set. However, I assume cleaning that up would naturally fall out of the above as we'd need to start returning (value, is_hidden, help) (maybe turned into a struct? if so, ideally that refactor would be its own commit) so we can handle all of these cases.

With that said, this is strictly an improvement from where we are at and I'm willing to merge as-is. I would remove the "Closes" so we'd keep that issue open for the rest of this work.

Would you prefer me merging this or wrapping up the above?

If you do go, I would recommend putting the test fixture changes in their own commits, showing the behavior at the time of that commit, with the feature work done in a follow up commit. This makes it a lot easier to review the tests / behavior change as I can more easily see what behavior changed in the feature commits via the test changes.

@siketyan
Copy link
Author

siketyan commented Jan 4, 2024

@epage Thank you for your review! I prefer continuing this before merging.

If you do go, I would recommend putting the test fixture changes in their own commits, showing the behavior at the time of that commit, with the feature work done in a follow up commit.

Makes sense. I'll do that!

@epage
Copy link
Member

epage commented Jan 4, 2024

I'm curious, what led you to working on this?

@siketyan
Copy link
Author

siketyan commented Jan 5, 2024

@epage Actually I was trying to generate shell completion from clap with dynamic listing, as explained in #1232. Then I found the tracking issue #3166 and found what I can do for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support hiding flags/subcommands/possible values for native compeletions
2 participants