tmux_cmd doesn't include kwargs

[askedrelic]

In tmux_cmd https://github.com/tony/libtmux/blob/189114cde7e125a482ab984749637fa44d66b284/libtmux/common.py#L132

the func receives args and kwargs but only args are added to form the cmd str
https://github.com/tony/libtmux/blob/189114cde7e125a482ab984749637fa44d66b284/libtmux/common.py#L176

Here is some IPDB output showing this, from tracking down tmux-python/tmuxp#184

# Reach tmux_cmd() with args and kwargs
> /usr/local/var/pyenv/versions/2.7.11/lib/python2.7/site-packages/libtmux/server.py(99)cmd()
     97                 raise ValueError('Server.colors must equal 88 or 256')
     98
---> 99         return tmux_cmd(*args, **kwargs)
    100
    101     def _list_sessions(self):

ipdb> args
self = <libtmux.server.Server object at 0x10fd11350>
args = [u'-2', 'set-environment', 'FOO', 'bar']
kwargs = {u'-t': u'$8'}

# Reach cmd and kwargs not included
ipdb>
> /usr/local/var/pyenv/versions/2.7.11/lib/python2.7/site-packages/libtmux/common.py(179)__init__()
    177         cmd = [str(c) for c in cmd]
    178
--> 179         self.cmd = cmd
    180
    181         try:

ipdb> cmd
['/usr/local/bin/tmux', '-2', 'set-environment', 'FOO', 'bar']

So this seems pretty big; any command using tmux_cmd would apply in the current session instead of the target session. Maybe it hasn't been noticed since a common use case isn't use tmuxp inside of an existing TMUX session?

[tony]

kwargs weren't intended to be supported like that in current / prior versions.

kwargs it only there to pick up tmux_search_paths and append_env_path. Other than that, any instances of kwargs entering tmux_cmd are a bug.

That said, I have no objection to adding it if it makes the experience more consistent / pleasant. However, I did want the signature on tmux_cmd to be similar to subprocess' signature.

When I written the tmux_cmd object, it was inspired by subprocess code in saltstack and pip (circa 2012/2013).

To play a bit of devil's advocate in terms of an intuitive, pythonic API: Do you know any other python subprocess-like (command) libraries that accept command line flags as kwargs?

[tony]

It does seem big that kwargs = {u'-t': u'$8'} is being passed into tmux_cmd.

Think we could smoosh that back into args?

[askedrelic]

Thanks for the context and quick reply Tony! So args should contain multiple values, as a proper list. I agree that kwargs seems more optional or should be used for internal flags.

For your 2nd comment, about changing the Session.cmd call? https://github.com/tony/libtmux/blob/master/libtmux/session.py#L75,L77

args = ['set-environment']
if '-t' not in args:
    args.extend(['-t', self.id])

# args = ['set-environment', '-t', 'id']

This should probably not break backwards compatibility too much; it seems better to explicitly include the session id.

[tony]

Yes that's on the right track

Even better, args += ['-t', self.id] (https://stackoverflow.com/questions/4176980/is-extend-faster-than)

I agree that kwargs seems more optional or should be used for internal flags.

It may be worth to put a comment in there that **kwargs was only put for those options.

As an aside (not for right now): When I wrote this, I wanted to have a signature for commands like tmux_cmd('list-panes', '-a'). After reflection, it sort of complicated the code. In the future, we may want to change the signature entirely to rely on a list variable instead of *args*. 1. because using *argsmeans we can't use named keyword arguments. and 2. passing in the command (e.g.['list-sessions']``) as a list is closer to subprocess' signatures. Which is probably going to be more intuitive to python programmers since that's the style used in the standard library.

[tony]

This should probably not break backwards compatibility too much; it seems better to explicitly include the session id.

The test suite is pretty solid. It tests against tmux itself and has enough coverage there's room to do internal refactors in the future (e.g. normalizing the tmux_cmd argument signature to be more consistent with subprocess')

[tony]

#65 is merged.

[askedrelic]

Closing since #65 was the outcome.

We did not include kwargs in tmux_cmd, but made a more targeted fix in Session.cmd()