metomi/isodatetime

Possible bug or documentation bug in TimeRecurrence() get_next() and get_prev() methods

mo-jareddrayton opened this issue · 2 comments

TimeRecurrence() get_next() and get_prev() Behaviour

The docstrings for the get_next() and get_prev() state the following.

"Return the next timepoint after this timepoint in the recurrence series, or None."
"Return the previous timepoint before this timepoint in the recurrence series, or None."

However, this only seems to work properly when providing a TimePoint that itself is a valid time in the recurrence series.

If you pass a TimePoint that isn't a valid TimePoint within the series, the methods will return the given TimePoint with the cycle duration added/subtracted, rather than returning None which I think should be the expected behaviour based on the docstrings.

import metomi.isodatetime.parsers as parse

recurrence = parse.TimeRecurrenceParser().parse('R/2000/P4Y')
# Example where the given TimePoint does exist within the series.
date_time = parse.TimePointParser().parse('2000-01-01T00:00Z')
print(date_time)
print(recurrence.get_next(date_time))
print("Is this a valid TimePoint in this recurrence series?")
print(recurrence.get_is_valid(recurrence.get_next(date_time)))
2000-01-01T00:00:00Z
2004-01-01T00:00:00Z
Is this a valid TimePoint in this recurrence series?
True
# Example where the given TimePoint does not exist in the series.
date_time = parse.TimePointParser().parse('2001-01-01T00:00Z')
print(date_time)
print(recurrence.get_next(date_time))
print("Is this a valid TimePoint in this recurrence series?")
print(recurrence.get_is_valid(recurrence.get_next(date_time)))
2001-01-01T00:00:00Z
2005-01-01T00:00:00Z
Is this a valid TimePoint in this recurrence series?
False
# Example where the given TimePoint does not exist in the series.
date_time = parse.TimePointParser().parse('2007-01-01T00:00Z')
print(date_time)
print(recurrence.get_prev(date_time))
print("Is this a valid TimePoint in this recurrence series?")
print(recurrence.get_is_valid(recurrence.get_prev(date_time)))
2007-01-01T00:00:00Z
2003-01-01T00:00:00Z
Is this a valid TimePoint in this recurrence series?
False

I think changing self.get_is_in_bounds() to self.get_is_valid in

def get_is_valid(self, timepoint: "TimePoint") -> bool:
may be enough to fix this?

    def get_next(self, timepoint: "TimePoint") -> "TimePoint":
        """Return the next timepoint after this timepoint in the recurrence
        series, or None."""
        if self._repetitions == 1 or timepoint is None:
            return None
        next_timepoint = timepoint + self._duration
        if self._get_is_in_bounds(next_timepoint):
            return next_timepoint
        return None

    def get_prev(self, timepoint: "TimePoint") -> "TimePoint":
        """Return the previous timepoint before this timepoint in the
        recurrence series, or None."""
        if self._repetitions == 1 or timepoint is None:
            return None
        prev_timepoint = timepoint - self._duration
        if self._get_is_in_bounds(prev_timepoint):
            return prev_timepoint
        return None

Hi Jared, does TimeRecurrence().get_first_after() do what you are after?

I admit the get_next()/get_prev() docstrings may be misleading, and there is no get_first_before() method either

Hi Ronnie, yes thanks I've already been using TimeRecurrence().get_first_after() and it works very nicely. A get_first_before() would be a great addition too!