Skip to content

Run every minute at fixed rate, no drift.#116

Closed
dgrant wants to merge 2 commits intodbader:masterfrom
dgrant:run_every_minute_fixed_rate
Closed

Run every minute at fixed rate, no drift.#116
dgrant wants to merge 2 commits intodbader:masterfrom
dgrant:run_every_minute_fixed_rate

Conversation

@dgrant
Copy link

@dgrant dgrant commented Nov 27, 2016

Trying to extend .at() to apply to this case:

every().minute.at('::30').do(mock_job)

@dgrant
Copy link
Author

dgrant commented Nov 28, 2016

I've added some more validation as well on the argument to .at()

@PatrickMurray
Copy link

Hi, I'm really looking forward to this update. Is there an ETA when it will be branched out to PIP3?

@dbader
Copy link
Owner

dbader commented Dec 8, 2016

I like this – thanks for the PR @dgrant 😃 Let's aim for getting this into the next release (0.5.0) (cc @PatrickMurray).

In terms of next steps we'd need to:

  • update the docstring(s) to mention and explain the new syntax
  • add an FAQ item to help people discover this feature and reduce the amount of GitHub issues created ;-)
  • add yourself to AUTHORS.rst
  • update HISTORY.rst

I can take on some of that when I prep the release but if you could go ahead and make these changes here in the PR that would be super. Thanks!

@dbader dbader added this to the 0.5.0 milestone Dec 8, 2016
@dgrant
Copy link
Author

dgrant commented Dec 12, 2016

Just updating now.

One concern in some of these asserts will break old code using this module. Assuming people are using fixed versioning in their requirements.txt they'll be okay... but it's still not nice to break them. Perhaps we could provide warnings?

One example is that it used to be possible to say:

every.hour.at(03:30).do(job)

That doesn't really make sense though. What they really mean is:

every.hour.at(:30).do(job)

...or they didn't mean "hourly" here? they meant one-time instead? Anyways, previously the first one was allowed, but now I'm asserting.

@dgrant
Copy link
Author

dgrant commented Apr 17, 2017

Can this be merged?

@dbader
Copy link
Owner

dbader commented Jul 25, 2017

Hey there, sorry for remaining silent on this for so long. My concern here is that the _schedule_next_run functions is getting quite complicated at this point. It's quite the "beast" now and I'd like to keep the complexity of the schedule code base as low as possible.

I don't have a lot of time to spend on refactoring at the moment and I don't feel comfortable making lasting changes that create more work for me as the maintainer.

I'd be open for suggestions on how the scheduling code can be refactored.

@dgrant
Copy link
Author

dgrant commented Oct 20, 2017

@dbader That _schedule_next_run method is just barely over one-screen long for me on this PR, hardly a "beast" IMO. I just shortened it a bit :-)

@dgrant
Copy link
Author

dgrant commented Oct 31, 2017

I'm willing to help refactor things some more in the future, but I'd prefer that this gets merged in. I think think this makes things better and more complete, and of course all old tests pass. I don't see a reason to hold this up. Yes, I agree that some refactoring would be nice, but I don't think it makes sense to wait for some refactoring to happen. Any refactoring that touches this code will cause conflicts and extra unnecessary work. I also don't think this makes anything any worse.

@dgrant dgrant force-pushed the run_every_minute_fixed_rate branch 3 times, most recently from 1991b22 to 0c123bb Compare February 5, 2018 18:15
@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage increased (+0.9%) to 100.0% when pulling c99827a on dgrant:run_every_minute_fixed_rate into a877e73 on dbader:master.

@dgrant dgrant force-pushed the run_every_minute_fixed_rate branch from 015a5ad to b50fe28 Compare February 5, 2018 18:29
@dgrant
Copy link
Author

dgrant commented Feb 5, 2018

I've rebased and squashed this.

@dgrant dgrant force-pushed the run_every_minute_fixed_rate branch from b50fe28 to 1c7e832 Compare February 13, 2018 07:30
@forkandspoon
Copy link

forkandspoon commented Sep 12, 2018

Any update to this? Have been using dgrant's branch with success. It would be great to get it merged in.

Fixed flake errors and refactored code to be a bit cleaner.

added another test case for unit=minutes, and .at('::00') where the current time is *:*:30

Add test case for running at particular time in HH:MM:SS

Updated docstrings, readme, fax, and history, and added a bit more testing

fixed syntax/style issues

reformat docstring
@dgrant dgrant force-pushed the run_every_minute_fixed_rate branch from 1c7e832 to 463917d Compare September 12, 2018 19:45
@dgrant dgrant closed this Nov 21, 2020
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.

6 participants