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

Retry and timeout can be change by setting a class attribute #884

Merged
merged 4 commits into from
Dec 27, 2020

Conversation

titilambert
Copy link
Contributor

@titilambert titilambert commented Dec 5, 2020

No breaking change
With this, each class can have its own retry and timeout setting, ie:

class Vacuum(Device):
   retry =  5
   timeout = 10

   ...

@titilambert
Copy link
Contributor Author

@rytilahti
Here the change made for retry and timeout in viomi vacuum PR (#808)

miio/device.py Outdated Show resolved Hide resolved
@rytilahti
Copy link
Owner

Hi @titilambert, first of all, your efforts to provide tests is really appreciated! :-) I saw some commits on this PR in notifications, and wanted to mention that you can also run tests locally using pytest miio in the main source directory, or by calling tox to run all checks what the CI system does.

I noticed you are trying to check out if a parameter has been given (rightly so!), so I just wanted to mention that you may want to check if assert_called_with() works instead of manually call args list. I'm not that familiar with pytest/mock myself either, so I cannot definitely say if that works or not, but the name implies that it'd be useful in this case.

@titilambert
Copy link
Contributor Author

Hi @titilambert, first of all, your efforts to provide tests is really appreciated! :-) I saw some commits on this PR in notifications, and wanted to mention that you can also run tests locally using pytest miio in the main source directory, or by calling tox to run all checks what the CI system does.> Hi @titilambert, first of all, your efforts to provide tests is really appreciated! :-) I saw some commits on this PR in notifications, and wanted to mention that you can also run tests locally using pytest miio in the main source directory, or by calling tox to run all checks what the CI system does.

I noticed you are trying to check out if a parameter has been given (rightly so!), so I just wanted to mention that you may want to check if assert_called_with() works instead of manually call args list. I'm not that familiar with pytest/mock myself either, so I cannot definitely say if that works or not, but the name implies that it'd be useful in this case.

@rytilahti Thanks, I just found it (like 5 minutes before your comment :D ) I just squashed and pushed.
So I guess it's now ready for review :)

Thanks again

miio/tests/test_device.py Show resolved Hide resolved
miio/tests/test_device.py Show resolved Hide resolved
@titilambert
Copy link
Contributor Author

@rytilahti I just added a new test and made some adjustments

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

@rytilahti rytilahti merged commit 0f7ba91 into rytilahti:master Dec 27, 2020
@rytilahti rytilahti mentioned this pull request Mar 13, 2021
xvlady pushed a commit to xvlady/python-miio that referenced this pull request May 9, 2021
…ti#884)

* Retry and timeout can be change by setting a class attribute

* Fix PR comments

* Add tests

* Improve test for device retry and timeout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants