Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rules with interval 1 are not found with the occurrences_between after the first month #114

Closed
conzett opened this Issue Nov 6, 2012 · 5 comments

Comments

Projects
None yet
3 participants

conzett commented Nov 6, 2012

For example

new_schedule = Schedule.new(DateTime.now)
new_rule = Rule.daily.day(:friday)
new_schedule.add_recurrence_rule new_rule

Hash looks like this:

:start_date:
  :time: 2012-11-05 15:30:00.000000000 Z
  :zone: UTC
:rrules:
- :validations:
    :day:
    - 5
  :rule_type: IceCube::DailyRule
  :interval: 1
:exrules: []
:rtimes: []
:extimes: []

now we try to grab all instances of this schedule for the month it was created in:

occurrences = schedule.occurrences_between(DateTime.now.beginning_of_month,
DateTime.now.end_of_month)

This works, occurrences is populated with all occurrences.

However if we go one month ahead:

start = DateTime.now.change(month: month, year: year).beginning_of_month
finish = DateTime.now.change(month: month, year: year).end_of_month

occurrences = schedule.occurrences_between(start, finish)

occurrences is now empty. This is true for both weekly and daily rules.

The strange things is if the interval is 2 for either daily or weekly rules the occurrences_between function returns all the correct instances regardless of what month in the future you set.

Am I just misunderstanding the way this is supposed to work? Sorry for the lack of specs to prove this, I'm a little crunched for time.

Collaborator

avit commented Dec 2, 2012

start = DateTime.now.change(month: month, year: year).beginning_of_month
finish = DateTime.now.change(month: month, year: year).end_of_month

What are the values of month and year that you are changing here?

avit added a commit to avit/ice_cube that referenced this issue Dec 22, 2012

Collaborator

avit commented Dec 22, 2012

It looks like the problem is in TimeUtil not handling DateTime correctly because DateTime adds by days, not seconds. Note the difference between:

Time.local(2012, 12, 21) + 1   # => 2012-12-21 00:00:01
DateTime.new(2012, 12, 21) + 1 # => 2012-12-22 00:00:00

Hence, when we add interval * ONE_DAY we get the wrong jump.

Collaborator

avit commented Dec 28, 2012

I recently made it so that whatever value the schedule.start_time is set to, the occurrence methods would always return the same class (plain Time or ActiveSupport::TimeWithZone or DateTime). Doing this found & fixed some other problems related to DateTime but I'm now wondering whether DateTime should even be supported as a start_time value, and just put up a warning while converting it to Time, because:

  • DateTime behaves more Date-like than Time-like (it just happens to have time-of-day attributes but that's about it)
  • it has an inconsistent interface and lacks all the usual methods for calculating & querying
  • it's magnitudes slower

It would take some workarounds to properly support DateTime transparently:

  • Retain DateTime value in start_time
  • Internally convert to Time for all calculations
  • Convert occurrences back to start_time.class (DateTime) for output

I don't believe this has any benefit. Is there any valid use case for DateTime?

I vote to remove support by converting to_time with warning. Any thoughts, @seejohnrun?

(By the way, I posted an answer on StackOverflow that covers the different ruby time classes. I tried to show why DateTime is dumb.)

conzett commented Dec 28, 2012

@avit Sorry I've taken so long to respond, I did end up realizing that I needed to use time rather than datetime, and I agree that time makes more sense to just use time all around. I think this should just be called out prominently in the docs.

Owner

seejohnrun commented Dec 29, 2012

I like the idea of the conversion with a warning.
DateTime is the worst.

@avit avit closed this Feb 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment