Add hinting of arg value types for zsh/fish completion #1793

intgr · 2020-04-07T23:42:57Z

Adds new method/attribute Arg::value_hint, taking a ValueHint enum
as argument. The hint can denote accepted values, for example: paths,
usernames, hostnames, commands, etc.

This initial implementation supports hints for the zsh and fish
completion generators, support for other shells can be added later.

intgr · 2020-04-07T23:44:22Z

Is still missing some functionality and needs some polish, but I wanted to open a PR to get some feedback first.

pksunkara · 2020-04-08T06:25:12Z

Looks like a great start. Appreciate this @intgr

Btw, could you help us test if #850 has been fixed or not? I couldn't get it working on my local zsh

pickfire · 2020-04-08T13:44:34Z

Is it possible to extract the type of data to do value hinting automatically in addition to this, like making use of rust types like File will get a file? I think this should be harder but helps reduces user effort, of course the limitation here it that we can't figure out if we we want a file or a host from rust type itself.

clap_generate/src/generators/shells/zsh.rs

intgr · 2020-04-08T14:37:55Z

Is it possible to extract the type of data to do value hinting automatically

Good idea, I will look into it.

intgr · 2020-04-08T12:16:36Z

src/build/arg/mod.rs

+    ///     .value_hint(ValueHint::CommandWithArguments)
+    /// # ;
+    /// ```
+    pub fn value_hint(mut self, value_hint: ValueHint) -> Self {


Looks like by convention, I should also use self.setb(ArgSettings::TakesValue); here?

Also, can there only be one value hint, can't it be a merged value hint or a list?

Hmm, can you imagine any use cases for combining multiple hints for one argument?

Seems it would be quite an edge case. For situations like that, I think it would be better to provide some "escape hatch" to manually override parts of the generated completion file, rather than trying to accommodate such weirdness.

src/build/arg/value_hint.rs

intgr · 2020-04-08T14:02:06Z

src/build/arg/value_hint.rs

+    /// Command name looked up from PATH or full path to executable
+    CommandName,
+    /// A command and each argument as multiple strings
+    CommandWithArguments,


The CommandWithArguments is a very important use case for implementing command wrappers, but also very different from all the others. For it to work correctly, it assumes that the arg is a positional argument, has multiple(true) and app has AppSettings::TrailingVarArg.

Can/should I add these checks to some place?

I'd go with setting multiple(true) and AppSettings::TrailingVarArg automatically and documenting this behavior.

clap_generate/src/generators/shells/zsh.rs

pksunkara · 2020-04-08T17:25:32Z

Also, can we do something about bash and fish too? I could try my hand at them later, but it would be great if we can support them in this PR.

CreepySkeleton

Looks great, just a little question.

clap_generate/src/generators/shells/zsh.rs

intgr · 2020-04-08T18:44:12Z

Also, can we do something about bash and fish too? I could try my hand at them later, but it would be great if we can support them in this PR.

Honestly, I'd prefer to keep the scope of this PR as small as possible. This is my first contribution to clap, I'm still learning Rust, that's already lots of new things. I haven't used bash for years, never even tried fish.

Dylan-DPC-zz · 2020-04-08T18:45:55Z

@intgr agree, go on don't worry. we can discuss that in a separate issue/pr

CreepySkeleton · 2020-04-10T02:20:50Z

Is it possible to extract the type of data to do value hinting automatically

I doubt this is possible with raw clap since it's "stringy typed", no way to get type information. This might be possible with derive but it would be a very simple heuristic (is it path or not). This hint is not a big burden, anyway.

CreepySkeleton · 2020-04-10T16:53:01Z

Also, can we do something about bash and fish too? I could try my hand at them later, but it would be great if we can support them in this PR.

I can try to improve bash later. no experience with fish, and I'm not really interested.

pickfire · 2020-04-11T17:01:53Z

I use fish here but I don't seemed to see something equivalent (maybe I don't have much experience with completions), IIRC fish could use custom command to do these kind of completion hinting and there is no limit on what could be hint, this is both good news (it is very flexible since we can just inject some code) and bad news (there is just too many things we can hint) for us.

benjumanji · 2020-04-11T19:40:42Z

I use fish here but I don't seemed to see something equivalent

As is the usual way with fish, rather than create lots of specialized options fish simply uses the same tools as the end user to provide useful functionality.

https://fishshell.com/docs/current/index.html#useful-functions-for-writing-completions

