Retain spaces in quoted strings in window options #37

kaushalmodi · 2017-04-26T21:52:22Z

With the below in tmux config:

setw -g pane-border-format " #P "

Before the fix:

('pane-border-format', '"', '#P', '"')

After the fix:

('pane-border-format', ' #P ')

Fixes tmux-python/tmuxp#239 With the below in tmux config: setw -g pane-border-format " #P " Before the fix: ('pane-border-format', '"', '#P', '"') After the fix: ('pane-border-format', ' #P ')

tony · 2017-04-27T22:16:04Z

I added a test you can work again, but shlex.split isn't storing the expected value.

_____________________________ test_set_show_window_options ______________________________

session = Session($1 libtmux_ziQLip)

    def test_set_show_window_options(session):
        """Set option then Window.show_window_options(key)."""
        window = session.new_window(window_name='test_window')

        window.set_window_option('main-pane-height', 20)
        assert window.show_window_options('main-pane-height') == 20

        window.set_window_option('pane-border-format', ' #P ')
>       assert window.show_window_options('pane-border-format') == [' #P ']
E       assert '"' == [' #P ']
E        +  where '"' = <bound method Window.show_window_options of Window(@2 2:test_window, Session($1 libtmux_ziQLip))>('pane-border-format')
E        +    where <bound method Window.show_window_options of Window(@2 2:test_window, Session($1 libtmux_ziQLip))> = Window(@2 2:test_window, Session($1 libtmux_ziQLip)).show_w
indow_options

tony · 2017-04-27T22:16:18Z

I also rebased it against the latest changes, and dropped python 2.6

tony · 2017-04-27T22:16:29Z

Can you give it a look @kaushalmodi?

kaushalmodi · 2017-04-27T22:18:25Z

Will have a look.. could probably mean that set_window_option has somehow to be updated to support spaces in quotes.

kaushalmodi · 2017-04-27T22:44:25Z

I am missing out something obvious here.. how do I run the test_set_show_window_options test locally?

I tried make test in the libtmux repo but the py.test referenced in the Makefile does not exist.

> make test
py.test
make: py.test: Command not found
make: *** [test] Error 127

tony · 2017-04-27T22:45:36Z

Did you try loading the project with tmuxp load path/to/libtmux? You can also manually invoke ./bootstrap_env.py and source into the virtual environment created.

Also you can do pip install requirements/test.txt

kaushalmodi · 2017-04-27T22:47:31Z

I got this from the .travis.yml and that ran the tests: python3 setup.py test... though fails a different test:

_____________________________________________________________ test_set_height _____________________________________________________________

session = Session($1 libtmux_8kct1jov)

    def test_set_height(session):
        window = session.new_window(window_name='test_set_height')
        window.split_window()
        pane1 = window.attached_pane
        pane1_height = pane1['pane_height']

        pane1.set_height(4)
        assert pane1['pane_height'] != pane1_height
>       assert int(pane1['pane_height']) == 4
E       AssertionError: assert 3 == 4
E        +  where 3 = int('3')

tests/test_pane.py:36: AssertionError
================================================== 1 failed, 77 passed in 16.67 seconds ===================================================

tony · 2017-04-27T22:48:23Z

Rerun it and see if it keeps happening. Could be a fluke.

kaushalmodi · 2017-04-27T22:50:23Z

Still getting the same error.. will tackle this tomorrow. Thanks for the guidance.

tony · 2017-04-27T22:50:59Z

Yes go ahead, and try to do it via tmuxp load path/to/libtmux and see what happens

kaushalmodi · 2017-04-28T12:44:12Z

I still see that test_set_height error. But now I see the test_set_show_window_options error too.

For my own record, I can run:
(on tcsh)

tmuxp load path/to/libtmux # That installs the virtual env in .venv
source .venv/bin/activate.csh
make test='tests/test_window.py::test_set_show_window_options'

kaushalmodi · 2017-04-28T14:30:32Z

I'll need your help to understand this problem.. here is my debug attempt: kaushalmodi@5e94ad8

Running

make test='tests/test_window.py::test_set_show_window_options'

gives

        window.set_window_option('pane-border-format', 'foo')
        assert window.show_window_options('pane-border-format') == '"foo"'

        window.set_window_option('pane-border-format', '#P')
        assert window.show_window_options('pane-border-format') == '"#P"'

        # window.set_window_option('pane-border-format', '#P')
        # assert window.show_window_options('pane-border-format') == ['#P']

        window.set_window_option('pane-border-format', ' #P ')
