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.

Sign up for GitHub

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

Jump to bottom

Add support for completions in fish shell + a few dev scripts to ease workflow #2281

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add support for completions in fish shell + a few dev scripts to ease workflow #2281

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e846cf6
- add support for completions in `fish` shell
Dec 21, 2022
c0ee6e8
17.6.3-0
Dec 22, 2022
d84dc66
Updated gitignore to ignore tgz files
Dec 22, 2022
4513d9b
Fixed an issue with nested commands in the `fish` completion template.
Dec 23, 2022
6cb69fb
build: pin npm version (#2284)
bcoe Dec 28, 2022
2c144c4
feat: add method to hide option extras (#2156)
jly36963 Dec 28, 2022
1fd530a
chore: add en strings for unknown command (#2262)
shadowspawn Dec 28, 2022
f37ee6f
fix: copy the description of the option to its alias in completion (#…
tasshi-me Dec 28, 2022
99b8281
- add support for completions in `fish` shell
Dec 21, 2022
6ae7a5e
Updated gitignore to ignore tgz files
Dec 22, 2022
e0e6f7f
Fixed an issue with nested commands in the `fish` completion template.
Dec 23, 2022
c04439a
- `package.json`: fixed nit
Dec 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ package-lock.json
example.*
.DS_Store
.npmrc
*.tgz
20 changes: 20 additions & 0 deletions lib/completion-templates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,23 @@ _{{app_name}}_yargs_completions()
compdef _{{app_name}}_yargs_completions {{app_name}}
###-end-{{app_name}}-completions-###
`;

export const completionFishTemplate = `### {{app_name}} completion - begin. generated by omelette.js ###
Copy link
Member

Choose a reason for hiding this comment

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

Although we may well wish to acknowledge Omelette somewhere, it seems misleading to say "generated by omelette.js" when it was generated by Yargs?

#
# yargs command completion script
#
# Installation: {{app_name}} {{completion_command}} >> ~/.config/fish/config.fish.
Copy link
Member

Choose a reason for hiding this comment

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

I noticed a completions folder when I installed fish to try this out. Would it be more appropriate to suggest writing the completion into ~/.config/fish/completions/{{app_name}}.fish?

https://fishshell.com/docs/current/completions.html#where-to-put-completions

#
function _{{app_name}}_yargs_completion
set cmd (commandline -b)
for arg in (string split " " $cmd)
set -a args "'$arg'"
end
set completions (eval {{app_name}} --get-yargs-completions $args)
for completion in $completions
echo -e $completion
end
end
complete -f -c {{app_name}} -a '(_{{app_name}}_yargs_completion)'
### {{app_name}} completion - end ###
`;
69 changes: 52 additions & 17 deletions lib/completion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@ type CompletionCallback = (
/** Instance of the completion module. */
export interface CompletionInstance {
completionKey: string;

generateCompletionScript($0: string, cmd: string): string;

getCompletion(
args: string[],
done: (err: Error | null, completions: string[] | undefined) => void
): any;

registerFunction(fn: CompletionFunction): void;

setParsed(parsed: DetailedArguments): void;
}

Expand All @@ -34,6 +38,8 @@ export class Completion implements CompletionInstance {
private customCompletionFunction: CompletionFunction | null = null;
private indexAfterLastReset = 0;
private readonly zshShell: boolean;
private readonly fishShell: boolean;
private readonly bashShell: boolean;

constructor(
private readonly yargs: YargsInstance,
Expand All @@ -45,6 +51,10 @@ export class Completion implements CompletionInstance {
(this.shim.getEnv('SHELL')?.includes('zsh') ||
this.shim.getEnv('ZSH_NAME')?.includes('zsh')) ??
false;
this.fishShell = this.shim.getEnv('SHELL')?.includes('fish') ?? false;
this.bashShell =
Copy link
Member

Choose a reason for hiding this comment

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

I get the intent, but the combination of the name bashShell and the use in the later code block are confusing. I'll comment more there.

this.shim.getEnv('SHELL')?.includes('bash') ??
(!this.zshShell && !this.fishShell);
}

private defaultCompletion(
Expand Down Expand Up @@ -92,8 +102,11 @@ export class Completion implements CompletionInstance {
this.usage.getCommands().forEach(usageCommand => {
const commandName = parseCommand(usageCommand[0]).cmd;
if (args.indexOf(commandName) === -1) {
if (!this.zshShell) {
if (this.bashShell) {
Copy link
Member

Choose a reason for hiding this comment

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

It is hard to reason about what shell gets handled where here. The original code had basically zsh and not zsh. I suggest keep the same sort of logic, so it is a more obvious diff, and easier to understand what gets handled where.

if (this.fishShell) {
} else if (this.zshShell) {
} else {
// bash et al
}

completions.push(commandName);
} else if (this.fishShell) {
const desc = usageCommand[1] || '';
completions.push(commandName.replace(/:/g, '\\:') + '\t' + desc);
} else {
const desc = usageCommand[1] || '';
completions.push(commandName.replace(/:/g, '\\:') + ':' + desc);
Expand All @@ -107,7 +120,7 @@ export class Completion implements CompletionInstance {
private optionCompletions(
completions: string[],
args: string[],
argv: Arguments,
_argv: Arguments,
current: string
) {
if (
Expand All @@ -119,7 +132,7 @@ export class Completion implements CompletionInstance {
this.yargs.getGroups()[this.usage.getPositionalGroupName()] || [];

Object.keys(options.key).forEach(key => {
const negable =
const negatable =
!!options.configuration['boolean-negation'] &&
options.boolean.includes(key);
const isPositionalKey = positionalKeys.includes(key);
Expand All @@ -128,11 +141,12 @@ export class Completion implements CompletionInstance {
if (
!isPositionalKey &&
!options.hiddenOptions.includes(key) &&
!this.argsContainKey(args, key, negable)
!this.argsContainKey(args, key, negatable)
) {
this.completeOptionKey(key, completions, current);
if (negable && !!options.default[key])
if (negatable && !!options.default[key]) {
this.completeOptionKey(`no-${key}`, completions, current);
}
}
});
}
Expand Down Expand Up @@ -188,15 +202,19 @@ export class Completion implements CompletionInstance {
}

private getPreviousArgChoices(args: string[]): string[] | void {
if (args.length < 1) return; // no args
if (args.length < 1) {
return;
} // no args
let previousArg = args[args.length - 1];
let filter = '';
// use second to last argument if the last one is not an option starting with --
if (!previousArg.startsWith('-') && args.length > 1) {
filter = previousArg; // use last arg as filter for choices
previousArg = args[args.length - 2];
}
if (!previousArg.startsWith('-')) return; // still no valid arg, abort
if (!previousArg.startsWith('-')) {
return;
} // still no valid arg, abort
const previousArgKey = previousArg.replace(/^-+/, '');

const options = this.yargs.getOptions();
Expand Down Expand Up @@ -230,15 +248,21 @@ export class Completion implements CompletionInstance {
private argsContainKey(
args: string[],
key: string,
negable: boolean
negatable: boolean
): boolean {
const argsContains = (s: string) =>
args.indexOf((/^[^0-9]$/.test(s) ? '-' : '--') + s) !== -1;
if (argsContains(key)) return true;
if (negable && argsContains(`no-${key}`)) return true;
if (argsContains(key)) {
return true;
}
if (negatable && argsContains(`no-${key}`)) {
return true;
}
if (this.aliases) {
for (const alias of this.aliases[key]) {
if (argsContains(alias)) return true;
if (argsContains(alias)) {
return true;
}
}
}
return false;
Expand All @@ -255,8 +279,14 @@ export class Completion implements CompletionInstance {
const isShortOption = (s: string) => /^[^0-9]$/.test(s);
const dashes =
!startsByTwoDashes(current) && isShortOption(key) ? '-' : '--';
if (!this.zshShell) {
if (this.bashShell) {
completions.push(dashes + key);
} else if (this.fishShell) {
const desc = descs[key] || '';
completions.push(
dashes +
`${key.replace(/:/g, '\\:')}\t${desc.replace('__yargsString__:', '')}`
);
} else {
const desc = descs[key] || '';
completions.push(
Expand Down Expand Up @@ -333,17 +363,22 @@ export class Completion implements CompletionInstance {

// generate the completion script to add to your .bashrc.
generateCompletionScript($0: string, cmd: string): string {
let script = this.zshShell
const script = this.zshShell
? templates.completionZshTemplate
: this.fishShell
? templates.completionFishTemplate
: templates.completionShTemplate;
const name = this.shim.path.basename($0);

// add ./ to applications not yet installed as bin.
if ($0.match(/\.js$/)) $0 = `./${$0}`;
if ($0.match(/\.js$/)) {
$0 = `./${$0}`;
}

script = script.replace(/{{app_name}}/g, name);
script = script.replace(/{{completion_command}}/g, cmd);
return script.replace(/{{app_path}}/g, $0);
return script
.replace(/{{app_name}}/g, name)
.replace(/{{completion_command}}/g, cmd)
.replace(/{{app_path}}/g, $0);
}

// register a function to perform your own custom
Expand Down
8 changes: 6 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "yargs",
"version": "17.6.2",
"version": "17.6.3-0",
jglanz marked this conversation as resolved.
Show resolved Hide resolved
"description": "yargs the modern, pirate-themed, successor to optimist.",
"main": "./index.cjs",
"exports": {
Expand Down Expand Up @@ -76,6 +76,7 @@
"gts": "^3.0.0",
"hashish": "0.0.4",
"mocha": "^9.0.0",
"npm-run-all": "^4.1.5",
"rimraf": "^3.0.2",
"rollup": "^2.23.0",
"rollup-plugin-cleanup": "^3.1.1",
Expand All @@ -94,10 +95,13 @@
"coverage": "c8 report --check-coverage",
"prepare": "npm run compile",
"pretest": "npm run compile -- -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs",
"compile": "rimraf build && tsc",
"compile": "rimraf build && tsc -b tsconfig.json",
"compile:watch": "npm run compile -- --watch",
Copy link
Member

Choose a reason for hiding this comment

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

Is --watch built into newer versions of npm?

Copy link
Author

Choose a reason for hiding this comment

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

When you use -- with npm scripts it passes thru to the end of the configured script, so compile:watch materializes as rimraf build && tsc -b tsconfig.json --watch

Copy link
Member

Choose a reason for hiding this comment

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

  1. It does involve some copy-paste repetition, but I prefer the pattern you used for build:cjs:watch where you copied the command so you could add the watch.
  2. the dev script does a compile, then runs compile-watch, which does a compile, which includes a rimraf which runs in parallel to build:cjs:watch. I'll make a comment there too...

So if the combination of comments makes sense, I suggest

Suggested change
"compile:watch": "npm run compile -- --watch",
"compile:watch": "tsc --watch -p tsconfig.json",

Copy link
Member

Choose a reason for hiding this comment

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

I realised I am comfortable passing command-line flags into npm run-script from the command-line, just don't normally do it between run-scripts, so weakening my suggestion above. Just an alternative approach!

"postcompile": "npm run build:cjs",
"build:cjs": "rollup -c rollup.config.cjs",
"build:cjs:watch": "rollup -w -c rollup.config.cjs",
"postbuild:cjs": "rimraf ./build/index.cjs.d.ts",
"dev": "npm run compile && run-p compile:watch build:cjs:watch",
Copy link
Member

Choose a reason for hiding this comment

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

I think the current scripts mean this will run compile twice, and invokes rimraf twice. Maybe do the rimraf up front and remove it from the watch scripts. (Disclaimer: making these suggestions online without trying it for real!)

Suggested change
"dev": "npm run compile && run-p compile:watch build:cjs:watch",
"dev": "rimraf build && run-p compile:watch build:cjs:watch",

Copy link
Member

Choose a reason for hiding this comment

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

What is run-p? (Perhaps run-parallel from some package you have installed?)

"check": "gts lint && npm run check:js",
"check:js": "eslint . --ext cjs --ext mjs --ext js",
"clean": "gts clean"
Expand Down