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

jglanz · 2022-12-22T00:08:32Z

added support for completions in `fish` shell

To use with fish, in fish shell, run <app-name> completion >> ~/.config/fish/config.fish

add scripts making `dev` a bit easier

- add scripts making `dev` a bit easier To use with `fish`, in fish shell, run `<app-name> completion >> ~/.config/fish/config.fish`

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

…args#2269)

bcoe

Left a couple nits, looks mostly good to me though. Thank you for the contribution.

package.json

bcoe · 2022-12-28T18:32:13Z

package.json

@@ -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",


Is --watch built into newer versions of npm?

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

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.

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",

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!

jglanz · 2022-12-28T23:44:38Z

For sure, i'll get it cleaned up in the next day or so. Of all the arg parsing frameworks, nothing compares. Happy to contribute moving forward as well.

- add scripts making `dev` a bit easier To use with `fish`, in fish shell, run `<app-name> completion >> ~/.config/fish/config.fish`

@mshrtsr

- rebased against `desc` fix for zsh alias completions from @mshrtsr - fixed a `bash` misspelling in tests - added `stderr` to `testCmd` result for better error reporting

jglanz · 2022-12-29T17:53:57Z

@bcoe hey - sorry when i rebased I messed up some ordering, everything has been addressed and all tests pass. If you'd like me to scrap the PR and do up a fresh one just say so, but everything is correct & complete now. Apologies, but it is all correct now, just LMK

jglanz · 2023-01-16T20:58:57Z

Left a couple nits, looks mostly good to me though. Thank you for the contribution.

@bcoe Happy New Year! I resolved the nits, can you give it a quick review? I'd love to get this landed as I'm using it at work ;)

bcoe · 2023-02-13T16:44:58Z

@jglanz your work looks good to me, but there seems to be a regression happening in Node 16. Any thoughts?

bcoe

Okay, I think I got tests passing again (there was a regression introduced in Node 16).

The one thing blocking this PR is that I believe coverage drops a little bit, mind taking a look? we might just be missing one test.

shadowspawn · 2023-02-25T22:51:05Z

package.json

@@ -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",


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.

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",

shadowspawn · 2023-02-25T22:54:42Z

package.json

    "postbuild:cjs": "rimraf ./build/index.cjs.d.ts",
+    "dev": "npm run compile && run-p compile:watch build:cjs:watch",


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",

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

shadowspawn · 2023-02-26T00:26:15Z

(Added some light comments on the helper scripts. Inspired by this, I should start using them instead of forgetting to recompile after code changes!)

shadowspawn · 2023-03-12T05:18:06Z

lib/completion-templates.ts

@@ -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 ###


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

shadowspawn · 2023-03-12T05:21:45Z

lib/completion.ts

@@ -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 =


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.

shadowspawn · 2023-03-12T05:25:49Z

lib/completion.ts

@@ -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) {


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 }

shadowspawn · 2023-03-12T05:34:50Z

(Darn you GitHub, why are you showing me changesets that have landed in main! 😡 )

(Edit: hmm, might be because of the merge conflict? So comparing against older version of main... Hopefully!)

shadowspawn · 2023-04-13T08:09:28Z

lib/completion-templates.ts

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


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

shadowspawn · 2023-04-13T08:34:09Z

I checked my suspicion that the current dev run-script not going to work as intended. I replaced the actions with echo statements so I could see what was going on to more easily see what got called. This exposed the postcompile script getting in on the action too! (The existing script setup is quite subtle!)

    "compile": "echo run-script-compile",
    "compile:watch": "npm run compile -- --watch",
    "postcompile": "npm run build:cjs",
    "build:cjs": "echo run-script-build:cjs",
    "build:cjs:watch": "echo run-script-build:cjs:watch",
    "postbuild:cjs": "rimraf ./build/index.cjs.d.ts",
    "dev": "npm run compile && npm run compile:watch && npm run build:cjs:watch",

% npm run dev

> yargs@17.7.1 dev
> npm run compile && npm run compile:watch && npm run build:cjs:watch


> yargs@17.7.1 compile
> echo run-script-compile

run-script-compile

> yargs@17.7.1 postcompile
> npm run build:cjs


> yargs@17.7.1 build:cjs
> echo run-script-build:cjs

run-script-build:cjs

> yargs@17.7.1 postbuild:cjs
> rimraf ./build/index.cjs.d.ts


> yargs@17.7.1 compile:watch
> npm run compile -- --watch


> yargs@17.7.1 compile
> echo run-script-compile --watch

run-script-compile --watch

> yargs@17.7.1 postcompile
> npm run build:cjs


> yargs@17.7.1 build:cjs
> echo run-script-build:cjs

run-script-build:cjs

> yargs@17.7.1 postbuild:cjs
> rimraf ./build/index.cjs.d.ts


> yargs@17.7.1 build:cjs:watch
> echo run-script-build:cjs:watch

run-script-build:cjs:watch

shadowspawn · 2023-04-13T09:27:39Z

In retrospect this PR might have been better as two PR, so apologies that I have feedback about both aspects!

The naming I am familiar with from work and I think is more common in the wild is npm run watch rather than npm run dev to spin up the watches.

For example:

shadowspawn · 2023-04-15T01:58:16Z

Are you still interested in working on this @jglanz ?

I appreciate it has been slow going for you from initial submission, I have new suggestions, and you have it working locally anyway. If you aren't interested then I'll close this and other people may pick up the pieces.

(I personally do not use fish shell, but do want a watch script! I would list you as co-author if I submit a PR for that.)

Jonathan Glanz added 4 commits December 21, 2022 19:05

- add support for completions in fish shell

e846cf6

- add scripts making `dev` a bit easier To use with `fish`, in fish shell, run `<app-name> completion >> ~/.config/fish/config.fish`

17.6.3-0

c0ee6e8

Updated gitignore to ignore tgz files

d84dc66

Fixed an issue with nested commands in the fish completion template.

4513d9b

jglanz mentioned this pull request Dec 23, 2022

fish shell completion support added in pr #2281 - please review whenever possible #2282

Open

bcoe and others added 4 commits December 28, 2022 18:03

build: pin npm version (yargs#2284)

6cb69fb

feat: add method to hide option extras (yargs#2156)

2c144c4

chore: add en strings for unknown command (yargs#2262)

1fd530a

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

fix: copy the description of the option to its alias in completion (y…

f37ee6f

…args#2269)

bcoe requested changes Dec 28, 2022

View reviewed changes

Jonathan Glanz added 4 commits December 29, 2022 11:39

- add support for completions in fish shell

99b8281

- add scripts making `dev` a bit easier To use with `fish`, in fish shell, run `<app-name> completion >> ~/.config/fish/config.fish`

Updated gitignore to ignore tgz files

6ae7a5e

Fixed an issue with nested commands in the fish completion template.

e0e6f7f

- package.json: fixed nit

c04439a

- rebased against `desc` fix for zsh alias completions from @mshrtsr - fixed a `bash` misspelling in tests - added `stderr` to `testCmd` result for better error reporting

jglanz requested a review from bcoe December 29, 2022 17:55

bcoe added the ci label Feb 13, 2023

bcoe requested changes Feb 13, 2023

View reviewed changes

line-o mentioned this pull request Feb 23, 2023

Add completion for fish shell eXist-db/xst#95

Open

shadowspawn reviewed Feb 25, 2023

View reviewed changes

shadowspawn reviewed Mar 12, 2023

View reviewed changes

shadowspawn reviewed Apr 13, 2023

View reviewed changes

shadowspawn mentioned this pull request Apr 22, 2023

Add watch run-script #2320

Merged

shadowspawn mentioned this pull request Apr 25, 2024

Support fish shell completions. #1904

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jglanz commented Dec 22, 2022

bcoe left a comment

bcoe Dec 28, 2022

jglanz Dec 29, 2022

shadowspawn Feb 25, 2023

shadowspawn Mar 23, 2023

jglanz commented Dec 28, 2022

jglanz commented Dec 29, 2022

jglanz commented Jan 16, 2023

bcoe commented Feb 13, 2023

bcoe left a comment

shadowspawn Feb 25, 2023

shadowspawn Feb 25, 2023

shadowspawn Apr 13, 2023

shadowspawn commented Feb 26, 2023

shadowspawn Mar 12, 2023

shadowspawn Mar 12, 2023 •

edited

shadowspawn Mar 12, 2023

shadowspawn commented Mar 12, 2023 •

edited

shadowspawn Apr 13, 2023

shadowspawn commented Apr 13, 2023

shadowspawn commented Apr 13, 2023

shadowspawn commented Apr 15, 2023

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

		"postbuild:cjs": "rimraf ./build/index.cjs.d.ts",
		"dev": "npm run compile && run-p compile:watch build:cjs:watch",

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

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

Are you sure you want to change the base?

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

Conversation

jglanz commented Dec 22, 2022

added support for completions in fish shell

add scripts making dev a bit easier

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

Choose a reason for hiding this comment

jglanz commented Dec 28, 2022

jglanz commented Dec 29, 2022

jglanz commented Jan 16, 2023

bcoe commented Feb 13, 2023

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

shadowspawn commented Feb 26, 2023

Choose a reason for hiding this comment

shadowspawn Mar 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowspawn commented Mar 12, 2023 • edited

Choose a reason for hiding this comment

shadowspawn commented Apr 13, 2023

shadowspawn commented Apr 13, 2023

shadowspawn commented Apr 15, 2023

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

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

added support for completions in `fish` shell

add scripts making `dev` a bit easier

shadowspawn Mar 12, 2023 •

edited

shadowspawn commented Mar 12, 2023 •

edited