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
Add support for completions in fish
shell + a few dev scripts to ease workflow
#2281
base: main
Are you sure you want to change the base?
Conversation
- 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>
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.
Left a couple nits, looks mostly good to me though. Thank you for the contribution.
@@ -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", |
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.
Is --watch
built into newer versions of npm?
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.
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
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 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 tobuild:cjs:watch
. I'll make a comment there too...
So if the combination of comments makes sense, I suggest
"compile:watch": "npm run compile -- --watch", | |
"compile:watch": "tsc --watch -p tsconfig.json", |
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 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!
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`
- 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
@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 |
@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 ;) |
@jglanz your work looks good to me, but there seems to be a regression happening in Node 16. Any thoughts? |
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.
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.
@@ -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", |
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 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 tobuild:cjs:watch
. I'll make a comment there too...
So if the combination of comments makes sense, I suggest
"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", |
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 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!)
"dev": "npm run compile && run-p compile:watch build:cjs:watch", | |
"dev": "rimraf build && run-p compile:watch build:cjs:watch", |
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.
What is run-p
? (Perhaps run-parallel from some package you have installed?)
(Added some light comments on the helper scripts. Inspired by this, I should start using them instead of forgetting to recompile after code changes!) |
@@ -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 ### |
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.
Although we may well wish to acknowledge Omelette somewhere, it seems misleading to say "generated by omelette.js" when it was generated by Yargs?
@@ -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 = |
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 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.
@@ -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) { |
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 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
}
(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!) |
# | ||
# yargs command completion script | ||
# | ||
# Installation: {{app_name}} {{completion_command}} >> ~/.config/fish/config.fish. |
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 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
I checked my suspicion that the current
|
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 For example: |
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.) |
added support for completions in
fish
shellTo use with
fish
, in fish shell, run<app-name> completion >> ~/.config/fish/config.fish
add scripts making
dev
a bit easier