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(completions): Dynamic fish completions #5048

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

ModProg
Copy link
Contributor

@ModProg ModProg commented Jul 28, 2023

Supersedes #4177.

Part of #3917.

@ModProg ModProg mentioned this pull request Jul 28, 2023
@ModProg
Copy link
Contributor Author

ModProg commented Jul 28, 2023

@epage is there any e2e tests for dynamic completions yet? I only found them for the "legacy" completions, bash dynamic completions only asserts the generated completion script AFAICT:

#[cfg(feature = "unstable-dynamic")]
#[test]
fn register_minimal() {
use clap_complete::dynamic::Completer;
let name = "my-app";
let bin = name;
let completer = name;
let mut buf = Vec::new();
clap_complete::dynamic::shells::Bash
.write_registration(name, bin, completer, &mut buf)
.unwrap();
snapbox::Assert::new()
.action_env("SNAPSHOTS")
.matches_path("tests/snapshots/register_minimal.bash", buf);
}

And the tests for dynamic only includes tests without the shells:
https://github.com/clap-rs/clap/blob/master/clap_complete/tests/testsuite/dynamic.rs

I'd extend the utilities in common for dynamic completion testing? And add some basic tests for bash and fish?

@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch 5 times, most recently from aabe822 to 79fab94 Compare July 28, 2023 11:50
@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch 2 times, most recently from a087baa to 6cc45d2 Compare July 28, 2023 12:55
clap_complete/Cargo.toml Outdated Show resolved Hide resolved
@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch from 6cc45d2 to 26055b6 Compare July 28, 2023 15:29
@ModProg
Copy link
Contributor Author

ModProg commented Jul 28, 2023

I joined the example into one file, though some of the workarounds feel a bit hacky.

One thing is, that when the unstable-dynamic feature is removed we'd need to use a different cfg for the example, but that is probably fine.

To not conflict with the non dynamic build, the dynamic binary needs to be compiled in a different folder.

@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch 2 times, most recently from eed4ac5 to 78b079e Compare July 28, 2023 17:38
@ModProg
Copy link
Contributor Author

ModProg commented Jul 28, 2023

I also had to rename test because of it's name conflict.

@ModProg
Copy link
Contributor Author

ModProg commented Jul 28, 2023

@epage
Copy link
Member

epage commented Jul 28, 2023

My PR has been merged

@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch from 78b079e to 2b58b83 Compare July 29, 2023 04:20
@ModProg
Copy link
Contributor Author

ModProg commented Jul 29, 2023

Not sure about the sorting to make bash test work, probably a bit hacky.

@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch 3 times, most recently from 19f1b2b to e3babb0 Compare July 29, 2023 07:07
@ModProg
Copy link
Contributor Author

ModProg commented Jul 30, 2023

  1. I'd like to add help messages for the completions, this could probably be done by having the complete function return pairs of completion and help message.

  2. I'd like to only complete flags when the current argument starts with --. Should I add an argument to complete that enables this? Or should I just enable that by default?

  3. Concerning path completions. I'm not sure what the best way to handle these in a fish compatible way would be. I could enable glob support in our path completions, though that is pretty complex for fish completions:

    • expand globs: */ba would complete to */bash.rs and */banana.rs (given the paths a/bash.rs, a/banana.rs, and b/bash.rs exist)
    • complete substrings (not only prefixes) rc/he/ba would complete to src/shell/bash.rs.

Or we could query the shell to handle this for us, and we'd only do filtering afterwards, i.e. complete can take an IntoIterable<PathBuf> and then filter them accordingly.

@ModProg ModProg requested a review from epage July 30, 2023 16:14
@ModProg
Copy link
Contributor Author

ModProg commented Jul 31, 2023

Is there a place to document degradations?

I.e. ways the dynamic completions worsen user experience compared to static completions.

For fish taht would currently be:

@epage
Copy link
Member

epage commented Jul 31, 2023

Not sure about the sorting to make bash test work, probably a bit hacky.

Can you remove it from this PR? This is focused on fish support and as we've both seen, there seem to be complications with bash testing.

... list of changes...

I'd recommend creating issues for these so we can discuss and resolve them independently

Is there a place to document degradations?

The tracking issue.

@ModProg ModProg force-pushed the fish-dynamic-completions-v3 branch from 2b58b83 to 4f9cf6b Compare July 31, 2023 18:42
Copy link
Member

@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!

@epage
Copy link
Member

epage commented Jul 31, 2023

btw I rewrote the PR description to not close #3917 since it looked like you were treating that as a mini-tracking issue. I leave it to you on whether you actually want to close that (and use the main tracking issue) or keep it open after this is merged

@ModProg
Copy link
Contributor Author

ModProg commented Jul 31, 2023

looked like you were treating that as a mini-tracking issue.

I think it makes sense to collect the shell specific things in the fish issue, though the generic things such as the option filtering or help displaying should probably also be added to the main tracking issue.

@epage
Copy link
Member

epage commented Jul 31, 2023

Thanks for your patience through all of the back and forth in getting this in!

@epage epage merged commit 40650fa into clap-rs:master Jul 31, 2023
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.

2 participants