search

kiorky / croniter

MIT License
410 stars 40 forks source link

Fix #12: `set_current(force=True)` by default #13

Closed agateblue closed 2 years ago

agateblue commented 2 years ago

@kiorky I've started a PR that includes a simple test case for this issue and a proposed solution

The test case fails on master, passes on v1.2.0. As far as I can tell, during https://github.com/kiorky/croniter/commit/1ea781aa1936214088fa6451c5d62537045869dd, the set_current() method signature slightly changed, including an optional force parameter with a default to False.

When calling set_current(start_time) before get_next(), which Procrastinate does, without specifying force, the set_current() call is basically a no-op, which causes the change of behaviour.

My suggested fix is to switch the default value for the force argument to True. This fixes the test, restores backward compatibility, and also makes more sense from a UX point of view IMHO: if you're calling set_current() with a non null start_time, you most likely do want it to be used :)

kiorky commented 2 years ago

Thx a lot, i added some more explicit calls to set_current(force=?) as we toggled the default.

The force= was just introduced anyway, so no chance it's used in the wild moreover when it's more an internal API switch that something users will play around.

agateblue commented 2 years ago

Yeah that's what I thought too, I wouldn't be surprised to be the only one encountering this :smile: