Add validation for recurrence rules

[jonhester]

This takes care of #108 and will prevent things like #121 from happening. Throws error if not valid. Validation runs at the beginning of next invocation. @tejasmanohar and @santigimeno, what do you think?

[tejasmanohar]

+1 from me. Solves existing issues, I'm for it...

[santigimeno]

It would we great that validate also returned info about what field is invalid. Then we could throw and error with a message like this: Invalid RecurrenceRule. Field year is out of range

[jonhester]

@santigimeno Good idea. I'll add this.

[jonhester]

I'll update this later today.

[jonhester]

@tejasmanohar @santigimeno see if this is better

[santigimeno]

In the tests we should check that the error message is the expected.
Other than that LGTM

[jonhester]

@santigimeno Good call. I'll add the checks.

[tejasmanohar]

LGTM now. What about you, @santigimeno ?

[santigimeno]

@jonhester Why are you validating at the beginning of every invocation. Wouldn't just validate once at scheduling suffice?

[jonhester]

@santigimeno You are correct that this is not ideal, but since nextInvocation is exposed as part of the api, this seems to me to be the only way to ensure that RecurrenceRule will always get validated. Something that should definitely be accounted for in a refactor. Check the RecurrenceRule tests to see what I mean. Here is an example from the first recurrence rule test:

var rule = new schedule.RecurrenceRule();
      rule.second = 5;

      var next = rule.nextInvocationDate(base);

[santigimeno]

Why dont you add a validated property to RecurrenceRule so it only validates the object once?

[jonhester]

That could be done but could still not catch the error if someone does this:

var rule = new schedule.RecurrenceRule();
      rule.second = 5;

      var next = rule.nextInvocationDate(base);

      rule.second = 60

      next = rule.nextInvocationDate(base);

Which is abusing the API for sure, but it's possible.

[santigimeno]

Yeah, I see... still, I think we should not be validating in every invocation when scheduling a job. What about validating in the schedule function, then setting a _do_not_validate property to true. If that property is set we would not validate a RecurrenceRule. When using the RecurrenceRule API this property would not be set and would always be validated.

[santigimeno]

Still one could abuse the schedule API too, so I don't really know.

[jonhester]

@santigimeno Yeah, any way you cut it there is going to have to be a major refactor to fix this the right way. I think your idea is a good compromise to be able to go ahead and implement this. As we are working on the next major version we should make sure we account for this sort of thing.

[jonhester]

@santigimeno @tejasmanohar Should we hold this over until the next major version when we can validate in a way that's not a hack, or do you think it's worth implementing in the current version?

[tejasmanohar]

@jonhester My vote is validate in a way that's not a hack and wait for 1.0. What's yours + @santigimeno? We can just go w/ majority.

[santigimeno]

Yep. I would wait too

[tejasmanohar]

Then, we'll re-open this when the time comes?

[jonhester]

Yeah, there's no possible way to do it correctly now. But working on this made me a lot more familiar with some of the issues and limitations of the current version so I think it was still worth my time.

Sent from my iPhone

On Mar 21, 2015, at 7:01 PM, Tejas Manohar notifications@github.com wrote:

Closed #122.

—
Reply to this email directly or view it on GitHub.