Prevent illogical rule combinations #420
Conversation
Limit each example to 1 second in case of infinite loops. Can be redefined with timeout: 0 on example metadata.
Reject illogical combinations of rule types with intervals that would never align with any occurrences.
When time parts (hour_of_day, minute_of_hour, second_of_minute) are specified on recurrence rules, the rules would jump over valid occurrences unless they were realigned.
Some combinations of rule parts do not apply. Rather than exposing all possible validations on all recurrence rule types, limit to ones that produce results.
| schedule.add_recurrence_rule Rule.minutely(60).day(4).hour_of_day(14, 15, 16).minute_of_hour(0) | ||
| expect(schedule.occurring_at?(Time.local(2011, 11, 17, 15, 30))).to be_falsey | ||
| end | ||
|
|
There was a problem hiding this comment.
This spec originally fixed an infinite loop by skipping over valid occurrences. It should actually occur on the time in this example so I removed it.
seejohnrun
left a comment
seejohnrun
left a comment
There was a problem hiding this comment.
Going to need a minor version bump on this I think 👍
There's a lot to unpack here, but this looks good - thank you for taking it on!
|
|
||
| include Validations::ScheduleLock | ||
|
|
||
| include Validations::HourOfDay |
There was a problem hiding this comment.
👍 to splitting these out so we can only have them where they make sense
lib/ice_cube/rules/daily_rule.rb
Outdated
| return unless freq == :wday || freq == :day | ||
| return unless @validations[:interval] | ||
|
|
||
| interval_validation = @validations[:interval].first |
There was a problem hiding this comment.
Maybe worth moving this interval-checking into a util that we can use in both places
There was a problem hiding this comment.
Yes, I'm not 100% happy with some of the duplication & super-calling either. I'll see what I can do.
lib/ice_cube/validated_rule.rb
Outdated
| end | ||
|
|
||
| def other_fixed_value_validations | ||
| @validations.values.flatten.select { |v| |
There was a problem hiding this comment.
nit: do / end on multi-line blocks
There was a problem hiding this comment.
Ok, sure. 😄
I've always been more partial to Jim Weirich's semantic brace style, and I try to sneak it in when I can:
- Use { } for blocks that return values (functional)
- Use do / end for blocks that are executed for side effects (procedural)
It actually turns out that doing it this way, it works out that very often it's the same as the single-line vs. multi-line style, because of the kinds of things that go into procedural vs. functional blocks.
There was a problem hiding this comment.
Okay I can be down with that - I like the idea :)
| def interval(interval) | ||
| @interval = normalized_interval(interval) | ||
| interval = normalized_interval(interval) | ||
| verify_alignment(interval, :wday, :interval) { |error| raise error } |
There was a problem hiding this comment.
Why do we yield this error up instead of just raiseing it?
There was a problem hiding this comment.
This is so that it gives a more relevant trace, since it appears from the called interval public method interface, instead of raising from somewhere deeper down.
Move methods related to input verification into their own object.
2 participants
Fixes #416
Fixes #406