8000 Retain spaces in quoted strings in window options by kaushalmodi · Pull Request #37 · tmux-python/libtmux · GitHub
[go: up one dir, main page]
Skip to content

Retain spaces in quoted strings in window options#37

Merged
tony merged 3 commits intotmux-python:masterfrom
kaushalmodi:fix-tmuxp-issue-239
Apr 28, 2017
Merged

Retain spaces in quoted strings in window options#37
tony merged 3 commits intotmux-python:masterfrom
kaushalmodi:fix-tmuxp-issue-239

Conversation

@kaushalmodi
Copy link
Contributor
@kaushalmodi kaushalmodi commented Apr 26, 2017

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 ')

@kaushalmodi kaushalmodi mentioned this pull request Apr 26, 2017
Closed
@kaushalmodi @tony
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 tony force-pushed the fix-tmuxp-issue-239 branch from 2c98c07 to 8900f52 Compare April 27, 2017 21:48
@tony
Copy link
Member
tony commented Apr 27, 2017

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
Copy link
Member
tony commented Apr 27, 2017

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

@tony
Copy link
Member
tony commented Apr 27, 2017

Can you give it a look @kaushalmodi?

@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 27, 2017

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

@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 27, 2017

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
Copy link
Member
tony commented Apr 27, 2017

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
Copy link
Contributor Author
kaushalmodi commented Apr 27, 2017 •
edited
Loading

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
Copy link
Member
tony commented Apr 27, 2017

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

@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 27, 2017

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

@tony
Copy link
Member
tony commented Apr 27, 2017

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

@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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

image

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017

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
Copy link
Member
tony commented Apr 28, 2017

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.

@kaushalmodi
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 kaushalmodi force-pushed the fix-tmuxp-issue-239 branch from 6bea5d3 to 6a3a024 Compare April 28, 2017 17:26
@codecov-io
Copy link
codecov-io commented Apr 28, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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
Copy link
Member
tony commented Apr 28, 2017

> 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
tony reviewed Apr 28, 2017
View reviewed changes
tests/test_window.py
assert window.show_window_options('main-pane-height') == 40
assert window.show_window_options()['main-pane-height'] == 40

if has_gte_version('2.3'):
Copy link
Member
@tony tony Apr 28, 2017

Choose a reason for hiding this comment

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

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

@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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
image

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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
Copy link
Member
tony commented Apr 28, 2017

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017

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
Copy link
Member
tony commented Apr 28, 2017

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017

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
Copy link
Member
tony commented Apr 28, 2017

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 :shipit:

@tony tony merged commit 6a3a024 into tmux-python:master Apr 28, 2017
@kaushalmodi
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017

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.

@kaushalmodi kaushalmodi deleted the fix-tmuxp-issue-239 branch April 28, 2017 18:42
@tony
Copy link
Member
tony commented Apr 28, 2017

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
Copy link
Contributor Author
kaushalmodi commented Apr 28, 2017 •
edited
Loading

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tony tony tony left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tmux freeze fails

3 participants

@kaushalmodi @tony @codecov-io
0