clipanion icon indicating copy to clipboard operation
clipanion copied to clipboard

feat: completion (experimental)

Open paul-soporan opened this issue 4 years ago • 9 comments

This PR implements support for shell completions of all argument types. It's still a WIP, but I'm getting really really really close to having the first iteration ready for review.

Note: This PR also fixes some small bugs and improves various errors.

TODO list

Required before merging this PR:

  • [x] Investigate the behavior of -<tab>
  • [x] Implement PartialCommand type correctly
  • [x] Make sure that completions are sorted the same way across all shells
  • [x] Rework and finalize API of completion commands
  • [x] Code cleanup
  • [ ] ~~Documentation~~ (will document before we mark it as stable)
  • [ ] ~~Demo~~ (will add a demo before we mark it as stable)

Nice to have: (can be implemented after this PR is merged)

  • [ ] Escape shell completions according to each shell's rules
  • [ ] Complete all paths
  • [ ] Complete all long option names
  • [ ] Investigate possible bug where rest consumes trailing positionals (not introduced by this PR so can be left for another time)
  • [ ] Look into how each shell aggregates partially overlapping results and see whether we need to do anything to improve the way the completions are displayed
  • [ ] Option.stringify
  • [ ] Make clipanion display how much time is spent executing each completion function when in debug mode
  • [ ] Support PowerShell's CompletionType
  • [ ] Sort the completions
  • [ ] Cache the completions

paul-soporan avatar May 30 '21 22:05 paul-soporan

Note: This error will only go away once clcs gets published to the registry.

paul-soporan avatar May 31 '21 21:05 paul-soporan

I've only started the review, but it looks really nice! Some areas of improvements I've identified so far:

  • Documentation (it's a bit difficult to find out how to use it)
  • Integrating it with the demo example (tied to the previous point)
  • I feel like there should be a shortcut to register all completion-related shortcuts in one go

arcanis avatar Jun 03 '21 16:06 arcanis

Implement PartialCommand type correctly

Declared here: https://github.com/arcanis/clipanion/pull/89/files#diff-4a1eed5930fe968b46fdf97ea4fc46e1c61585a16fbd0b4565eae339a345d050R46 I'm not sure how the type needs to change, though.

Investigate possible bug where rest consumes trailing positionals

Is there a brief explanation of this bug and how to reproduce it?

Escape shell completions according to each shell's rules

Is there a way to escape spaces with compgen -W ''? Did a bit of searching; it seems like compgen doesn't support spaces in completion words passed to -W?

Look into how each shell aggregates partially overlapping results and see whether we need to do anything to improve the way the completions are displayed

Is there a brief example of this? I'm not entirely sure what this means or when it occurs.

cspotcode avatar Aug 05 '21 14:08 cspotcode

I'm not sure how the type needs to change, though.

See https://github.com/arcanis/clipanion/pull/89/commits/c4d8d916cee5dca735060462f9d44d5c44dc5a80

Is there a brief explanation of this bug and how to reproduce it?

I don't fully remember but I'll look into it.

Is there a way to escape spaces with compgen -W ''? Did a bit of searching; it seems like compgen doesn't support spaces in completion words passed to -W?

I have no idea but I'll also look into this.

Is there a brief example of this? I'm not entirely sure what this means or when it occurs.

It occurs whenever Clipanion returns results that have some common properties (e.g. the completionText or the description). I want to know what the behavior of each shell is and what we could do either when normalizing the results in clcs or when setting the shell completion options to make the displayed completions look better.

paul-soporan avatar Aug 11 '21 22:08 paul-soporan

Really impressive work; I didn't look at clcs in details yet, but the changes in Clipanion look mostly fine (just the part about the callbacks being non-serializables; since serializing the state machine didn't happen so far I wouldn't be too sad to factor it away if it improves performances, but I'd prefer if it was in a separate PR, if ever) and it works well on my zsh.

Some extra remarks:

  • It seems there's a bug if a command has options that have validators attached. The demo didn't work until I removed the maxRetries option. I suspect it's because it causes the code to throw.

  • There's some documentation to be made around the fact that the binary must have a name; in particular, the yarn <bin name> story will have to be figured out (although I suspect it'll be much easier than yarn <script name>, which is perhaps too much to ask).

Really, impressive work as always!

arcanis avatar Jan 19 '22 14:01 arcanis

Oh, something else I forgot to add: I noticed that completions can merge options & non-options. I wonder if we could improve that to only show non-options if there's any (with the assumption that if someone wants to complete an option, it's not unreasonable they'd solve the ambiguity by first typing the leading -). I suspect the results would more closely match the expectations:

image

arcanis avatar Jan 19 '22 14:01 arcanis

it's not unreasonable they'd solve the ambiguity by first typing the leading -

That's already what happens, we only complete options if a - is typed, otherwise we only complete path segments and positionals. The thing is that the clipanion builtins that look like options (--help, --version, and --clipanion=definitions) are actually path segments, not options. Having special treatment for path segments that look like options is on my TODO list, but I decided to leave it for a future iteration.

paul-soporan avatar Jan 19 '22 15:01 paul-soporan

I see, that's surprising at first but it makes sense 👍

arcanis avatar Jan 19 '22 15:01 arcanis

It seems there's a bug if a command has options that have validators attached.

Totally forgot about validators, I'll need a bit of time to think about how validators should behave with PartialCommand. I'm thinking about disabling the validators while completing things but there's also the problem of coercions :thinking:

There's some documentation to be made around the fact that the binary must have a name;

I'll leave all documentation for future PRs.

in particular, the yarn story will have to be figured out (although I suspect it'll be much easier than yarn

I'll continue the discussion about this in the review conversation about binaryName.

paul-soporan avatar Jan 19 '22 18:01 paul-soporan