[ANN] cronexpression, a cron expression parser

[rhill]

I needed a generic *standalone* cron expression parser, so here it is:

https://github.com/gorhill/cronexpression

Simplest way to use it, example:

    import "github.com/gorhill/cronexpression"
    import "time"
    ...
    nextTime := cronexpression.NextTime("0 0 * * 5L", time.Now())

Which would return "2013-09-27 00:00:00" (as of now).

If a cron expression needs to be repeatedly used with different time stamp, than it is more efficient to:

    cronexpr := cronexpression.NewCronExpression("0 0 * * 5L")
    ...
    nextTime := cronexpr.NextTime(fromTime)

I am sorta new to Go, any input is welcome.

[Carlos Castillo]

If you focus on good godoc documentation, you might find it easier to just put a link to the godoc.org site in the README.md for the API section (and possibly some of the others), since you will either be online (at github) and can click the link, or have the package downloaded (via go get), and can run godoc locally. This way your README.md is smaller, and easier to keep in sync with the documentation and state of the package.

The following suggestions are in order of recommendation. These recommendations are solely about the API (as seen in godoc - http://godoc.org/github.com/gorhill/cronexpression). 

Because of the package, your types & functions are namespaced, so you don't need as long type and function names if a short name would do.

Highly Recommended (used often in stdlib):

cronexpression.NewCronExpression -> cronexpression.New or .Parse

Recommended (since package name says what type of expression it is)

type cronexpression.CronExpression -> .Expression

Suggested (since return value implies what is returned):

expr|cronexpression.NextTime -> .Next

expr|cronexpression.NextTimeN -> .NextN

If the package level functions NextTime/NextTimeN are so straight forward and obvious it might make more sense to just leave them out (especially with the .New rename), and chain the constructor with a Next/NextN:

cronexpression.NextTime(str, t) -> cronexpression.New(str).Next(t)

cronexpression.NextTimeN(str,t,n) -> cronexpression.New(str).NextN(t,n)

Also cronexpression may be too long for a package name, you could use cronexpr instead. This may be an excessive change since it would involve re-naming the repo.

[rhill]

Thanks for your feedback, I converted all according to your suggestions, it all makes sense (eventually it will become natural for me to think the Go way). The only thing I didn't change as of now is the package name, 'cronexpression' => 'cronexpr', because I am not sure how I can change the entry over at godoc.org, otherwise I would do it right away, as it is the best time to do it, before the library starts being used.

[rhill]

Ah I see how to change the project name at http://godoc.org/-/about.

On Saturday, August 31, 2013 8:37:48 AM UTC-4, rhill wrote:

... because I am not sure how I can change the entry over at godoc.org, otherwise I would do it right away, as it is the best time to do it, before the library starts being used.

[Rodrigo Moraes]

I agree with all Carlos suggestions, so I think you made the right decision. :)

[arsh...@gmail.com]

Is it possible to add support for time zones. So that you can specify times in particular time zones. 

[Carlos Castillo]

Time for round 2, this one will look more at the code...

1. Your tests are broken, they import "cronexpr" when that isn't the correct path for the package, especially when people "go get" it. You can fix it by:
   
• Using the correct path: "github.com/gorhill/cronexpr". This is the simplest. I have done this on my local copy of the code so the tools don't complain.
• Changing the tests from "external" (defined in package cronexpr_test) to "internal" (defined in package cronexp), but that would require some rewrites in the body of the document. This has the advantage that you can move the package again without breaking the tests, and that you can test/bench your unexported methods directly.
2. Use tools to check the code:
• "go vet" - No problems (once tests are fixed)
• "golint ." (to install: go get github.com/golang/lint/golint)
• Doesn't like your documentation style
• Finds several occasions of you writing "for x,_ := range slice" when you can just write "for x := range slice"
• "deadcode" (install: go get github.com/remyoudompheng/go-misc/deadcode) - No unused private methods/functions/variables
• "errcheck github.com/gorhill/cronexpr" (install: go get github.com/kisielk/errcheck)
• complains that you are ignoring the errors returned on lines 93+94
• In both cases the functions called always seem to return nil, so you might want to change the return type (to nothing)
• errcheck is annoying in that you have to remember to write the whole path to you package
• "go test -cover" (coverage testing, only at tip, but should be in go1.2)
• Only 69.6% of the statements in cronexpr.go are run by the tests.
• It might be good to add some internal tests too
• attached is the result of running "go test -coverprofile cover.out -covermode atomic ; go tool cover -html cover.out -o cover.html"
• Red is untested code
• You don't test Example.NextN (big problem)
• You don't test failure states (eg: bad input should result in panic/error or test fails)
3. Add examples (see how http://golang.org/src/pkg/time/example_test.go provides examples for http://golang.org/pkg/time/#pkg-examples)
• Declared in *_test.go files (ie: not built as part of package)
• Examples should be in package cronexpr_test so that code displayed in godoc is properly namespaced.
• Examples are compiled by go test, so any incorrect examples (changed API) are errors
• Using the "// Output: blah" method (see: http://golang.org/src/pkg/time/example_test.go#L54) allows Examples to be tested for correct semantics/results, and such are mini-tests.
• Naming:
• ExampleFoo will be attached to function Foo in documentation (if it exists)
• ExampleExpression_Next will be attached to method Expression.Next in documentation
• Anything else should be attached to the package's documentation
4. You should try to minimize panic-ing at the API level, someone using your code will not appreciate a panic that will stop their app.
• Document which methods will panic, and why
• Try to return errors from your functions/methods instead of panic-ing
• Generally don't panic on information that could easily be from outside the program, or is hard to check outside your methods:
• An argument string, or other complex argument type where validity can't be checked trivially
• Anything coming from a user, or an uncontrolled file on disc
• You are then leaving it up to the programmer using your package to decide if the error should terminate the program
• Therefore, since the string to Parse is potentially user supplied data, and definitely hard for the programmer to check, Parse should probably return an error to the caller.
• Panic-ing is OK if the programmer calls a method with bad arguments in a way that could never be right:
• NextN panic-ing on a negative value for N is fine (but document it!)
• Programmers wishing to avoid the panic can then write the "if n <= 0" check themselves
• Similarly for calling Next, or NextN with a zero time, but your current behaviour of returning a zero time back (GIGO) is probably better (just be sure it's documented!)
• If you want to simplify the error handling and code inside your package, you could use panic/recover yourself eg:
• atoi's panic (to avoid returning an error, making the code simpler) should still be recover()'d by Parse, and then returned as an error, since it is probably the result of bad data in the string.
5. Suggestion: Although the code isn't performance-critical some benchmarks could still be useful.
• Many of the profiling tools built into go, and provided automatically to go test, (eg: cpu, memory, block, race) are more useful when run on benchmarks.
6. Suggestion: You could move some functions to their own code files to make your code easier to browse:
• Helper functions at bottom could go into util.go
  
• If you simplify the API functions/methods and types to the point where each is a few lines at most, then separating the API from the implementation gives you a single (or small number) of files to read if you don't have access to, or don't want to use, godoc.
• Again strictly a suggestion because in many ways it's a matter of personal style.

[Carlos Castillo]

I realize now that some of my "round 2" advice produces a little problem vis-a-vis my original advice. If cronexpr.Parse returns an error, then you can't easily use it in a chain to do "cronexpr.Parse(exprString).Next(timeVal)", since Parse will return 2 values and make the expression invalid.

There are many ways to resolve this, here are two:

1. Put back your package-level "Next" and "NextN" functions. This is simple enough.
2. Create an alternative Parse function, called MustParse() which only returns the *Expression, but panics in all situations that Parse would return an error.
• The advantage is that it allows programmers using the package to differentiate the situations where the expression is uncertain (user data), vs. when it's under the programmer's control and should always be correct.
• This is what the regexp package does with it's MustCompile/MustCompilePOSIX functions (http://golang.org/pkg/regexp/#MustCompile)

[Carlos Castillo]

AFAIK, it already supports timezones:

• Whatever timezone of the time you past to Next, the output will be in as well.
• When you ask for (say) 5:13AM, it computes it for the timezone given, so thus the output will be 5:13AM in the timezone.
• If you want to compute time in another timezone, then you can use the Time.In method to change the timezone of the input timevalue, and the output timevalue will match it properly.

If this isn't what you're looking for, can you then explain what you are?

[rhill]

Thanks very much for your valuable input. I've gone through and reworked the code according to (my understanding of) your suggestions.

[shijuk...@gmail.com]

I Have a created a cron expression which will run at every 3 minutes interval (3.05 AM, 3.08 AM,3.11 AM etc) from Monday to Friday and once it reach to every hour(1 AM, 2 AM, 3 AM ,4 Am etc)the interval is became 5 minutes (3.5 AM , 3.8 AM.... 3.58 AM,4.5 AM) again it will run as the normal interval 3 minutes. but how can i make this expression to tell that the five minutes interval should not happen at every hour it should happen at 8AM , 11AM , 2PM an 5 PM and the remaining schedule should be 3 minutes interval.  

Please see my current cron expression here

0 5/3 00-02,05-17,19-23 ? * MON-FRI *

your help is really appreciated.