Skip to content
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

Merged
merged 5 commits into from Feb 17, 2021

Conversation

gardenappl
Copy link
Contributor

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:

export type CompletionFunction =
  | SyncCompletionFunction
  | AsyncCompletionFunction
  | FallbackCompletionFunction;

interface SyncCompletionFunction {
  (current: string, argv: Arguments): string[] | Promise<string[]>;
}

interface AsyncCompletionFunction {
  (current: string, argv: Arguments, done: (completions: string[]) => any): any;
}

interface FallbackCompletionFunction {
  (
    current: string,
    argv: Arguments,
    defaultCompletion: () => any,
    done: (completions: string[]) => any
  ): any;
}

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:

export type CompletionFunction =
  | LegacyCompletionFunction
  | FallbackCompletionFunction;

interface LegacyCompletionFunction {
  (current: string, argv: Arguments): string[] | Promise<string[]>;
}

interface FallbackCompletionFunction {
  (current: string, argv: Arguments, defaultCompletion: () => any): string[] | Promise<string[]> | void;
}

Copy link
Member

@bcoe bcoe left a 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.

@@ -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();
Copy link
Member

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?

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)
Copy link
Member

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(
Copy link
Member

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;
Copy link
Member

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests read quite well 👍

@gardenappl gardenappl force-pushed the enable-fallback-to-standard-completion branch 4 times, most recently from d20001d to 881b204 Compare February 16, 2021 22:22
@gardenappl gardenappl force-pushed the enable-fallback-to-standard-completion branch from 881b204 to 486532d Compare February 16, 2021 22:24
@gardenappl
Copy link
Contributor Author

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 also fixed handling of short options in aliases (see changes in unit tests).

@gardenappl
Copy link
Contributor Author

I am still kinda bothered by the fact that the API itself is not very clean. Right now the behaviour of completion(...) depends on how many arguments the function accepts:

  • 2 args = synchronous function or Promise function, can't call default
  • 3 args = callback function, can't call default
  • 4 args = callback function, can call default

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.

Copy link
Member

@bcoe bcoe left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants