clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
14.02k stars 1.03k forks source link

Intentionally control when a space is appended for native completions #5587

Open epage opened 1 month ago

epage commented 1 month ago

A trailing space signifies that there is nothing else to complete for this argument and the next argument should be started.

Cases where a space is appropriate

Cases where a space is not appropriate

Open questions

Note: we might be able to defer some of these open questions as "parity with stable, static clap_complete" is the minimum bar

shannmu commented 1 month ago

A trailing space design

What do we support now

Cases where we should append the trailing space

Cases where we should not append the trailing space

Cases that depend on the context

values

[!NOTE] What is the context when completing the values of flag At the code level, it refers to the various settings of Arg.

  • num_args
  • value_delimiter
  • trailing_var_arg refers to num_args

At the meaning of value level

  • filepath
  • custom possible values

BTW, when checking the completion for the values of the flag, I found that we might support ignore_case for possible values if the arg is set with ArgSettings::IgnoreCase. This will also provide a great user experience.

Open questions

When there is only one option left for a completion and there is "something more", like --opt completing for --option=value, instead of completing to --option should we offer every --option=value possible?

I think that we should not offer every --option=value possible. For example, if the completion of value comes from clap::ValueHint::FilePath, it may generate too many completions, which makes user confused. It would be better to let the user enter some leading prompt characters.

When the "something more" is optional (using num_args), should we offer with space and without

I think this question was mentioned above.

shannmu commented 1 month ago

Only the cases of the values is relatively complex and are blocked by other issues. The cases for others are relatively certain. We could get benefit from https://github.com/clap-rs/clap/commit/67e31af334346e35a540c49ba2fec46bda5ae880, and add has_space to CompletionCandidate.

shannmu commented 1 month ago

Looking forward to your thoughts. Your feedback is really important!

epage commented 1 month ago

number of values are not matched the number of num_arg

This isn't a question of the number being matched as num_args only applies to 1 2 3 and not 1,2,3.

The way num_args ties into this is when no arguments are possible, like num_args(0..=1) for --color allowing --color to be a shortcut for --color=always.

So this is a distinct case from the others.

epage commented 1 month ago

I think that we should not offer every --option=value possible. For example, if the completion of value comes from clap::ValueHint::FilePath, it may generate too many completions, which makes user confused. It would be better to let the user enter some leading prompt characters.

This is a question of framing.

When we have

Arg::new("option")
    .value_name("value")
    .value_hint(ValueHint::FileType)
    .requires_equals(true)

If the user has foo --opt\t, do we view completion as being only for the current token or do we view completion as for the end result?

@ModProg would be good to get your input on this, if possible.

epage commented 1 month ago

add has_space to CompletionCandidate.

I think I'd prefer for this to be more descriptive, like incomplete. Depending on how we want to handle "maybe incomplete", an Option<bool> or an enum might be needed (where Incomplete is just one variant)

epage commented 1 month ago

Only the cases of the values is relatively complex and are blocked by other issues.

As mentioned in https://github.com/clap-rs/clap/issues/5587#issuecomment-2243557807, #3921 is not a blocker for this. #3922 is a soft-blocker. We could close this out for what is possible now and push responsibility for that case onto #3922 but I think I'd prefer to track this as a cohesive whole so we focus on what the entire user experience around this is.

ModProg commented 1 month ago

One thing to check is verify which shells actually support different behavior here, e.g., fish won't honer us providing the space or not, but uses a list of characters to decide (https://github.com/fish-shell/fish-shell/issues/7832).

The list: https://github.com/fish-shell/fish-shell/blob/ff72080aac0263048b2a9906c34736b40994f6c2/src/reader.rs#L5589 '/', '@', ':', '.', ',', '-' or a '='

In my testing, a trailing slash in the completion was actually escaped: example world\

So e.g. for an option that is numargs=0..=1, at least for the fish shell we should provide at least both --opt and --opt=.

I think that we should not offer every --option=value possible.

We could make this depend on two factors, number of possible values and whether there are other options left.

--opt\t: --optimal, --option, --option=

--optio\t: --option, --option=a, --option=b, --option=c

shannmu commented 1 month ago

I haven't found a way to handle fish's trailing space yet. As for bash, zsh, elvish, and powershell, we can close the trailing space and handle trailing space in the rust native completion logic.

ModProg commented 1 month ago

I think if we just extend CompletionCandidate to have a should_space() or get_with_space() don't care about the naming, the shell specifics shouldn't be an issue.

Though it does mean, that for lists in something like fish, it could make sense to offer completions with trailing commas? Either exclusively or both, producing both should probably also be limited to when there aren't too many completions on screen.

Either --a=1,2,| -> 1 1, 2 2, 3 3, or just 1, 2, 3, (I'm not sure if we support trailing commas in the argument parsing).

epage commented 1 month ago

(I'm not sure if we support trailing commas in the argument parsing).

Looks like we just do a basic split:

https://github.com/clap-rs/clap/blob/5efa52ad4501393d50e236d10a979313a61d4929/clap_builder/src/parser/parser.rs#L1164-L1166

epage commented 1 month ago

See https://github.com/clap-rs/clap/pull/5576#discussion_r1690019695

Should -ci[TAB] complete to -ci<file> or -ci

ModProg commented 1 month ago

Looks like we just do a basic split:\n\nhttps://github.com/clap-rs/clap/blob/5efa52ad4501393d50e236d10a979313a61d4929/clap_builder/src/parser/parser.rs#L1164-L1166

so if we go the route of using trailing , in fish, we'd need to make the parsing ignore a trailing comma.

Though this would technically be a breaking change as I'd expect -ax,y, to produce ["x", "y", ""] currently. But one would need to do -ax,y,, to get the same if we stripped the trailing comma.