New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow calling standard completion function from custom one #1855
Allow calling standard completion function from custom one #1855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
lib/completion.ts
Outdated
@@ -34,6 +34,78 @@ export function completion( | |||
const current = args.length ? args[args.length - 1] : ''; | |||
const argv = yargs.parse(args, true); | |||
const parentCommands = yargs.getContext().commands; | |||
|
|||
function defaultCompletion(): Arguments | void { | |||
const handlers = command.getCommandHandlers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would be tempted to pull each of these major pieces of logic into a helper function within completion.ts
:
commandCompletions
?optionCompletions
?customCompletions
?
lib/completion.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
function isSyncCompletionFunction( | ||
completionFunction: CompletionFunction | ||
): completionFunction is SyncCompletionFunction { | ||
return completionFunction.length < 3; | ||
} | ||
|
||
function isFallbackCompletionFunction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be completionFunction.arguments.length
I'm surprised this worked.
}); | ||
|
||
it('allows calling callback instead of default completion function', done => { | ||
checkUsage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests read quite well 👍
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: