Skip to content

feat(_comp_compgen_split): use _comp_compgen_split for -W '$(...)' #989

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 10 commits into from
May 29, 2023

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented May 23, 2023

Resolve #958

This PR introduces a new generator _comp_compgen_split that splits the argument to generate words. The difference from the builtin compgen -W is that it is safe to pass command outputs to the argument because it doesn't evaluate the expansions in the argument. It was still possible with passing the command substitution with single quotes as compgen -W '$(command)', but it requires excessive quoting and also is incompatible with _comp_compgen when the command contains positional parameters such as $1. In addition, the inside of the single-quoted command substitutions '$(...)' is not checked by shellcheck.

This is still a draft because I think I need to add tests for the new function _comp_compgen_split.

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

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

Pre-approved with some comments, those tests added, and conflicts resolved.

BTW this exposed quite a few unaddressed cases more where we are incorrectly using a hardcoded command name instead of the one being invoked, and missing $PATH prepends. I'll look into them sometime later when this is in.

@scop
Copy link
Owner

scop commented May 23, 2023

Resolve #958

I'm not sure if it's the intent here, but if this would be in the merge commit, I'm not sure if it would automatically close the referenced issue. https://github.blog/2013-01-22-closing-issues-via-commit-messages/ has some related info on the recognized words, but that's a bit dated. I've usually used Closes: ....

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 23, 2023

Resolve #958

I'm not sure if it's the intent here,

The current version of the list of keywords are described in Linking a pull request to an issue using a keyword - GitHub Docs, which includes resolve. I actually tend to use Resolve if that is not really an existing bug. I've actually already used Resolve in #986, and it seems to have worked intendedly to successfully close #932.

When I'm unsure about whether the issue is really linked, I hover the mouse pointer over the keyword to see if it shows a tooltip. In this case, it seems to show one:

image

@akinomyoga
Copy link
Collaborator Author

I pushed more fixes: 788a862, 7becdda

@akinomyoga akinomyoga force-pushed the _comp_compgen-6 branch 3 times, most recently from deeaec6 to 8bc4c93 Compare May 23, 2023 23:09
The option `-X arg` for `compgen` is specified twice, but only the
latter takes effect.  Also, `-X kernel` excludes the word `kernel` but
the latter `-X '!*.ko'` also excludes the word `kernel` because it
only leaves the words matches the pattern `*.ko`.  Thus, we can just
filter the words by `-X '!*.ko'`.
@akinomyoga akinomyoga marked this pull request as ready for review May 28, 2023 20:08
@akinomyoga akinomyoga merged commit a3345c6 into scop:master May 29, 2023
@akinomyoga akinomyoga deleted the _comp_compgen-6 branch May 29, 2023 11:25
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.

Over-eager positional param check in _comp_compgen
2 participants