Skip to content
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

Add tmux completion #81

Closed
wants to merge 1 commit into from
Closed

Add tmux completion #81

wants to merge 1 commit into from

Conversation

srsudar
Copy link

@srsudar srsudar commented Oct 13, 2016

This is adapted from the following project:

https://github.com/srsudar/tmux-completion

I've tried to follow the contributing guidelines and similar PRs that add a file. My bash knowledge is sufficiently low that some of the contributing guidelines are over my head. If I'm missing any I apologize.

Do you have any documentation on the testing framework you're using so I can write tests? Or a pointer to a command with a minimal test that can serve as an example?

This is adapted from the following project:

https://github.com/srsudar/tmux-completion
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.

Bunch of comments posted. But first, most importantly, have you asked if the tmux project would be interested in including this with tmux itself?

Wrt testing, see doc/testing.txt

_init_completion || return

COMPREPLY=()
cur="${COMP_WORDS[COMP_CWORD]}"
Copy link
Owner

Choose a reason for hiding this comment

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

Setting COMPREPLY and cur here are not necessary


COMPREPLY=()
cur="${COMP_WORDS[COMP_CWORD]}"
onePrev="${COMP_WORDS[COMP_CWORD-1]}"
Copy link
Owner

Choose a reason for hiding this comment

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

How is onePrev different from the already existing $prev?

cur="${COMP_WORDS[COMP_CWORD]}"
onePrev="${COMP_WORDS[COMP_CWORD-1]}"

if [ "$COMP_CWORD" -ge 2 ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

Use $cword instead of $COMP_CWORD

windowCommands=("ls", "list-sessions")

prev="${COMP_WORDS[COMP_CWORD-2]}"
if [ "$prev" = "attach" ] || [ "$prev" = "attach-session" ] ; then
Copy link
Owner

Choose a reason for hiding this comment

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

Use words instead of COMP_WORDS and cword instead of COMP_CWORD. Also, rename this to something like preprev instead of overloading the usual definition of prev

if [ "$prev" = "attach" ] || [ "$prev" = "attach-session" ] ; then
if [ "$onePrev" = "-t" ] ; then
# Get a list of all session names.
# We're assuming this output is in the form:
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like there's a missing indent step within the if block here

fi

opts=" \
attach-session \
Copy link
Owner

Choose a reason for hiding this comment

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

Not a big fan of this long hardcoded list in the first place. Can't it be generated from somewhere?

If not, please indent the options, and the backslashes are unnecessary I think.

unlink-window \
up-pane"

COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
Copy link
Owner

Choose a reason for hiding this comment

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

$cur in double quotes


COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
return

Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary return and empty line

COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
return

}
Copy link
Owner

Choose a reason for hiding this comment

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

"} &&" here, see existing completions

@@ -0,0 +1,134 @@
# tmux(1) completion
Copy link
Owner

Choose a reason for hiding this comment

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

Missing -*- shell-script -*-, see existing completions

@srsudar
Copy link
Author

srsudar commented Oct 13, 2016

I hadn't even thought to check if tmux wanted to incorporate it. Looking at their repo I see that in their README they link to another repo that provides tmux-specific completion.

That pointer, along with this closed issue, make it sound like they aren't keen on including the completions as part of tmux. Would you prefer I ask explicitly before considering an eventual merge?

The completion script they link to looks like it attempts to do more things than this PR, but it is giving me errors when I try to use it. Since they recommend that one in their documentation, however, maybe accepting something like this would constitute a break from their semi-official completion? Would that preclude a merge?

@scop
Copy link
Owner

scop commented Oct 14, 2016

I suggest you work with upstream and the other tmux completion author first. I cannot comment on their wishes and practices as I'm not at all involved with those projects, but It's generally the better the closer to upstream packages completions are kept. That way for instance some things can be hardcoded in the completion (e.g. the option list: if the completion was shipped with tmux, it could be easily kept up to date with the tmux version it ships), which is not a luxury we have in bash-completion. Also, more importantly, a maintainer who is actively using the completion is a good thing to have, if not a necessity. I'm not a tmux user and there are no other active project members at the moment.

Closing at least for now, post a note here if things don't work out with upstream and the other recommended tmux completion author. But be prepare to make a case for inclusion here, I'm reluctant to add another completion in bash-completion over upstream tmux recommendations and for the reasons outlined above.

@scop scop closed this Oct 14, 2016
@srsudar
Copy link
Author

srsudar commented Oct 14, 2016

All of those seem like compelling points to me. No worries.

@imomaliev
Copy link

@srsudar Hi. I am current maintainer of tmux-bash-completion. What errors are you having? You can submit your issues here https://github.com/imomaliev/tmux-bash-completion/issues.

@srsudar
Copy link
Author

srsudar commented Oct 17, 2016

I decided not to open an issue because I only tried very briefly and I wasn't sure it was a problem with the project rather than my setup. I'll open an issue there just so you can respond and this doesn't linger as something unaddressed about the project. =)

@srsudar
Copy link
Author

srsudar commented Oct 17, 2016

Opened an issue.

@Boruch-Baum
Copy link

I just corresponded with the tmux developer, and confirmed that he has no interest in having his repository host a bash completion script. I recently wrote a very comprehensive one, and can offer it to this project. In reading the prior comments on this thread, I see a reference to another tmux-completion repository, so I looked there, and mine is way more comprehensive. If you're interested, I can make a PR.

@imomaliev
Copy link

@Boruch-Baum Hi, I am currently maintaining bash-completion script for tmux that is suggested in README for tmux https://github.com/imomaliev/tmux-bash-completion/ if you are interested in helping I could give you collaborator access

@Boruch-Baum
Copy link

@imomaliev : Hi! I don't mind sharing. My preference is for my code contribution to be widely distributed (as long as it works well). If the 'bash completion' accepts my work, then it automatically is included in the debian and other repositories and for anyone who has the package, it will 'just work' without having to search the internet, and manually maintain the file. tmux is a popular enough package to deserve inclusion here. If this doesn't work, I might try approaching the developers of the tmux plugin manager, which is also a debian package, and is very common for tmux user to use. If that doesn't work out, then I might try making it a tmux plug-in, which seems do-able since the mechanics are just a shell script - it just doesn't need a keybinding. Having a totally stand-alone repository for a single bash completion just doesn't seem right; imagine the hassle if that were the case for every package!

@scop
Copy link
Owner

scop commented Apr 21, 2019

I agree that generally hosting a completion with us here is the 2nd best option to having it upstream. So my suggestion for you is to:

  1. merge your work into a single file, perhaps in the @imomaliev repository so improvements while you work are available to the public until it lands here
  2. adjust code style, practices etc with the ones in this repository
  3. once all that is done, prepare another pull request here and I'll have a look
  4. volunteer to maintain the code here in bash-completion once it's in

@Boruch-Baum
Copy link

@scop @imomaliev @jackfletch : Jack Fletcher's comment finally got me to at least post my work, but I'm not available now for the back-and-forth that @scop is suggesting, so if someone else will carry the ball on it, great. See here.

@scop
Copy link
Owner

scop commented Apr 19, 2020

Any progress on this front? I'm slowly starting to use tmux myself, so would have more incentive to help out.

@Boruch-Baum
Copy link

Boruch-Baum commented Apr 19, 2020 via email

@imomaliev
Copy link

imomaliev commented Apr 19, 2020

I would gladly help with making this completion file https://github.com/imomaliev/tmux-bash-completion part of bash-completion upstream, or any other file chosen as base for this

@scop
Copy link
Owner

scop commented May 1, 2020

so you should be able to just drop it into your bash-completion folder and benefit from it.

Of course I won't do something like that, we have a project that hosts and ships completions here :) Personally I only use completions that are shipped along with the respective upstream tools, or ones we ship here, to maximize sharing the benefit to others.

@scop
Copy link
Owner

scop commented May 1, 2020

I would gladly help with making this completion file https://github.com/imomaliev/tmux-bash-completion part of bash-completion upstream, or any other file chosen as base for this

Since you're the only one who promised active help within these past two weeks, if you are available also to help out by maintaining the file within bash-completion in long term starting when it's in (I'll give you contributor access for that), you get to choose the base implementation. If you agree, feel free to post a PR and let's start working on it.

@scop
Copy link
Owner

scop commented May 1, 2020

The above comment doesn't BTW mean that others would not be welcome to help out too, and I'll gladly give collaborator access to people now and later who do good work and can show some, are willing to commit to the project's ways, and intend to be available to help out in longer term. I just want to have someone (or more) active here besides myself to drive the inciusion.

@imomaliev
Copy link

imomaliev commented Jul 28, 2020

If there are no new collaborators, on this issue, I think I will have time next week to start working on this

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.

4 participants