rossta / montrose

Recurring events library for Ruby. Enumerable recurrence objects and convenient chainable interface.
https://rossta.net/montrose/
MIT License
842 stars 52 forks source link

Starts doesn't behave as expected #42

Closed buffpojken closed 8 years ago

buffpojken commented 8 years ago

When using hash-notation to create a recurrence, starts doesn't seem to behave as expected, see attached example.

schedule = Montrose.r(every: :week, on: "tuesday", at: "5:00")

old_dates = schedule.take(5)

new_schedule = schedule.merge(starts: "2016-06-23")

new_dates = new_schedule.take(5)

puts old_dates == new_dates

third_schedule = Montrose.r(every: :week, on: "tuesday", at: "5:00", starts: "2016-06-23")

third_dates = third_schedule.take(5)

puts third_dates == old_dates

Shouldn't both the #merge and the new schedule with starts return dates after 2016-06-23?

rossta commented 8 years ago

Yes, you're right, I agree this is a bug. Currently, :at overrides the :starts option so trying to set :starts to something else isn't working as expected. The :at option does imply a :starts time when none is given but it should also take an existing :starts option into account.

buffpojken commented 8 years ago

Thanks for prompt reply!

Hmm, yes - I see what you mean.

I'll see if I have time to PR that tonight or tomorrow.

buffpojken commented 8 years ago

What I think would be a more obvious behaviour is the following:

:at should be used to set hour/minute in terms of when within a given day a given recurrence start but :starts should be used to set a starting date.

The combination of date + time within the scope of :starts doesn't really make sense since the relevancy of the time-part is highly contextual on the other set of rules within a given recurrence context, and no expressiveness is lost by separating those into two separate options, i.e. {:starts => "2016-12-31", :at => "5 am"} is equivalent with {:at => "2016-12-21 5 am"}.

Basically, what I'm proposing is that :at should ignore (and not set) any date parts while :starts should ignore (and not set) any time parts provided.

Thoughts?

buffpojken commented 8 years ago

Hmm, follow-up thought:

Given the behaviour described above - :starts as a term should maybe be renamed to :from or similar?

rossta commented 8 years ago

Thanks for your ideas, I'd have to give this some thought. Treating :starts as only the date parts could be plausible but I want to consider how this affects other use cases. This all touches on a semantics issue I noted in #6; :starts is playing dual roles as either the actual starting point of the recurrence and, in other cases, just a lower bound, like a :from or :after option would suggest.

rossta commented 8 years ago

@buffpojken I opened #43 as a way to reconcile that :starts acts like a date or a datetime depending on the context. Now when using :at, the recurrence will start at the first :at time of day on the date represented in the :starts timestamp. Let me know what you think, otherwise, I'll merge in the next day or so.

buffpojken commented 8 years ago

I think it looks splendid :)

I'll test it out in our project once it's merged but it sure looks fine to me!