-
Notifications
You must be signed in to change notification settings - Fork 391
feat(tmux): new completion #1364
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
base: main
Are you sure you want to change the base?
Conversation
At a glance, the test failure looks unrelated. Let me know if I need to do anything about 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.
The general quality of this PR seems very good, but I'm busy this week. I'll look at this later.
Thanks, and no rush. (In addition to the changes in comments above, I also added a new test |
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.
Looks good overall to me on a quick glance, thanks!
Left a few remarks, but I'll happily defer remainder of this review to someone else, as I don't use tmux myself (yet, anyway, I guess).
completions/tmux
Outdated
case "$option_type" in | ||
command) | ||
_comp_compgen_split -l -- \ | ||
"$(LC_ALL=C tmux list-commands -F "#{command_list_name}")" |
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.
Would be good to pass around the original tmux command invoked instead of hardcoding just tmux
from $PATH
(or as an alias or a function) -- tmux
might not be in $PATH
(or the like) in the first place, or the tmux
there might not be the one that was invoked (maybe another one with a full path to it was).
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.
To add more context, one simple way is to use "$1"
(at the top-level function) or "${comp_args[0]}"
(in the helper functions that are called from the top-level function). A more robust way is to use _comp_dequote "${comp_args[0]}"
as _comp_compgen_help__get_help_lines
is doing:
bash-completion/bash_completion
Lines 1561 to 1569 in 90162b0
local REPLY | |
_comp_dequote "${comp_args[0]-}" || REPLY=${comp_args[0]-} | |
help_cmd=("${REPLY:-false}" "$@") | |
;; | |
esac | |
local REPLY | |
_comp_split -l REPLY "$(LC_ALL=C "${help_cmd[@]}" 2>&1)" && | |
_lines=("${REPLY[@]}") |
Note: I try to slightly change the behavior of _comp_dequote
in 3e088b7 (PR #1357), so that one does not need to prefix || REPLY=${comp_args[0]-}
.
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 checking the implementation, I think maybe we can just use this function _comp_compgen_help__get_help_lines
for broader purposes (with a small adjustment). Even with the current implementation, we can use _comp_compgen_help__get_help_lines
for the present purpose in the following way:
_comp_compgen_help__get_help_lines -- list-commands -F "#{command_list_name}" &&
REPLY=("${_lines[@]}") &&
_comp_compgen -- -W '"${REPLY[@]}"'
Then, we may consider renaming this function to _comp_run_cmd
and change its output variable from _lines
to REPLY
.
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.
_comp_compgen_help__get_help_lines
is private, so I shouldn't use it as is, right?
Should I wait for changes to _comp_compgen_help__get_help_lines
and/or _comp_dequote
, or use "${comp_args[0]}"
/ "$1"
for now?
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.
_comp_compgen_help__get_help_lines
is private, so I shouldn't use it as is, right?
Yes, so I wrote
Then, we may consider renaming this function to
_comp_run_cmd
and change its output variable from_lines
toREPLY
.
Should I wait for changes to
_comp_compgen_help__get_help_lines
and/or_comp_dequote
, or use"${comp_args[0]}"
/"$1"
for now?
You can change _comp_compgen_help__get_help_lines
if other maintainers agree with 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.
Ok, I'll wait a bit to see if @scop or others have opinions before changing this part.
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 added _comp_cmd_tmux__run
to do this without _comp_compgen_help__get_help_lines
for now. Happy to switch to that later if maintainers want to rename it and make it public.
_comp_dequote "${comp_args[0]-}" || REPLY=${comp_args[0]-}
_comp_dequote
's documentation says that REPLY
is an array, so I didn't use that exactly. Should _comp_compgen_help__get_help_lines
be changed too?
I also updated the semicolon detection to treat REPLY
as an array too.
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.
OK, I think we can update _comp_compgen_help__get_help_lines
(_comp_run_cmd
) in another PR.
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
_comp_compgen_help__get_help_lines
be changed too?
Yeah, and it is actually addressed in 3e088b7 in #1357 (where REPLY
is assigned after REPLY=()
). It was actually a fix after the CI #2838 failure.
Thanks for the reviews! I'll try to get to these comments soon. |
Just FYI, tmux/tmux@68ffe65 will change some of the usage syntax in the next tmux release. The changes look relatively minor, and I'd like to support both the old and new syntaxes for at least a little while. And since this PR is already rather large, I'd like to go forward with this just supporting the syntax in released tmux versions, then make another PR adding support for the new changes, if that's ok. |
OK. |
I think I've addressed all the comments now, let me know if there's anything else I should change. (Or if I missed anything.) In addition to changes from the comments, I also added one more test case that tmux/tmux#4480 reminded me about:
|
From tmux/tmux#259 it doesn't look like the tmux maintainers were interested in having a bash completion script in the upstream repo. However, I added license headers to put these new files under either bash-completion's license or tmux's, in case they want it there in the future. I'm aware of a handful of existing tmux bash completion scripts, below. As far as I can tell, they all hard-code a decent amount of tmux's available options, commands, etc. Some are also abandoned and out of date with more recent versions of tmux. Rather than base this code off of those, I decided to implement completion using tmux's own introspection commands as much as possible. Hopefully that will reduce the ongoing maintenance work and make it stay up to date with most tmux changes automatically. This commit has a relatively minimal set of completions, see the TODO in _comp_cmd_tmux__value(). I have code for more completions in varying states of readiness, but this commit is already pretty large. I'll make follow-up PR(s) for those. I'm willing to maintain this script. (And I'm hoping that the design mentioned above will make that easier.) Existing implementations that I'm aware of: * https://github.com/Bash-it/bash-it/blob/master/completion/available/tmux.completion.bash * https://github.com/Boruch-Baum/tmux_bash_completion * https://github.com/imomaliev/tmux-bash-completion * scop#81 * https://github.com/srsudar/tmux-completion
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.
Thank you for all your investigation and updates! LGTM. The following points can be addressed later if necessary:
- Move
_comp_cmd_tmux__log
tobash_completion
with the name_comp_log
- Update
_comp_compgen_help__get_help_lines
for more general use cases
Since the CI tests are failing (hopefully to be fixed by #1373):
(Ignore the |
From tmux/tmux#259 it doesn't look like the tmux maintainers were interested in having a bash completion script in the upstream repo. However, I added license headers to put these new files under either bash-completion's license or tmux's, in case they want it there in the future.
I'm aware of a handful of existing tmux bash completion scripts, below. As far as I can tell, they all hard-code a decent amount of tmux's available options, commands, etc. Some are also abandoned and out of date with more recent versions of tmux. Rather than base this code off of those, I decided to implement completion using tmux's own introspection commands as much as possible. Hopefully that will reduce the ongoing maintenance work and make it stay up to date with most tmux changes automatically.
This commit has a relatively minimal set of completions, see the TODO in _comp_cmd_tmux__value(). I have code for more completions in varying states of readiness, but this commit is already pretty large. I'll make follow-up PR(s) for those.
I'm willing to maintain this script. (And I'm hoping that the design mentioned above will make that easier.)
Existing implementations that I'm aware of: