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

Parser options #75

Merged
merged 4 commits into from
Sep 18, 2016
Merged

Parser options #75

merged 4 commits into from
Sep 18, 2016

Conversation

lukescott
Copy link
Contributor

Parse options https://github.com/webconnex/cron/tree/parser-options
This adds Parser and NewParser with the ability to:

  • Select which fields are included. So you can exclude seconds, or any other field you don't want to use.

  • Set other options that might be useful, like making certain assumptions about the schedule.

    The Parse function now looks like this:

    var defaultParser = NewParser(
        Second | Minute | Hour | Dom | Month | DowOptinal | Descriptor,
    )
    
    // Parse returns a new crontab schedule representing the given spec.
    // It returns a descriptive error if the spec is not valid.
    //
    // It accepts
    //   - Full crontab specs, e.g. "* * * * * ?"
    //   - Descriptors, e.g. "@midnight", "@every 1h30m"
    func Parse(spec string) (_ Schedule, err error) {
        return defaultParser.Parse(spec)
    }

    This could be a resolution for Cron Format not the same as Cron Spec #58. All you would need to do is remove Second and it would be spec compliant. But for those who need it, they can create a new parser with the option.

    I'm not sure about Dow being optional and being "spec complaint" - but there are two options for this - Dow and DowOptional which are interchangeable - one makes it required, the other allows you to omit it.

(See #69)

@robfig
Copy link
Owner

robfig commented Sep 14, 2016

Does #73 meet your need? If not, can you describe the use case for creating a non-standard parser? It seems like having some kind of semi-standard syntax is a big benefit, and changing the set of stuff accepted would be fairly confusing if it didn't enable some significant new use case.

That being said, it does look cool to configure the parser like that!

@lukescott
Copy link
Contributor Author

We use it to omit the time options since we don't use them. We store a schedule associated with a subscription and scheduled the next date. We also added some non-standard options (like week of year) that was needed to accomplish a bi-weekly schedule.

You could keep the function in #73 by passing a different set of options, much like how I presented Parse above.

There are also a couple parser options that make some fields optional or required (like day of week).

I have some other features built on the parser options branch described in #69.

@lukescott
Copy link
Contributor Author

@robfig I've brought this up to date with master. ParseStandard looks like this now:

var standardParser = NewParser(
    Minute | Hour | Dom | Month | Dow | Descriptor,
)

// ..
func ParseStandard(standardSpec string) (Schedule, error) {
    return standardParser.Parse(standardSpec)
}

Which looks almost exactly like Parse, but lacks Second and replaces DowOptional with Dow.

Copy link
Owner

@robfig robfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just the two exported types should have godoc

// - Descriptors, e.g. "@midnight", "@every 1h30m"
func Parse(spec string) (Schedule, error) {
if spec[0] == '@' {
type Parser struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc please

if standardSpec[0] == '@' {
return parseDescriptor(standardSpec)
}
type ParseOption int
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc please

@lukescott
Copy link
Contributor Author

godocs added. Also fixed golint warning complaining about an else { return ... }.

Copy link
Owner

@robfig robfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!!

@robfig robfig merged commit 990e14e into robfig:master Sep 18, 2016
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.

None yet

2 participants