Skip to content

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

Conversation

EmilyGraceSeville7cf
Copy link
Contributor

@EmilyGraceSeville7cf EmilyGraceSeville7cf commented Sep 17, 2024

Description

Fixes issue #

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@EmilyGraceSeville7cf EmilyGraceSeville7cf marked this pull request as draft September 17, 2024 17:04
complete -c $command -f

complete -c $command \
-s h \
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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. 😄

Copy link
Member

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).

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

That's fine.

@EmilyGraceSeville7cf EmilyGraceSeville7cf marked this pull request as ready for review September 17, 2024 17:44
@@ -0,0 +1,41 @@
set command ajv
Copy link
Member

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

Copy link
Member

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.


complete -c $command -f

set subcommands_with_descriptions 'validate\t"Validate the files against a schema"' \
Copy link
Member

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.


complete -c $command -f

complete -c $command -s h -l help -d 'Show [h]elp'
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@EmilyGraceSeville7cf EmilyGraceSeville7cf Sep 29, 2024

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

Choose a reason for hiding this comment

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

Missed these.

@faho faho closed this Mar 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants