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

Fix roborock timers' next_schedule on repeated requests #1520

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

phil9909
Copy link
Contributor

@phil9909 phil9909 commented Sep 6, 2022

Every call to get_next of a croniter instance returns a different timestamp (because it's a cron iterator).

from croniter import croniter
from datetime import datetime

cron = croniter("* * * * *", datetime(2022, 1, 1))

cron.get_next(ret_type=datetime) # returns datetime.datetime(2022, 1, 1, 0, 1)
cron.get_next(ret_type=datetime) # returns datetime.datetime(2022, 1, 1, 0, 2)
cron.get_next(ret_type=datetime) # returns datetime.datetime(2022, 1, 1, 0, 3)

Because of this Timer.next_schedule will return different values each time. I don't think this is intended.

This PR will fix this behavior.

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2022

Codecov Report

Merging #1520 (a85e42f) into master (207a053) will increase coverage by 0.05%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #1520      +/-   ##
==========================================
+ Coverage   82.15%   82.21%   +0.05%     
==========================================
  Files         145      145              
  Lines       14161    14168       +7     
  Branches     3416     3418       +2     
==========================================
+ Hits        11634    11648      +14     
+ Misses       2302     2295       -7     
  Partials      225      225              
Impacted Files Coverage Δ
...o/integrations/vacuum/roborock/vacuumcontainers.py 80.71% <90.90%> (+3.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@phil9909 phil9909 force-pushed the fix-next-schedule branch 2 times, most recently from 4505a99 to 0864650 Compare September 6, 2022 16:20
This also fixes wrong type information in the Timer constructor
@phil9909
Copy link
Contributor Author

phil9909 commented Sep 6, 2022

Had to add a tests to make codecov happy.

While doing this I fixed a typing mistake in the init function of the Timer class (use pytz.BaseTzInfo instead of datetime.tzinfo which seems more accurate).

@rytilahti rytilahti added the bug label Sep 8, 2022
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, the 3.11-dev is sometimes flaky so let's ignore that for now. Thanks @phil9909! 👍

@rytilahti rytilahti changed the title fix: next_schedule returns different result each call Fix roborock timers' next_schedule on repeated requests Sep 14, 2022
@rytilahti rytilahti merged commit de3290a into rytilahti:master Sep 14, 2022
@phil9909 phil9909 deleted the fix-next-schedule branch September 14, 2022 22:17
@phil9909
Copy link
Contributor Author

Any plans to create a release soon?

@rytilahti
Copy link
Owner

Hi @phil9909, the current master has diverged so much that it's not a simple task to create a new release from it, but I might consider creating a new point release based on the latest release and some cherry-picked fixes if there is an urgent need that cannot wait?

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

Successfully merging this pull request may close these issues.

3 participants