pksunkara · 2020-04-12T17:29:50Z

Hey, sorry about this, but you should probably rebase on master since we touched clap_generate this week.

intgr · 2020-04-12T17:46:22Z

@benjumanji Actually yeah, I looked into fish already on Thursday and whipped up some rudimentary support. It's surprisingly sane compared to bash and zsh completion definitions.

pksunkara

Yeah, that is why I use fish. I would have added support later on anyway, but it's not priority clap wise. Glad you could finish it.

Please write here once you consider the PR complete so that we can review. Thanks.

src/build/arg/mod.rs

intgr · 2020-04-12T18:28:41Z

Yeah this is still WIP and not entirely ready for a review yet. Meanwhile, some points for bikeshedding:

CreepySkeleton suggested that ValueHint::CommandWithArguments should automatically set AppSettings::TrailingVarArg and multiple(), which would make hints directly affect command parsing as well. So it seems it's not just a "hint" any more.

Any suggestions what to rename ValueHint to? CompletionType seems bad for the same reason, it doesn't just affect completion. ValueKind? ...
I'm not happy with the naming of ValueHint::CommandString (one string containing a whole shell command) and ValueHint::CommandWithArguments (command and arguments as separate arguments), suggestions welcome.

CreepySkeleton · 2020-04-12T18:39:15Z

Right, but allowing completions to disagree with actual implementation is also not very good. Well, this is not necessarily bad, we may try to sell it as "Help the generator but remember: enforcing correctness is up to you".

intgr · 2020-04-12T18:41:03Z

Yeah it makes sense, I'm just thinking if I should stop calling it a "hint" now.

pksunkara · 2020-04-12T19:44:33Z

Or, we could keep it as a hint, but add an assert that any arg hinted as ValueHint::CommandWithArguments is inside an app with setting TrailingVarArg. I recently made asserts easier to implement with #1797

CreepySkeleton · 2020-04-12T20:48:52Z

I'd go for ValueKind and extensive documentation.

benjumanji · 2020-04-13T10:26:49Z

I know my opinion means nothing here, but @intgr I'm am so excited for this to land! I have a new cli project that I kicked off last night and was going to pause it to work on this but you have basically already implemented everything I need.

EDIT: I also think ValueKind is a better name, and I am really for the embedding of a bit of shell to do fully dynamic completions. I don't think it will be reasonably possible to avoid this, and it isn't untrusted input or anything insane like that. At least I don't think it will be possible to avoid without clap itself basically reimplementing a ton of stuff, which seems like a really bad trade off.

intgr · 2020-04-15T22:26:12Z

@benjumanji I definitely see the value in custom completion functions or some other "escape hatch" mechanism, but it seems more complicated, less portable between shells and not something I need myself right now.

However, it's worth thinking ahead to what that API would look like, to make sure I'm not painting clap into a corner somehow with my current work. Personally I can't imagine any way they would interact, apart from completion generators which would prefer the custom completion over ValueHint if there is one.

intgr · 2020-06-05T07:38:28Z

By the way, do completion test snapshots (e.g. static ZSH_SPECIAL_CMDS: &str in clap_generate/tests/completions.rs) have to be updated by hand or is there an easier way?

pksunkara · 2020-06-05T07:39:40Z

No easier way for now :(

CreepySkeleton · 2020-07-07T11:57:47Z

@intgr knock knock

intgr · 2020-07-18T12:03:06Z

Hey people. I'm on vacation now and hopefully with enough motivation to move this forward. Sorry for the wait. :)

One part of this MR that I have postponed is the YAML (de)serialization part, if anyone can give me some quick pointers -- where to start -- that would be great.

CreepySkeleton · 2020-07-18T13:21:20Z

One part of this MR that I have postponed is the YAML (de)serialization part, if anyone can give me some quick pointers -- where to start -- that would be great.

Just implement FromStr for it (case insensitive) and then use yaml_str! as it's used in thereabouts.

intgr · 2020-07-18T17:38:34Z

I ended up creating a new yaml_str_parse!() macro because the yaml_str! does not call parse() on the value. Or should I just inline the code instead of creating a new macro?

btw I also found it confusing that there are 2 different macros with the name yaml_str!

intgr · 2020-07-18T22:48:18Z

I think the initial feature set is now complete, feel free to start reviewing.

There are issues with the fish generator, which already existed before my changes:

It does not handle positional arguments at all -- only named arguments.
There are some odd completion issues with examples/value_hints.rs, which disappear if I remove the -n __fish_use_subcommand. It seems the generator shouldn't add it if the App doesn't use subcommands at all?

I'm not very interested in fixing these issues in the fish generator. Does that seem OK? Should I keep my fish changes in or back them out from this PR? I believe my changes don't make anything worse, but it's easier to notice these problems now.

intgr · 2020-07-20T13:10:30Z

I also added a support matrix table to ValueHint documentation:

Hint	zsh	fish¹
`AnyPath`	Yes	Yes
`FilePath`	Yes	Yes
`DirPath`	Yes	Yes
`ExecutablePath`	Yes	Partial
`CommandName`	Yes	Yes
`CommandString`	Yes	Partial
`CommandWithArguments`	Yes
`Username`	Yes	Yes
`Hostname`	Yes	Yes
`Url`	Yes
`EmailAddress`	Yes

fish completions currently only support named arguments (e.g. -o or --opt), not
positional arguments. ↩

intgr · 2020-07-31T15:34:44Z

Ping? Rebased to latest master.

CreepySkeleton

Sorry for the delay. I'm not the best person to review this kind of PR because I have very shallow experience with completions and stuff, but others doesn't seem to be very interested, so here we go.

I say the PR is ready. Everything that's supposed to work does work (except for that wasn't working before), documentation is in place, and I think the code is good. Great job!

Could you please fix the test (we changed extensions) and ensure cargo fmt & clippy is applied? And the squash the commits.

There are some odd completion issues with examples/value_hints.rs, which disappear if I remove the -n __fish_use_subcommand. It seems the generator shouldn't add it if the App doesn't use subcommands at all?

That's right. It's not your problem until you say so.

tests/yaml.rs

Adds new method/attribute `Arg::value_hint`, taking a `ValueHint` enum as argument. The hint can denote accepted values, for example: paths, usernames, hostnames, commands, etc. This initial implementation supports hints for the zsh and fish completion generators, support for other shells can be added later.

intgr · 2020-08-06T19:20:28Z

Thanks for the feedback!

Doh, I did not notice that the tests broke with my last rebase. Fixed!

Are you happy with the naming of ValueHint as well? Earlier you preferred ValueKind.

pickfire · 2020-08-07T03:05:23Z

Can we add conflicts for fish shell? Is that not possible?

CreepySkeleton · 2020-08-07T18:13:44Z

Are you happy with the naming of ValueHint as well? Earlier you preferred ValueKind.

That was then, this is now.

I now think that using "right" words in public API is more important than being 100% technically correct. What you did here, it was to allow hinting about the type first and foremost; stuff like assets and implications was brought after. It was a convince, not the intention. And the intention was to hint.

I'm pretty happy with how this went along.

bors r+

bors · 2020-08-07T18:34:43Z

Build succeeded:

intgr · 2020-08-07T23:36:55Z

Great, will be looking forward to the next (beta) release :)

pksunkara

Great work! I was unfortunately busy to review this immediately. But I don't see any issues at all.

pksunkara self-requested a review April 8, 2020 06:25

pickfire reviewed Apr 8, 2020

View reviewed changes

clap_generate/src/generators/shells/zsh.rs Outdated Show resolved Hide resolved

intgr commented Apr 8, 2020

View reviewed changes

intgr mentioned this pull request Apr 8, 2020

zsh completion is too strict on command line args #850

Closed

pksunkara reviewed Apr 8, 2020

View reviewed changes

clap_generate/src/generators/shells/zsh.rs Outdated Show resolved Hide resolved

CreepySkeleton reviewed Apr 8, 2020

View reviewed changes

clap_generate/src/generators/shells/zsh.rs Outdated Show resolved Hide resolved

pksunkara mentioned this pull request Apr 9, 2020

completions: Support adding additional code to complete values #568

Closed

intgr force-pushed the value-hints-for-zsh-completion branch from 288891b to 278ba99 Compare April 12, 2020 17:49

pksunkara reviewed Apr 12, 2020

View reviewed changes

src/build/arg/mod.rs Outdated Show resolved Hide resolved

intgr force-pushed the value-hints-for-zsh-completion branch from 278ba99 to 5d2a6dc Compare April 16, 2020 00:01

intgr force-pushed the value-hints-for-zsh-completion branch from 553292a to 0df6c30 Compare July 18, 2020 11:24

