tmux_cmd remove empty lines when it should not

[rockandska]

Hi,

I'm in the middle of writing a pytest-tmux plugin.
While writing unit/functional testing, I had an assertion test who failed for an obscure reason and didn't get why at first.
I finally discovered that capture-pane doesn't return what it really displayed.............

There's IMO a huge bug here.

Yes the empty lines should be cleaned but not all.

For example, a capture-pane of :

$ printf "  Hello World  .\n\n"
  Hello World  .

$

Should return :

$ printf "  Hello World  .\n\n"
  Hello World  .

$

and not

$ printf "  Hello World  .\n\n"
  Hello World  .
$

Any objection to clean empty lines starting from the last and stop until something is found ?

Regards,

[tony]

I'm in the middle of writing a pytest-tmux plugin.

We have an issue for this here: #381. Though not sure what your package will look like yet

There's IMO a huge bug here.

Thanks for the example! I am in favor of the output being accurate as possible, so we welcome the change!

Also to clarify, does this only for capture_pane()? The line linked refers to tmux_cmd() - so we'll need to see how CI reacts. Can you file a PR / tests / etc?

[tony]

@rockandska And also I can give you a priority review / quick turnaround - for this and #396 (comment).

We keep dev docs here: https://libtmux.git-pull.com/developing.html

[rockandska]

We have an issue for this here: #381. Though not sure what your package will look like yet

It is a WIP and will send you the repository when finished.
Not sure if it will fit all the needs people have, but we'll see ^^

Thanks for the example! I am in favor of the output being accurate as possible, so we welcome the change!

Great !

Also to clarify, does this only for capture_pane()? The line linked refers to tmux_cmd() - so we'll need to see how CI reacts.

I referenced tmux_cmd because it is what it is used on most cmd command.
So yeah, it will affect another commands too and we'll see how reacts the CI.

Can you file a PR / tests / etc?

Sure !

And also I can give you a priority review / quick turnaround - for this and #396 (comment).

I'll see if I can help on this, I'm not a skilled python developer

Regards

[tony]

I referenced tmux_cmd because it is what it is used on most cmd command.
So yeah, it will affect another commands too and we'll see how reacts the CI.

Let's do that

And even if it doesn't turn up anything useful, I have stuff in the pipeline to dramatically improve tmux_cmd for testability / reliability: #79 and not likely, but possibly SubprocessCommand from another project of mine.

I'll see if I can help on this, I'm not a skilled python developer

This is a good place to put your foot in the door since we use all the best modern tooling (pytest, flake8, black, isort, mypy, poetry). All the basic python open source practices that matters we pull in (and are open to suggestions to take in more)

If you install poetry, you may be able to clone libtmux and do tmuxp load ./path/to/libtmux and launch an environment (if you get any errors let me know)

[rockandska]

Ok, so the change is not without consequences on the CI (have the same locally) and need to take a closer look at the errors reported.

[tony]

@rockandska You should be able to test these locally as well

py.test tests/test_session.py::test_show_options tests/test_session.py::test_empty_session_option_returns_None tests/test_window.py::test_empty_window_option_returns_None

Retest on file watch

pytest-watch

py.test tests/test_session.py::test_show_options tests/test_session.py::test_empty_session_option_returns_None tests/test_window.py::test_empty_window_option_returns_None;
ptw . tests/test_session.py::test_show_options tests/test_session.py::test_empty_session_option_returns_None tests/test_window.py::test_empty_window_option_returns_None;

entr(1)

make watch_test test='tests/test_session.py::test_show_options tests/test_session.py::test_empty_session_option_returns_None tests/test_window.py::test_empty_window_option_returns_None'

[tony]

@rockandska Mirroring this

v0.15.0a0: GitHub release, git tag, PyPI package

pip install libtmux==0.15.0a0

[tony]

@rockandska Thank you for this contribution!

Also testing this release again tmuxp in tmux-python/tmuxp#805

Let me know how it works for fixing your scenario

[tony]

@rockandska And I should also say, well done on the test and the code changes.

[rockandska]

Thank you for this contribution!

You're welcome

Let me know how it works for fixing your scenario

I'm sure it will be perfect now that the capture_pane should return the exact output ^^

Any chance to see this backported in 0.8.x ?
Not sure if it is relevant these days, but I had in mind to have compatibility with 2.7 and 3.X

[tony]

backported in 0.8.x

I'm not sure how obtrusive this change would be. It's a fix, but what if the behavior broke existing working code at a patch level?

Checking now.

[tony]

@rockandska I pushed https://github.com/tmux-python/libtmux/tree/v0.8.x branch to GitHub

You can make a new branched on top of 0.8.x and make a PR against v0.8.x to backport #405 and we can see.

This is a harder request as we don't really support it. CI doesn't seem to work: https://github.com/tmux-python/libtmux/runs/8059651942?check_suite_focus=true

Let's say I were to take the effort to rejuvenate it: that's making heavy-ish modifications to a supposedly stable release.

[tony]

I can't guarantee backports to the Python 2.x branch.

Let's say also, that it doesn't pan out: It is okay to make a python 2.x forked package

Let's say there's also a high demand for python 2.x: We could entertain a "comeback branch", so long as someone's willing to maintain keeping the CI working. Hard, though. Poetry doesn't support python 2.7 anymore.