python-miio icon indicating copy to clipboard operation
python-miio copied to clipboard

Roborock timer: add find_next_schedule to replace next_schedule

Open phil9909 opened this issue 3 years ago • 2 comments

This is a follow up for: https://github.com/rytilahti/python-miio/pull/1520#discussion_r970052935

In PR #1520 we discussed, if next_schedule should return the next schedule relative to (A) the time of access or (B) the time of object creation. While alternative B has the advantage of "fresh" data, even if a reference to a Timer is kept for an extended period, it may be surprising to a user that a property of an object can change over time (without any external influence). In other words: I would be surprised, if in a single threaded program the following assertion could ever fail assert timer.next_schedule == timer.next_schedule. If an API behaves in a surprising way, it may lead to bugs as developers assume a certain behavior while using the API (see principle of least surprise).

As an alternative I suggested deprecating the next_schedule property and replace it with a method find_next_schedule. A method returning a different value based on the current time is far less surprising (IMHO).

To be honest: I'm not sure if it's worth the change. Maybe programs just should not keep the Timer objects in memory anyhow, as they might get changed by the user or another program at any point in time.

phil9909 avatar Sep 15 '22 21:09 phil9909

Codecov Report

Merging #1528 (ed7daf0) into master (818b891) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           master    #1528     +/-   ##
=========================================
  Coverage   82.19%   82.19%             
=========================================
  Files         145      145             
  Lines       14173    14173             
  Branches     3418     1607   -1811     
=========================================
  Hits        11650    11650             
  Misses       2298     2298             
  Partials      225      225             
Impacted Files Coverage Δ
...o/integrations/vacuum/roborock/vacuumcontainers.py 80.71% <100.00%> (ø)

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

codecov-commenter avatar Sep 15 '22 22:09 codecov-commenter

So, I don't think downstream users should cache the timer objects due to the reasons you already mentioned. The saved I/O is unlikely worth it and the use cases for it are rather uncommon, and if someone really needs that it is not so difficult to implement this on your own.

rytilahti avatar Sep 16 '22 16:09 rytilahti