intgr force-pushed the value-hints-for-zsh-completion branch from ae206b7 to e3b45ad Compare July 18, 2020 16:01

pksunkara self-requested a review July 19, 2020 10:49

intgr force-pushed the value-hints-for-zsh-completion branch from d49374f to 3827ac0 Compare July 20, 2020 12:17

intgr mentioned this pull request Jul 21, 2020

Rant: MultipleValues considered harmful #2031

Closed

intgr force-pushed the value-hints-for-zsh-completion branch from 0b3d8fb to 89bda82 Compare July 31, 2020 15:33

CreepySkeleton approved these changes Aug 6, 2020

View reviewed changes

tests/yaml.rs Outdated Show resolved Hide resolved

intgr force-pushed the value-hints-for-zsh-completion branch from dae4900 to c5b4e89 Compare August 6, 2020 19:16

intgr force-pushed the value-hints-for-zsh-completion branch from c5b4e89 to 64ee0f8 Compare August 6, 2020 19:21

intgr changed the title ~~Add hinting of allowed value types for zsh completion~~ Add hinting of arg value types for zsh/fish completion Aug 6, 2020

bors bot merged commit 6492ae2 into clap-rs:master Aug 7, 2020

pksunkara reviewed Aug 11, 2020

View reviewed changes

intgr mentioned this pull request Sep 15, 2020

Tracking Issue for 3.x #1037

Closed

87 tasks

This was referenced Dec 6, 2021

Dynamic completion support epage/clapng#92

Open

Compute argument possible values based on other arguments epage/clapng#157

Open

Add hinting of arg value types for zsh/fish completion #1793

Add hinting of arg value types for zsh/fish completion #1793

Conversation

intgr commented Apr 7, 2020 • edited Loading

intgr commented Apr 7, 2020 • edited Loading

pksunkara commented Apr 8, 2020

pickfire commented Apr 8, 2020

intgr commented Apr 8, 2020

intgr Apr 8, 2020

Choose a reason for hiding this comment

pickfire Apr 8, 2020

Choose a reason for hiding this comment

intgr Apr 8, 2020

Choose a reason for hiding this comment

intgr Apr 8, 2020

Choose a reason for hiding this comment

CreepySkeleton Apr 9, 2020

Choose a reason for hiding this comment

pksunkara commented Apr 8, 2020

CreepySkeleton left a comment

Choose a reason for hiding this comment

intgr commented Apr 8, 2020

Dylan-DPC-zz commented Apr 8, 2020

CreepySkeleton commented Apr 10, 2020

CreepySkeleton commented Apr 10, 2020

pickfire commented Apr 11, 2020

benjumanji commented Apr 11, 2020 • edited Loading

pksunkara commented Apr 12, 2020

intgr commented Apr 12, 2020

pksunkara left a comment

Choose a reason for hiding this comment

intgr commented Apr 12, 2020

CreepySkeleton commented Apr 12, 2020

intgr commented Apr 12, 2020

pksunkara commented Apr 12, 2020 • edited Loading

CreepySkeleton commented Apr 12, 2020

benjumanji commented Apr 13, 2020 • edited Loading

intgr commented Apr 15, 2020 • edited Loading

intgr commented Jun 5, 2020

pksunkara commented Jun 5, 2020

CreepySkeleton commented Jul 7, 2020

intgr commented Jul 18, 2020

CreepySkeleton commented Jul 18, 2020

intgr commented Jul 18, 2020

intgr commented Jul 18, 2020 • edited Loading

intgr commented Jul 20, 2020 • edited Loading

Footnotes

intgr commented Jul 31, 2020

CreepySkeleton left a comment

Choose a reason for hiding this comment

intgr commented Aug 6, 2020

pickfire commented Aug 7, 2020

CreepySkeleton commented Aug 7, 2020

bors bot commented Aug 7, 2020

intgr commented Aug 7, 2020

pksunkara left a comment

Choose a reason for hiding this comment

intgr commented Apr 7, 2020 •

edited

Loading

intgr commented Apr 7, 2020 •

edited

Loading

benjumanji commented Apr 11, 2020 •

edited

Loading

pksunkara commented Apr 12, 2020 •

edited

Loading

benjumanji commented Apr 13, 2020 •

edited

Loading

intgr commented Apr 15, 2020 •

edited

Loading

intgr commented Jul 18, 2020 •

edited

Loading

intgr commented Jul 20, 2020 •

edited

Loading