feat: completion (experimental) #89

paul-soporan · 2021-05-30T22:44:34Z

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:

Investigate the behavior of -<tab>
Implement PartialCommand type correctly
Make sure that completions are sorted the same way across all shells
Rework and finalize API of completion commands
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)

arcanis · 2022-01-19T14:37:21Z

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!

sources/core.ts

arcanis · 2022-01-19T13:23:34Z

sources/core.ts

@@ -867,6 +968,19 @@ export class CommandBuilder<Context> {

      for (let t = 0; t < path.length; ++t) {
        const nextPathNode = injectNode(machine, makeNode());
+
+        // TODO: find a way to not leak command into the core
+        registerDynamic(machine, lastPathNode, `isCompletion`, NODE_SUCCESS, [`setCompletion`, CompletionType.PathSegment, (request, command) => {


The main reason why tuples are passed to registerDynamic was to make the state machine serializable. We can't pass a callback as parameter for this to stay true

I feel like it would make more sense to make the state machine serializable again in a different PR when we actually start using the serializability property - the implementation would vary depending on the specific use case.

For example, I could have a workaround to not passing callbacks but it would only make the code more complicated for no reason for now, and if in the future we'd want to pre-generate the state machine for example and hydrate it at runtime the implementation would be incompatible and would have to be changed anyways.

arcanis · 2022-01-19T13:33:15Z

sources/core.ts

-    registerDynamic(machine, node, [`isBatchOption`, this.allOptionNames], node, `pushBatch`);
-    registerDynamic(machine, node, [`isBoundOption`, this.allOptionNames, this.options], node, `pushBound`);
+    registerDynamic(machine, node, [`isBatchOption`, this.allOptionNames], node, [`chain`, [
+      [`setBatchCompletion`, this],


Same thing, passing this will prevent serialization

arcanis · 2022-01-19T13:36:50Z

package.json

  },
  "scripts": {
    "prepack": "rm -rf lib && rollup -c",
    "postpack": "rm -rf lib",
-    "test": "FORCE_COLOR=1 mocha --require ts-node/register --extension ts tests",
+    "test:clipanion": "FORCE_COLOR=1 mocha --require ts-node/register tests/**/*.test.ts",
+    "test": "yarn test:clipanion & yarn test:clcs &",


I think mocha supports --parallel

Looks like I get TypeError: The "options" argument must be of type object. Received an instance of Array and Error: write EPIPE when using --parallel, no idea why

arcanis · 2022-01-19T13:40:20Z

tests/completion.test.ts

+        })).to.deep.equal([`sources`]);
+      });
+
+      it(`shouldn't spend time filtering out completions that don't match as that's the job of the shell scripts`, async () => {


Wouldn't it be done faster by JS rather than shells? (Plus, it could remove many entries that wouldn't have to go through the stdout stream, which isn't the fastest operation either)

arcanis · 2022-01-19T13:54:23Z

sources/advanced/builtins/completion/index.ts

+  completionCleanupCommandPaths,
+  completionProviderCommandPaths,
+  completionRequestCommandPaths,
+  completionDebugCommandPaths,


Those options are a bit long; can't we just have a single one that would operate as a configurable prefix? I use a similar pattern in Griselbrand:

https://github.com/arcanis/griselbrand/blob/main/sources/index.ts#L71

I need the granularity for Yarn (you might remember the discussion about @yarnpkg/completion-engine & friends). I think this is enough for the Yarn use case, and the common case that will satisfy the needs of 99% of CLIs is already implemented (the current path defaults).

completion is already the best namespace possible, so if a project deviates from it, it most likely will have to customize more than just the mount point (e.g. the magic required for Yarn completion).

If in the future a common use case for a common mount point arises, we can implement it, but for now I don't really see the need.

sources/advanced/builtins/completion/completionCleanup.ts

sources/advanced/builtins/completion/completionSetup.ts

arcanis · 2022-01-19T13:58:29Z

sources/advanced/builtins/completion/completionSetup.ts

+      // We make sure that we don't register a completion provider for an invalid binary before writing to the shell configuration file.
+      validateBinaryName(this.cli.binaryName);
+
+      return await setupShellConfigurationFile({


Nothing is currently printed; I think it'd be a good idea to print some basic information: which shell we detected, where we're going to write things, which command to run to remove it and/or update it.

arcanis · 2022-01-19T14:08:37Z

sources/advanced/builtins/completion/completionSetup.ts

+      validateBinaryName(this.cli.binaryName);
+
+      return await setupShellConfigurationFile({
+        completionProviderCommand: [this.cli.binaryName, ...completionProviderCommandPaths[0] ?? []].join(` `),


Question for a followup: I sometimes have CLI defined in my package.json; something like this:

"scripts": { "cli": "node ./scripts/cli.js" }

In those cases, I define binaryName as yarn cli. How would that work here? Is it something that should be implemented in Yarn or Clipanion (perhaps both: Yarn could somehow signal the "real" binary name to Clipanion, and use its proxy completion to get the real one)?

arcanis · 2022-01-19T14:41:23Z

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:

paul-soporan · 2022-01-19T15:05:16Z

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.

arcanis · 2022-01-19T15:38:01Z

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

paul-soporan · 2022-01-19T18:02:07Z

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 🤔

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 <script name>, which is perhaps too much to ask).

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

paul-soporan added 30 commits April 26, 2021 02:15

completion wip

c71b0fc

more tests

c348453

more jsdoc

c5e4112

complete bound names

ce76909

complete batches

62e6c29

support for long option names

3f134db

support negated long option names

98eb3a8

get rid of suggestMachine

e0abaa6

get rid of suggest

a433636

support invalid bound options

26cf59f

Merge remote-tracking branch 'arcanis/master' into completion

9c6a652

support help indexes

4940767

complete --help

b40dba9

get rid of partial statics

cc0e17f

test proxies and --

6f6a1e8

error on unsupported batch options

965fd14

better unsupported batch option errors

4ae545f

better unsupported binding errors

04c4906

imperative code for faster completions

3190cf4

move clcs to clipanion monorepo

d50a15c

clcs refactoring

3818810

more clcs refactoring

ab0ee28

even more clcs refactoring

9c5bd1b

cleanup

5a662ce

only display names that can actually be negated

77392d4

bind this to undefined in completion functions

4334160

throw error when cursor is missing from input in debug mode

d861e67

show descriptions for command segments

f922451

drop node 10 compatibility

8fd6da8

get rid of Command.prototype.completion

336c38f

paul-soporan added 8 commits January 17, 2022 00:58

fix: fix suffix slice call

bf8183a

refactor: pass colored in CompletionRequestCommand

62dc139

refactor: uniformize clcs command options

79c3758

refactor: get rid of unused hidden param

736f762

style: remove newline for consistency

0b1c43b

refactor: don't complete negated option names in binding

17e45e2

feat: validate binaryName before setting up configuration file

cade0ae

chore: increase timeout

cd35a84

paul-soporan marked this pull request as ready for review January 17, 2022 23:25

paul-soporan requested a review from arcanis January 17, 2022 23:35

arcanis reviewed Jan 19, 2022

View reviewed changes

Enables completion on the demo

ff8e391

fix: use absolute path in demo script

9b09ad3

paul-soporan added 3 commits January 19, 2022 20:20

refactor: add comment about +1

3e75a9c

refactor: add another comment

dc5feb6

refactor: add usage to completion {setup,cleanup}

cc01714

SamB mentioned this pull request Feb 12, 2023

[Feature] CLI autocomplete yarnpkg/berry#512

Open

Merge branch 'master' into completion

d102f1e

arcanis mentioned this pull request Jul 22, 2023

Implements tokenization #146

Merged

paul-soporan added 5 commits July 26, 2023 04:35

refactor: remove shim

dd58200

ci: change tested node versions

d6bd4e4

feat: allow providing binaryName to completion{Setup,Cleanup}

25ae45a

refactor: pass it to execute instead

33414ea

refactor: remove leftover

2b8c0cc

arcanis mentioned this pull request Oct 6, 2023

Lazy commands #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: completion (experimental) #89

feat: completion (experimental) #89

paul-soporan commented May 30, 2021 •

edited

Loading

arcanis commented Jan 19, 2022

arcanis Jan 19, 2022

paul-soporan Feb 2, 2022

arcanis Jan 19, 2022

arcanis Jan 19, 2022

paul-soporan Jan 19, 2022

arcanis Jan 19, 2022 •

edited

Loading

arcanis Jan 19, 2022

paul-soporan Feb 2, 2022

arcanis Jan 19, 2022

arcanis Jan 19, 2022

arcanis commented Jan 19, 2022

paul-soporan commented Jan 19, 2022

arcanis commented Jan 19, 2022

paul-soporan commented Jan 19, 2022

feat: completion (experimental) #89

Are you sure you want to change the base?

feat: completion (experimental) #89

Conversation

paul-soporan commented May 30, 2021 • edited Loading

TODO list

arcanis commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Jan 19, 2022

paul-soporan commented Jan 19, 2022

arcanis commented Jan 19, 2022

paul-soporan commented Jan 19, 2022

paul-soporan commented May 30, 2021 •

edited

Loading

arcanis Jan 19, 2022 •

edited

Loading