Skip to content
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

Modify retry() to take a callable and enforce the timeout #372

Merged
merged 9 commits into from
May 20, 2022

Conversation

categulario
Copy link
Contributor

@categulario categulario commented May 17, 2022

Previous implementation is equivalent to return True. This implementation, while API incompatible, is easy to use (I would modify the corresponding code in tmuxp myself) and enforces the timeout via an exception, which would help debugging since it provide the specific cause.

API docs updated accordingly. Environment variable preserved.

fixes #368 and addresses tmux-python/tmuxp#704 (comment) , it would also help with tmux-python/tmuxp#620

@CLAassistant
Copy link

CLAassistant commented May 17, 2022

CLA assistant check
All committers have signed the CLA.

@tony
Copy link
Member

tony commented May 17, 2022

@categulario poetry run flake8 and fix those, and it should let the rest of the tests run

@tony tony self-requested a review May 17, 2022 12:28
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #372 (8f555e5) into master (897bd24) will increase coverage by 0.41%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   88.13%   88.54%   +0.41%     
==========================================
  Files          15       16       +1     
  Lines        1500     1563      +63     
==========================================
+ Hits         1322     1384      +62     
- Misses        178      179       +1     
Impacted Files Coverage Δ
libtmux/test.py 51.76% <93.75%> (+8.90%) ⬆️
libtmux/exc.py 100.00% <100.00%> (ø)
tests/test_test.py 100.00% <100.00%> (ø)

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 897bd24...8f555e5. Read the comment docs.

libtmux/test.py Outdated Show resolved Hide resolved
@categulario
Copy link
Contributor Author

In the latest commits I even added three tests

libtmux/test.py Outdated Show resolved Hide resolved
libtmux/test.py Outdated Show resolved Hide resolved
libtmux/test.py Outdated Show resolved Hide resolved
libtmux/test.py Outdated Show resolved Hide resolved
libtmux/test.py Outdated Show resolved Hide resolved
@tony tony force-pushed the retry-redesign branch from b8c2569 to c041514 Compare May 20, 2022 09:32
@tony tony force-pushed the retry-redesign branch from c041514 to 036642e Compare May 20, 2022 09:39
@tony
Copy link
Member

tony commented May 20, 2022

@categulario

@tony tony force-pushed the retry-redesign branch 2 times, most recently from b0489b9 to 59f2723 Compare May 20, 2022 10:59
@tony tony force-pushed the retry-redesign branch from 59f2723 to 8f555e5 Compare May 20, 2022 11:00
@tony tony merged commit df17aaf into tmux-python:master May 20, 2022
@tony
Copy link
Member

tony commented May 20, 2022

@categulario merged in

v0.12.0a0: branch, pypi

I will need to drop python 3.7 and 3.8 in tmuxp since that's happening in libtmux before 0.12.0a0 can be added to tmuxp: tmux-python/tmuxp#763

@tony
Copy link
Member

tony commented May 20, 2022

i will update this

  • tmuxp v1.12.0a0: Drop 3.7 + 3.8 for libtmux compatibility: branch pypi
  • tmuxp v1.12.0a1: Update libtmux package for retry_until branch, pypi

@tony
Copy link
Member

tony commented May 20, 2022

tmux-python/tmuxp#776 has a concept of retry_until() with assert retry_until(..., raises=False)

@tony
Copy link
Member

tony commented May 20, 2022

@categulario As a next step you are welcome to make a PR moving retry() -> retry_until() in tmuxp.

if you do have something started let me know, if not i will get to it during the weekend. I have an example at tmux-python/tmuxp#776

@categulario
Copy link
Contributor Author

I will need to drop python 3.7 and 3.8 in tmuxp since that's happening in libtmux before 0.12.0a0 can be added to tmuxp: tmux-python/tmuxp#763

Is that necessary? python 3.7 is still maintained. and 3.8 comes in ubuntu 20.04. What exactly makes libtmux incompatible?

@categulario As a next step you are welcome to make a PR moving retry() -> retry_until() in tmuxp.

Sure thing!

@categulario categulario deleted the retry-redesign branch May 20, 2022 15:39
@tony
Copy link
Member

tony commented May 20, 2022

Is that necessary? python 3.7 is still maintained. and 3.8 comes in ubuntu 20.04. What exactly makes libtmux incompatible?

I thought about that.

I can look at reversing it, but I'd need to do it in libtmux first. See #374

@tony
Copy link
Member

tony commented May 20, 2022

I can expand #374 to cover 3.7 (this would need to happen during the weekend though)

@tony
Copy link
Member

tony commented May 20, 2022

@categulario libtmux and tmuxp support python 3.7 and 3.8 again ✅

Do you still want to move the tests over to retry_until() (if so do you have an ETA?) If not I can try to do it in the weekend

P.S. It's open source, none of us our paid, but the reason I hurry is the warnings due to the DeprecationWarning I added to retry()😄 :
image

@categulario
Copy link
Contributor Author

categulario commented May 21, 2022 via email

@tony
Copy link
Member

tony commented May 21, 2022

What is causing the warnings?
The warnings are from retry() being used in test_workspacebuilder.py (which should be resolved by moving to retry_until()

P.S. And the remainder - in python 3.10 - are from the user distutils.Version (which I'll get around to fixing soon)

I'm still interested in doing it. My eta would be the night of the Saturday which for you is the morning of the Saturday. I'm in UTC+8

That sounds good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

retry(seconds=...) broke
3 participants