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

Make Server.has_session() use returncode #68

Merged
merged 1 commit into from Mar 11, 2018

Conversation

Projects
None yet
4 participants
@jlargentaye

jlargentaye commented Aug 17, 2017

has_session() would erroneously return true if tmux returned an unexpected
string, such as "no current session".

Instead of adding yet another string to the list to check against, use the
return code of the 'tmux has-session' command, which is documented to return 0
(Shell true) if targeted session exists, and return 1 (Shell false) in any
other case.

This is untested. While it passed selftest, so did the unpatched version.

John de Largentaye
Make Server.has_session() use returncode
has_session() would erroneously return true if tmux returned an unexpected
string, such as "no current session".

Instead of adding yet another string to the list to check against, use the
return code of the 'tmux has-session' command, which is documented to return 0
(Shell true) if targeted session exists, and return 1 (Shell false) in any
other case.
@jlargentaye

This comment has been minimized.

jlargentaye commented Aug 17, 2017

Adresses #67

@codecov-io

This comment has been minimized.

codecov-io commented Aug 17, 2017

Codecov Report

Merging #68 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   79.97%   80.14%   +0.16%     
==========================================
  Files           8        8              
  Lines         844      836       -8     
==========================================
- Hits          675      670       -5     
+ Misses        169      166       -3
Impacted Files Coverage Δ
libtmux/common.py 87% <100%> (+0.13%) ⬆️
libtmux/server.py 78.91% <100%> (+0.5%) ⬆️

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 d6df234...398350e. Read the comment docs.

@tony

This comment has been minimized.

Member

tony commented Aug 18, 2017

That simplification looks nice.

To play devil's advocate / pythonics:

What about keep the string comparisons and raising exceptions? e.g. https://libtmux.git-pull.com/en/latest/api.html#exceptions and https://github.com/tony/libtmux/blob/189114cde7e125a482ab984749637fa44d66b284/libtmux/common.py#L523

Older versions of tmux could still subclass a new command base exception like TmuxCommandException.

That way the front-end user could try: and except: and not have to look up old tmux versions to see what exactly went wrong (since the language changed between tmux versions, one of libtmux' features is abstracting out version differences).

@tony

This comment has been minimized.

Member

tony commented Feb 24, 2018

I need more time on this, because I need to research and assure consistent behavior across older tmux versions.

Does #67 still happen? Does it happen with all tmux versions, or only certain ones?

In #67 (comment) it says:

After doing a "tmux kill-server", I can't reproduce the issue. It's looks like previously the tmux server had stayed running after the last session finished.

Can you try again? Have you ruled out it being a partial/fnmatch of the session name?

@tony

This comment has been minimized.

Member

tony commented Mar 4, 2018

Sorry for the late response, I am having issues keeping up with the project. I'm seeking maintainers @ tmux-python/tmuxp#290. If you (or someone you know) may be interested in helping maintain libtmux or tmuxp, feel free to contact me by email or in the tmux-python/tmuxp#290 issue.

@Shados

This comment has been minimized.

Shados commented Mar 8, 2018

@tony the has-session command has used the exit code in this manner from right when the command was introduced (I checked), so you don't need to worry about older tmux versions.

#67 was caused by the relatively rare situation of a tmux session remaining alive with no sessions, likely due to still having an attached client (somehow) and thus causing an unexpected error message when calling has-session. You can replicate this trivially using the option recently added to tmux master in tmux/tmux@623f4b1 (which is how I ran into the issue, and why I'm commenting).

This PR would fix that issue, and also provide resistance against future changes to has-session's error messages. Which aren't entirely unlikely, given how many different variants you're already accounting for...

@tony tony merged commit 520610e into tmux-python:master Mar 11, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/patch 100% of diff hit (target 79.97%)
Details
codecov/project 80.14% (+0.16%) compared to d6df234
Details
@tony

This comment has been minimized.

Member

tony commented Mar 11, 2018

@Shados thank you for the extra info!

@jlargentaye Thanks for the patch!

Looks good so far: https://travis-ci.org/tmux-python/libtmux/builds/352060852

If things go well, we can have this shipped out to 0.8.0, hopefully today.

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