cronex icon indicating copy to clipboard operation
cronex copied to clipboard

[Feature Request] Add seconds field

Open nagimov opened this issue 8 years ago • 19 comments

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?).

nagimov avatar Mar 20 '17 18:03 nagimov

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.

ericpruitt avatar Mar 21 '17 17:03 ericpruitt

Much appreciated, thanks!

nagimov avatar Mar 21 '17 17:03 nagimov

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.

ericpruitt avatar Mar 24 '17 01:03 ericpruitt

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!

nagimov avatar Mar 24 '17 04:03 nagimov

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.

ericpruitt avatar Mar 27 '17 00:03 ericpruitt

Awesome thanks!

nagimov avatar Mar 27 '17 01:03 nagimov

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:

  • Update (likely rewrite much of) the documentation.
    • Handling of "%" in the hour field has changed because I discovered a quirk in the original implementation. I'm not sure it qualifies as a "bug" because it may be another poor but intentional design decision I made years ago. I still need to give it some more thought to make sure I'm not overlooking something now.
  • Rewrite unit tests for new code; while rewriting cronex, I used the original code as a reference implementation so many my tests consisted of comparing the output of both versions on various expressions. They need to be rewritten because:
    • The errors in the rewritten library are more granular whereas I typically used common built-in exceptions like ValueError in the original code.
    • The only function in the common between the old and new code is "check_trigger." There are a couple of suspiciously similar functions, but they accept different inputs and handle certain errors differently.

Some of the new features that will be included are:

  • More support for Java Quartz expressions.
  • Method for getting future trigger times so you don't have to iterate over every minute. As an added bonus, using this method is faster than naive looping because the code that returns this information does not use a brute force approach.
  • Expressions for years and seconds are now supported but disabled / ignored by default.

ericpruitt avatar Apr 04 '17 03:04 ericpruitt

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?

nagimov avatar Apr 04 '17 16:04 nagimov

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.

ericpruitt avatar Apr 06 '17 07:04 ericpruitt

Sure sounds great

nagimov avatar Apr 06 '17 17:04 nagimov

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 avatar Apr 12 '17 04:04 ericpruitt

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 avatar Apr 12 '17 04:04 ericpruitt

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 avatar Sep 09 '17 22:09 ericpruitt

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.

ericpruitt avatar Sep 15 '17 21:09 ericpruitt

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.

nagimov avatar Sep 16 '17 01:09 nagimov

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

  • "check_trigger" no longer accepts a UTC offset. The "when" argument can be a time tuple, a (Y, M, D, HH, SS) tuple or Unix timestamp.
  • In the "__init__" method, the epoch is represented using a single argument. The epoch can be a Unix timestamp, time tuple, (Y, M, D) tuple (but "%" will not work in the hour, minute or seconds fields), a (Y, M, D, HH, SS) tuple or an Epoch instance.
  • The "__init__" method now accepts a "with_seconds" and "with_years" which can be used to toggle support for seconds / years in expressions.

ericpruitt avatar Sep 23 '17 20:09 ericpruitt

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

nagimov avatar Sep 25 '17 03:09 nagimov

Any update on this ?

BinarSkugga avatar May 05 '22 12:05 BinarSkugga

@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.

ericpruitt avatar May 06 '22 05:05 ericpruitt