ericpruitt / cronex

Heavily unit tested cron expression evaluation
MIT License
68 stars 12 forks source link

[Feature Request] Add seconds field #4

Open nagimov opened 7 years ago

nagimov commented 7 years ago

I'd like to add an extra seconds field into this. I'm pretty happy with the ability to use cron-like syntax for scheduling, but I'm running into an environment with required resolution of ~10 seconds. I'd like to keep using cron-like syntax while saving compatibility with old tasks at the same time, instead of running once a minute and sleeping within this minute (I think that's ugly). If I try to add this by myself, would you be interested in maintaining that code? What would you recommend doing (e.g. add seconds field to date_tuple and implement length checks, add an extra seconds = '*' kind of opt. argument, add a function wrapper, etc?).

ericpruitt commented 7 years ago

I am not opposed to accepting and maintaining (if you can even call it that; I haven't made any significant changes to this library in a very long time) contributions as long as they include unit tests and documentation. However, I will look into implementing this myself, and I'll get back to you within a few days.

nagimov commented 7 years ago

Much appreciated, thanks!

ericpruitt commented 7 years ago

I have implemented basic support for seconds, but I am curious if you also plan to use the arbitrary period triggers (%...) with the seconds field, and if you do plan to use them, do you need to be able to set a custom seconds filed in the epoch? Due to a poor design decision I made when I first created the library, I don't think I can support adding a seconds field to the epoch definition without potentially breaking existing users of this library. I am not opposed to doing that especially since I've wanted to refactor this library for quite some time, but if you don't need to set seconds in the epoch field, then I can push the change to the repository prior to doing that.

nagimov commented 7 years ago

I have few repeated tasks implemented through %..., but I can definitely survive without changing the epoch for them. I do see though why one would want to change seconds field within the epoch when using the library same way as I do (e.g. when running rare synchronized measurements - I'm in science lab/engineering environment). If you feel like refactoring, I would have no problems updating my schedule configs and code, since I'd like to stay on the latest version of the library anyways. I'm not in a rush either, so if pushing an extra release is anyhow troublesome - I'll happily wait for refactored revision. Thanks!

ericpruitt commented 7 years ago

I decided I would go ahead and start refactoring because I didn't want to kludge in epoch support for the seconds field. I am also adding support for years (like seconds, it will be off by default) since doing so won't require much, if any, year-specific logic. I will probably be done in the latter part of the upcoming week.

nagimov commented 7 years ago

Awesome thanks!

ericpruitt commented 7 years ago

The "refactor" ended up being a rewrite instead, so this has taken longer than originally anticipated.

The code is nearly feature complete, but remaining work includes:

Some of the new features that will be included are:

nagimov commented 7 years ago

Could you create help_needed-tagged issues for the remaining work if you'd like any help? Or are you planning to push to master only after you're done?

ericpruitt commented 7 years ago

My intent was to push to a secondary branch once I was ready for critique then make then update the master branch once I addressed (or dismissed) open issues, but if you'd like to help, I think the unit tests would be a good place to start because I don't expect the interface for any of the functions to change much if at all while I flesh out some of the remaining features. Would you be interested in doing that? If so, I will excise the work-in-progress functions and push an otherwise functional version of the rewrite to a separate branch.

nagimov commented 7 years ago

Sure sounds great

ericpruitt commented 7 years ago

I have pushed a branch called rewrite: https://github.com/ericpruitt/cronex/tree/rewrite . I am open to critique on code and documentation. In general, I want the unit tests for the rewritten module to have coverage comparable to that of the original. When testing a function to verify that it fails as expected, please use the most specific error the function can throw in a given context as opposed to checking for cronex.Error, for example. Unfortunately I don't have a written style guide, but I typically do something resembling PEP 8. You can take a look at https://github.com/ericpruitt/swadr/blob/master/src/tests.py to get an idea of how I tend to write unit tests nowadays. Please let me know if you have any questions.

ericpruitt commented 7 years ago

Forgot to mention -- I would like for the new code to work on Python 2.6+ and Python 3.2+. If there's a particularly compelling feature from a more recent version, I am open to reconsideration.

ericpruitt commented 7 years ago

In https://github.com/ericpruitt/cronex/issues/6, another user reported a bug in the unit tests for the repeater fields. This is a reminder that I need to do a better job of documenting how they trigger times are computed when I continue working on the rewrite.

ericpruitt commented 7 years ago

I plan to wrap up the rewrite in a few days. From my comment on the pull request with the tests:

The minimum Python version supported by the ddt test library is Python 2.7, and I want Cronex to support 2.6. I would prefer to have the unit test functions run through all test cases if one non-critical test fails. I will try to do this by deriving a custom unit test class from the one provided by the standard library, but if I can't create an interface I like, I may use ddt anyway since the suite doesn't need to be run by Cronex's users since the tests for the rewritten module will no longer rely on the host's clock.

I'll follow-up with the commit for you to take a look before I make it the official release assuming you're still interested in the library; I understand if you have come up with your own solution or otherwise don't have a use for Cronex.

nagimov commented 7 years ago

Great news thanks! I am still very interested in this. My current workaround includes stuff like sleep(30), so switching to full cronex implementation would certainly help.

ericpruitt commented 7 years ago

I've just pushed a completely functional Cronex with seconds support to the "rewrite" branch. Would you mind trying it out? Notable interface differences:

nagimov commented 7 years ago

Sure thanks I'll start migrating from original cronex next week

BinarSkugga commented 2 years ago

Any update on this ?

ericpruitt commented 2 years ago

@BinarSkugga, I don't personally use Cronex, and without some users willing to actually test the rewritten library, I don't have a lot of motivation to finish it although the code is mostly done at this point. If you and/or some other users were to try provide some feedback on the development branch, I would consider resuming work on the rewrite.