Skip to content

feat(jq): add support for all options, and fix --from-file #507

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

Merged
merged 1 commit into from
Mar 28, 2021

Conversation

rrthomas
Copy link
Contributor

@rrthomas rrthomas commented Mar 9, 2021

The options are not all listed by --help, so look in the man page to find a
nearly complete list (I hope!). Neither the man page nor --help mentions --help
or --version; I have not looked elsewhere to see if there might be yet more.

The treatment of -f|--from-file was incorrect: that option’s argument is the
filter, so the argument should be counted towards the total number of arguments
so far. Hence, remove --from-file from the list of options whose argument is
counted as an option.

@scop
Copy link
Owner

scop commented Mar 11, 2021

Thanks! Options commit pushed as be4e155

For the other parts, we could brush up a crystal ball and try to figure out if the jq being used does output a "complete enough" listing of its options with --help. If it doesn't, then use our hardcoded list here as a fallback -- either amending what we got, or replacing it altogether. Brittle/ugly, sure. Would you find that tolerable? If yes, what would you use as the "canary" to trigger use of our list?

@rrthomas
Copy link
Contributor Author

For the other parts, we could brush up a crystal ball and try to figure out if the jq being used does output a "complete enough" listing of its options with --help. If it doesn't, then use our hardcoded list here as a fallback -- either amending what we got, or replacing it altogether. Brittle/ugly, sure. Would you find that tolerable? If yes, what would you use as the "canary" to trigger use of our list?

How about --help as the canary?

@scop
Copy link
Owner

scop commented Mar 11, 2021

Maybe. Or if we predict they might not repeat --help in --help output, maybe --version.

@rrthomas
Copy link
Contributor Author

@scop your guess is as good as mine! But it would be weird not to include --help in the output after I filed an issue about this problem, and as most programs do…

@scop
Copy link
Owner

scop commented Mar 16, 2021

Cool. Would you mind revising this to implement the fallback behavior? --help works as the canary for me too.

@rrthomas
Copy link
Contributor Author

Sure, I can do that.

@rrthomas
Copy link
Contributor Author

Apologies for my slowness, I have been very busy, and just now that includes using jq.

I found that there's a serious problem with the completion as it stands. It tries to force the user to use a "mode" argument first, such as --slurp. However, the code as it stands does not count any mode arguments, so the number of arguments never exceeds 1 unless you manually type a file argument, so file arguments are never auto-completed.

I had a look at the manual of jq to determine what these mode arguments are, because in theory adding a list of them as the third argument to _count_args would make the code work.

However, the concept of "mode" does not seem to exist in the manual. Indeed, no such argument is required. (You can run jq quite happily without any command-line option.)

So, I propose simply deleting the args-counting code, specifically this bit:

    local args
    # TODO: DTRT with args taking 2 options
    # -f|--from-file are not counted here because they supply the filter
    _count_args "" "@(--arg|--arg?(json|file)|--slurpfile|--indent|--run-tests|-!(-*)L)"

    # 1st arg is filter
    echo jq args: $args
    ((args == 1)) && return
    # 2... are input files

@rrthomas
Copy link
Contributor Author

(I can still implement the --help canary when I finally get a moment, of course!)

@scop
Copy link
Owner

scop commented Mar 27, 2021

No problem wrt slowness. My time for bash-completion is spotty, too.

But I don't quite follow the suggestion, could you elaborate it with a literal example what doesn't work? I also think that's a separate issue from the original one, and would be better off handled in a separate issue if there is one.

Anyway, as a counterexample, if I delete the code you suggested, jq --slurpfile foo <TAB> no longer completes with (.json) filenames. In my jq manpage, --slurpfile is documented to take two args, first of which is a variable name (which we explicitly don't complete), and the second is a JSON filename.

@rrthomas
Copy link
Contributor Author

@scop I opened #510, and tried to improve the presentation to make it easier to understand what the problem is.

@rrthomas
Copy link
Contributor Author

@scop, I"ve updated my PR with the --help canary.

@rrthomas
Copy link
Contributor Author

Thanks very much for your patience and detailed feedback, @scop; as you can see, I'm not fluent programming in bash!

I hope I've dealt with all your feedback…

The options are not all listed by --help, so look in the man page to
find a nearly complete list (I hope!). Neither the man page nor --help
mentions --help or --version; I have not looked elsewhere to see if
there might be yet more.

Guard against the possibility of jq being fixed in future by testing to
see if its --help output contains “--help” and using it if so,
otherwise, use the hard-coded list; see
jqlang/jq#2284
@scop scop merged commit 450c676 into scop:master Mar 28, 2021
@scop
Copy link
Owner

scop commented Mar 28, 2021

I took the liberty to do some line wrapping changes, removed duplicated --version, fixed s/--compact-input/--compact-output/, and removed some superfluous quotes (shfmt -s is our friend). Thanks!

@rrthomas
Copy link
Contributor Author

Thanks for the polish and applying the PR; I've installed shfmt for future use.

@rrthomas
Copy link
Contributor Author

And good spot with the typo!

@scop
Copy link
Owner

scop commented Mar 28, 2021

Re shfmt, I suggest installing pre-commit if you wish to run all pre-commit checks configured for the project. CI also runs them, but is badly broken for other reasons at the moment.

@rrthomas
Copy link
Contributor Author

Thanks for the tip!

@github-actions github-actions bot mentioned this pull request Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants