-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add/Refactor completions for: JSON schema validators #10729
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/Refactor completions for: JSON schema validators #10729
Conversation
share/completions/boon.fish
Outdated
complete -c $command -f | ||
|
||
complete -c $command \ | ||
-s h \ |
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.
Please don't break these lines up so much. Just put it on one line, possibly two.
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.
Ig, I can use 80 character limit to determine whether to distribute a line over several ones or not. Wanna keep completions readable and not to make myself scroll horizontally.
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 will apply the same logic for all other my PRs. 😄
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.
These are well below 80 characters, which is a limit that we do not actually have in our fish script style.
If you must split them up, here's how I would do that:
complete -c $command \
-s o \
-d 'The output file of a migrated schema' \
-n "__fish_seen_subcommand_from validate migrate" \
-F -r
becomes
complete -c $command -s o -F -r \
-n "__fish_seen_subcommand_from validate migrate" \
-d 'The output file of a migrated schema'
Basically put the big condition and description on their own lines, but don't give each little option its own line. That just wastes a ton of vertical space and gives you very empty lines (-s o
is basically nothing).
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 your suggestions, I also put -a
on a separate line, to keep the following structure of the completion:
- option's values
- options with additional flags like
-F
- description
- condition
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.
That's fine.
share/completions/ajv.fish
Outdated
@@ -0,0 +1,41 @@ | |||
set command ajv |
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.
These should be local-scoped, or you'll overwrite someone's variable named "command".
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.
Tho tbh just remove this entirely and use command -c ajv
.
It's not like you're gonna copy these into other files anyway.
share/completions/ajv.fish
Outdated
|
||
complete -c $command -f | ||
|
||
set subcommands_with_descriptions 'validate\t"Validate the files against a schema"' \ |
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.
These definitely need to be set -l
.
share/completions/boon.fish
Outdated
|
||
complete -c $command -f | ||
|
||
complete -c $command -s h -l help -d 'Show [h]elp' |
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.
We don't really do the [h]
mnemonic, please remove it.
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 all other mnemonics be removed as well?
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've removed all mnemonics in square brackets.
|
||
complete -c $command -f | ||
|
||
complete -c $command -s h -l help -d 'Show [h]elp' |
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.
Missed these.
Description
boon
,ajv
,json_schemer
.jv
.Fixes issue #
TODOs: