Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Add seconds field #4

Open
nagimov opened this issue Mar 20, 2017 · 19 comments
Open

[Feature Request] Add seconds field #4

nagimov opened this issue Mar 20, 2017 · 19 comments

Comments

@nagimov
Copy link

nagimov commented Mar 20, 2017

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
Copy link
Owner

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
Copy link
Author

nagimov commented Mar 21, 2017

Much appreciated, thanks!

@ericpruitt
Copy link
Owner

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
Copy link
Author

nagimov commented Mar 24, 2017

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
Copy link
Owner

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
Copy link
Author

nagimov commented Mar 27, 2017

Awesome thanks!

@ericpruitt
Copy link
Owner

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.

@nagimov
Copy link
Author

nagimov commented Apr 4, 2017

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
Copy link
Owner

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
Copy link
Author

nagimov commented Apr 6, 2017

Sure sounds great

@ericpruitt
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

In #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
Copy link
Owner

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
Copy link
Author

nagimov commented Sep 16, 2017

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
Copy link
Owner

ericpruitt commented Sep 23, 2017

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.

@nagimov
Copy link
Author

nagimov commented Sep 25, 2017

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

@BinarSkugga
Copy link

Any update on this ?

@ericpruitt
Copy link
Owner

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

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

No branches or pull requests

3 participants