>       assert window.show_window_options('pane-border-format') == '" #P "'
E       assert '"' == '" #P "'
E         - "
E         + " #P "

tests/test_window.py:187: AssertionError
---------------------------------------------------------- Captured stdout call -----------------------------------------------------------
=== Setting Window Option ===
set_w_opt_dbg: "20"
cmd_dbg1: set-window-option (u'-t$1:2', u'main-pane-height', 20) {}
cmd_dbg2: set-window-option (u'-t$1:2', u'main-pane-height', 20) {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 main-pane-height False
cmd_dbg1: show-window-options (u'main-pane-height',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'main-pane-height') {}
=== Setting Window Option ===
set_w_opt_dbg: "foo"
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u'foo') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u'foo') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
=== Setting Window Option ===
set_w_opt_dbg: "#P"
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u'#P') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u'#P') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
=== Setting Window Option ===
set_w_opt_dbg: " #P "
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u' #P ') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u' #P ') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
======================================================== 1 failed in 0.47 seconds =========================================================

Also it probably needs to be

    assert window.show_window_options('pane-border-format') == '" #P "'

instead of

    assert window.show_window_options('pane-border-format') == [' #P ']

?

Update

Going down the rabbit hole.. kaushalmodi@6808f29

kaushalmodi · 2017-04-28T16:33:08Z

I am at a deadend now.. In the failing case (' #P '), something is calling Session._info, but I cannot trace back any further.

kaushalmodi@1e10452

tony · 2017-04-28T17:21:57Z

I think it's understable that "format" type options such as:

automatic-rename-format
pane-border-format
window-status-current-format
window-status-format
window-status-seperator

Are wrapped in quotes.

I think that these options should be unquoted on show, and re-quoted on setting.

The pane-border-format option was introduced in tmux 2.3, so do not run a test for that on older versions.

codecov-io · 2017-04-28T17:26:52Z

Codecov Report

Merging #37 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage    79.9%   79.92%   +0.02%     
==========================================
  Files           8        8              
  Lines         841      842       +1     
==========================================
+ Hits          672      673       +1     
  Misses        169      169

Impacted Files	Coverage Δ
libtmux/window.py	`83.01% <100%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 197c5f4...6a3a024. Read the comment docs.

kaushalmodi · 2017-04-28T17:44:21Z

I think that these options should be unquoted on show, and re-quoted on setting.

That is not needed after part 2 of the fix; now comparison is with just ' #P '.

And all the tests are now passing too!

.. though I still get this one failure. I don't get why Travis passes but it fails when I run make test.

_____________________________________________________________ test_set_height _____________________________________________________________

session = Session($1 libtmux_dBF9RL)

    def test_set_height(session):
        window = session.new_window(window_name='test_set_height')
        window.split_window()
        pane1 = window.attached_pane
        pane1_height = pane1['pane_height']

        pane1.set_height(4)
        assert pane1['pane_height'] != pane1_height
>       assert int(pane1['pane_height']) == 4
E       AssertionError: assert 3 == 4
E        +  where 3 = int('3')

tests/test_pane.py:36: AssertionError
================================================== 1 failed, 77 passed in 14.97 seconds ===================================================

tony · 2017-04-28T18:09:10Z

> assert int(pane1['pane_height']) == 4
E AssertionError: assert 3 == 4
E + where 3 = int('3')

@kaushalmodi Can I see your .tmux.conf? How big is your terminal window? Have you tried making the font smaller/terminal window bigger so there are more cells?

tony · 2017-04-28T18:12:58Z

tests/test_window.py

    window.set_window_option('main-pane-height', 40)
    assert window.show_window_options('main-pane-height') == 40
    assert window.show_window_options()['main-pane-height'] == 40

+    if has_gte_version('2.3'):


That was just added yesterday, looks like that function is already useful. 😄

kaushalmodi · 2017-04-28T18:18:21Z

Can I see your .tmux.conf?

here

I didn't realize that make test loaded the config too.

How big is your terminal window?

I think it is big enough

Have you tried making the font smaller/terminal window bigger so there are more cells?

No

I got that error to go away if I moved my ~.tmux.conf. I can incrementally comment my config to see what's causing that. Thanks.

kaushalmodi · 2017-04-28T18:21:41Z

It's caused when the pane border is enabled!

setw -g pane-border-status "bottom"

Update: And that too only when set to "bottom".. hmm

This works fine:

setw -g pane-border-status "top"

tony · 2017-04-28T18:22:47Z

Could you move your .tmux.conf then rerun the tests? No need to kill your main tmux server, since tests run on their own server.

Just trying to isolate if its a config issue or something else.

kaushalmodi · 2017-04-28T18:23:42Z

Could you move your .tmux.conf then rerun the tests?

That's what I did (see above).

No need to kill your main tmux server, since tests run on their own server.

Yup.

tony · 2017-04-28T18:25:43Z

I thought got that error to go away if I moved my ~.tmux.conf. I can incrementally comment my config to see what's causing that. Thanks.

Just read that, sorry. So did moving the tmux config make it test error go away?

kaushalmodi · 2017-04-28T18:27:28Z

So did moving the tmux config make it test error go away?

Yes. That fixed the error, and also changing the pane-border-status to "top" instead of "bottom".

tony · 2017-04-28T18:31:09Z

Okay that makes sense. So yes I think we can say it's a config issue, for now, and that this issue is good to merge.

LGTM

kaushalmodi · 2017-04-28T18:39:20Z

So yes I think we can say it's a config issue

Seems like a tmux issue.

If I am in a tmux window with panes split into top/bottom, display "#{pane_height}" increases by 1 when I switch pane-border-status from "top" to "bottom".

Thanks for merging this PR.

tony · 2017-04-28T19:05:25Z

If I am in a tmux window with panes split into top/bottom, display "#{pane_height}" increases by 1 when I switch pane-border-status from "top" to "bottom".

Understood.

Thanks for merging this PR.

Thanks for your time and your contribution. It is now on pypi as libtmux 0.7.1. You are credited in the CHANGES.

I thought there as an issue in tmuxp you had, but I can't find it. I still need to add a test on the tmuxp side.

kaushalmodi · 2017-04-28T19:08:32Z

I thought there as an issue in tmuxp you had, but I can't find it.

It is this same issue.. tmuxp freeze failed because I had set the pane border status to " #P " (tmux-python/tmuxp#239).

kaushalmodi mentioned this pull request Apr 26, 2017

tmux freeze fails tmux-python/tmuxp#239

Closed

Retain spaces in quoted strings in window options

8900f52

Fixes tmux-python/tmuxp#239 With the below in tmux config: setw -g pane-border-format " #P " Before the fix: ('pane-border-format', '"', '#P', '"') After the fix: ('pane-border-format', ' #P ')

tony force-pushed the fix-tmuxp-issue-239 branch from 2c98c07 to 8900f52 Compare April 27, 2017 21:48

add test for shlex.split on pane-border-format

da6ec0a

Part tmux-python#2 of shlex.split fix

6a3a024

The pane-border-format option was introduced in tmux 2.3, so do not run a test for that on older versions.

kaushalmodi force-pushed the fix-tmuxp-issue-239 branch from 6bea5d3 to 6a3a024 Compare April 28, 2017 17:26

tony reviewed Apr 28, 2017

View reviewed changes

tony merged commit 6a3a024 into tmux-python:master Apr 28, 2017

kaushalmodi deleted the fix-tmuxp-issue-239 branch April 28, 2017 18:42

Retain spaces in quoted strings in window options #37

Retain spaces in quoted strings in window options #37

Uh oh!

Conversation

kaushalmodi commented Apr 26, 2017

Uh oh!

tony commented Apr 27, 2017

Uh oh!

tony commented Apr 27, 2017

Uh oh!

tony commented Apr 27, 2017

Uh oh!

kaushalmodi commented Apr 27, 2017

Uh oh!

kaushalmodi commented Apr 27, 2017

Uh oh!

tony commented Apr 27, 2017

Uh oh!

kaushalmodi commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony commented Apr 27, 2017

Uh oh!

kaushalmodi commented Apr 27, 2017

Uh oh!

tony commented Apr 27, 2017

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushalmodi commented Apr 28, 2017

Uh oh!

tony commented Apr 28, 2017

Uh oh!

codecov-io commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony commented Apr 28, 2017

Uh oh!

tony Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony commented Apr 28, 2017

Uh oh!

kaushalmodi commented Apr 28, 2017

Uh oh!

tony commented Apr 28, 2017

Uh oh!

kaushalmodi commented Apr 28, 2017

Uh oh!

tony commented Apr 28, 2017

Uh oh!

kaushalmodi commented Apr 28, 2017

Uh oh!

tony commented Apr 28, 2017

Uh oh!

kaushalmodi commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kaushalmodi commented Apr 27, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading

codecov-io commented Apr 28, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading

kaushalmodi commented Apr 28, 2017 •

edited

Loading