-
Notifications
You must be signed in to change notification settings - Fork 391
refactor: _parse_{help,usage}
=> _comp_compgen_help
#954
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
d6f8402
to
c133b1c
Compare
8a203c1 is automatically generated by running
|
bash_completion
Outdated
# -u Parse the BSD style usage output. The default is in the GNU | ||
# style help output. | ||
# @param $1 command command; if "-", read from stdin and ignore rest of args | ||
# @param $2 opt command options (default: --help) |
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'm wondering if passing everything after a --
to the command would be a cleaner and more consistent API for this? For example _comp_compgen
uses that approach.
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 also thought about it, but as far as the command name $1
does not start with -
, we can actually omit --
. The difference in _comp_compgen
is that the arguments to compgen
always starts with -
. But I think it is good to prefix --
before every "$1"
.
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.
Hmm, maybe it is better to split the function into two separate functions _comp_compgen_help
and _comp_compgen_usage
.
Before introducing the idea _comp_compgen NAME
, I originally combined _parse_help
and _parse_usage
in a single function because it shared long codes of the command-line option parsing. But now the option parsing is moved to _comp_compgen
, so two processings help
and usage
now share only a small part of the code. I'll think about it again.
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 split the function into _comp_compgen_help
and _comp_compgen_usage
.
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 the split better. Perhaps still provide command options as $2...
instead of cramming them into single $2
string, for a bit cleaner call site code and avoiding a _comp_split
?
Then again, I suppose we could make the first parameter optional too, and default it to ${comp_args[0]}
. If so, usage like [-c command] [--] [command_opts...]
could be in order.
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.
Then again, I suppose we could make the first parameter optional too, and default it to
${comp_args[0]}
. If so, usage like[-c command] [--] [command_opts...]
could be in order.
I like this idea. Let me implement 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.
After thinking about it again, I have slightly changed the suggested command line usage:
usage: _comp_compgen_help -
usage: _comp_compgen_help -c cmd args...
usage: _comp_compgen_help [[--] args...]
- When we want to read stdin, requiring to write
_comp_compgen_help -c -
seems strange, so_comp_compgen_help -
is supported separately. - When the caller specifies the command and arguments, splitting between
$1
and$2...
with--
as_comp_compgen_help -c cmd -- args
also seems not so natural. So when the first argument is-c
, the later words are all treated as the command and arguments as is. _comp_dequote
is only applied tocomp_args[0]
. The command name specified through-c cmd args...
is not de-quoted by_comp_dequote
. When it is needed, the caller should take care of it.--help
(or--usage
in the case of_comp_compgen_usage
) is only supplemented when no arguments are specified. E.g., when the user uses_comp_compgen_help --
, it doesn't specify--help
but directly callscomp_args[0]
without arguments.
I rebased and modified existing commits. There are additional commits:
6f22632
to
9b1b5d9
Compare
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 could clean up some redundant passing of --help
while at it.
bash_completion
Outdated
# -u Parse the BSD style usage output. The default is in the GNU | ||
# style help output. | ||
# @param $1 command command; if "-", read from stdin and ignore rest of args | ||
# @param $2 opt command options (default: --help) |
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 the split better. Perhaps still provide command options as $2...
instead of cramming them into single $2
string, for a bit cleaner call site code and avoiding a _comp_split
?
Then again, I suppose we could make the first parameter optional too, and default it to ${comp_args[0]}
. If so, usage like [-c command] [--] [command_opts...]
could be in order.
03eb948
to
11e6e6d
Compare
Co-authored-by: Ville Skyttä <[email protected]>
We also remove the backslash of '-\?' within this patch. The backslash was introduced in commit 60b8fab, which seems to have tried to work around the failglob failure. Because we now directly specify the arguments to _comp_compgen_help so that we do not have to evaluate unquoted $2, we do not need to add the backslash anymore. Note that the backslash added to ssh-keygen in 60b8fab was removed in commit 4dc0016 where _parse_help was switched to _parse_usage. References: scop@60b8fab scop@4dc0016
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.
Nice, pre-approved with some comments addressed.
Co-authored-by: Ville Skyttä <[email protected]>
Co-authored-by: Koichi Murase <[email protected]>
scop#954 (comment) scop#964 scop#962 (comment) Co-authored-by: Ville Skyttä <[email protected]>
scop#954 (comment) scop#964 scop#962 (comment) Co-authored-by: Ville Skyttä <[email protected]>
This is based on #952 so would be kept to be Draft until PR #952 settles.
The own changes of this PR are 2ce600f...39b15f3. This PR includes the API change of
_parse_{help,usage}
and trivial rewriting for switching from_parse_{help,usage}
. Other non-trivial rewriting related to_parse_{help,usage}
will be included in later PRs.