Fix first weekly occurrences with non-Sunday week start #383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
avit
force-pushed
the
issues/360-wday_weekly_schedule_problem
branch
from
3b2f2cf to
e4f2d29
Compare
This was referenced Mar 20, 2017
avit
force-pushed
the
issues/360-wday_weekly_schedule_problem
branch
2 times, most recently
from
f251c57 to
c922ce1
Compare
|
Sorry for the delay - will have this back to you tonight! |
Instead of requiring a Schedule instance, rules only need a start time to align themselves to. This will allow the validate methods to be used in different contexts.
This option only applies to weekly rules and should not be listed everywhere to avoid confusion.
Simplify the Schedule and only handle realigning the start wday for weekly rules where it's needed.
avit
force-pushed
the
issues/360-wday_weekly_schedule_problem
branch
from
c922ce1 to
7fe1719
Compare
Realignment for start time doesn't apply to any rule types other than WeeklyRule. It also must be kept separate from the schedule's own start time because it's possible for it to move backwards to the start of the week.
avit
force-pushed
the
issues/360-wday_weekly_schedule_problem
branch
from
7fe1719 to
9eba907
Compare
|
Thanks, I'd just like a second pair of eyes on this since it's pretty big. (I hope it should make sense to review per-commit.) |
seejohnrun
reviewed
Mar 28, 2017
| @@ -25,8 +25,8 @@ def dst_adjust? | |||
| false | |||
| end | |||
|
|
|||
| def validate(step_time, schedule) | |||
| t0, t1 = schedule.start_time.to_i, step_time.to_i | |||
| def validate(step_time, start_time) | |||
| @@ -4,7 +4,7 @@ class HourlyRule < ValidatedRule | |||
|
|
|||
| include Validations::HourlyInterval | |||
|
|
|||
| def initialize(interval = 1, week_start = :sunday) | |||
| def initialize(interval = 1) | |||
There was a problem hiding this comment.
always thought these were a little clunky so I'm happy to see them go
|
|
||
| days = (step_time - start_time).to_i / ONE_DAY | ||
| interval = base_interval_validation.validate(step_time, start_time).to_i | ||
| min_wday = TimeUtil.normalize_wday(wday_validations.min_by(&:day).day, week_start) |
There was a problem hiding this comment.
I believe here this is the equivalent of map(&:day).min which may read nicer
There was a problem hiding this comment.
Did it this way because min_by takes one pass over the array and map.min takes two.
(However, this method is only called once per query, and it's a tiny array of 7 so it's not going to be significant either way.)
There was a problem hiding this comment.
Definitely - sounds good!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change corrects the wday offset to account for the week start option in weekly rules.
Some refactoring was done to avoid adding conditionals and more complexity into the Schedule class which was coordinating this:
week_startexcept for the Weekly rule that uses it. This is to avoid confusion about where this option is actually used.Much thanks to #360 and #284 for spotting the core issue here.