Allow calling standard completion function from custom one#1855
Allow calling standard completion function from custom one#1855bcoe merged 5 commits intoyargs:masterfrom
Conversation
bcoe
left a comment
There was a problem hiding this comment.
@gardenappl this is looking really solid to me. What jumps out at me the most is that we've collected enough logic in completion.ts, that I think we'd benefit from a tiny bit of refactoring.
| const parentCommands = yargs.getContext().commands; | ||
|
|
||
| function defaultCompletion(): Arguments | void { | ||
| const handlers = command.getCommandHandlers(); |
There was a problem hiding this comment.
i would be tempted to pull each of these major pieces of logic into a helper function within completion.ts:
commandCompletions?optionCompletions?customCompletions?
| options.boolean.includes(key); | ||
| // If the key and its aliases aren't in 'args', add the key to 'completions' | ||
| let keyAndAliases = [key].concat(aliases[key] || []); | ||
| if (negable) |
There was a problem hiding this comment.
It's this block especially where I think we'd benefit from a bit more decomposition. Could completeOptionKey could be pulled out to the top level of the file, along with us breaking apart defaultCompletion into a couple more helpers?
I think if we then add comments to each of the helpers, the completion logic might become easier to consume.
| return completionFunction.length < 3; | ||
| } | ||
|
|
||
| function isFallbackCompletionFunction( |
There was a problem hiding this comment.
I like the use of type detection here 👍
| function isFallbackCompletionFunction( | ||
| completionFunction: CompletionFunction | ||
| ): completionFunction is FallbackCompletionFunction { | ||
| return completionFunction.length > 3; |
There was a problem hiding this comment.
should this be completionFunction.arguments.length I'm surprised this worked.
| }); | ||
|
|
||
| it('allows calling callback instead of default completion function', done => { | ||
| checkUsage( |
d20001d to
881b204
Compare
881b204 to
486532d
Compare
|
I tried splitting the code into multiple functions, but they required a lot of arguments and it didn't look good at all. So instead I spent some time refactoring the whole thing into a class. |
|
I am still kinda bothered by the fact that the API itself is not very clean. Right now the behaviour of
But this means that if you want to call the default function then you have to use callbacks. I already mentioned this when I created the PR and would like to hear some feedback on that. |
bcoe
left a comment
There was a problem hiding this comment.
I think pulling the logic into a class was a very good compromise... and approach we could perhaps use elsewhere in the codebase.
LGTM. I will merge and release to a next branch so that you can test.
See #1013
Based on work by @tkarls (#1013 (comment)). This is my first time working with TypeScript so my solution might be a bit ugly.
One thing that worries me is that this API requires you to use callbacks. There is no good way to overload this function:
Each function takes 2, 3 or 4 arguments respectively.
One possible solution: since you're about to release a new major version, it might be OK to just remove the ability to use callbacks, in favor of Promises. So then the new API would look like this: