implement the skip option #522

yoch · 2017-11-14T11:48:27Z

Try to implement the SKIP option (which solves issue #149).

To be compatible with the RFC-7529 (#285), I also added RSCALE element because skip depends on it, but without the support of non-gregorian calendars.

Edit from pganssle:

Just for tracking, here's a to-do-list of in-scope cases for skip to be implemented in.

The start date of the recurrence has a day-of-month value greater
than the smallest day-of-month value for any month in any year in
the specified calendar system.
A "BYMONTHDAY" element in an "RRULE" has a day-of-month value
greater than the smallest day-of-month value for any month in any
year in the specified calendar system.
A combination of a "BYMONTHDAY" element and a "BYMONTH" element
in an "RRULE" has a value corresponding to a leap day in the
specified calendar system.
A "BYYEARDAY" element in an "RRULE" has an absolute value greater
than the smallest number of days in any year in the specified
calendar system.
A "BYWEEKNO" element in an "RRULE" has an absolute value greater
than the smallest number of weeks in any year in the specified
calendar system.

yoch · 2017-11-14T11:57:58Z

I haven't created new tests yet, but here some examples:

>>> from dateutil.rrule import *
>>> from datetime import datetime
>>> from pprint import pprint
>>> start_date = datetime(2014, 12, 31) 
>>> rr=rrule(freq=MONTHLY, count=4, dtstart=start_date); print(rr); pprint(list(rr))
DTSTART:20141231T000000
RRULE:FREQ=MONTHLY;COUNT=4
[datetime.datetime(2014, 12, 31, 0, 0),
 datetime.datetime(2015, 1, 31, 0, 0),
 datetime.datetime(2015, 3, 31, 0, 0),
 datetime.datetime(2015, 5, 31, 0, 0)]
>>> rr=rrule(freq=MONTHLY, count=4, dtstart=start_date, rscale='GREGORIAN', skip='BACKWARD'); print(rr); pprint(list(rr))
DTSTART:20141231T000000
RRULE:FREQ=MONTHLY;COUNT=4;RSCALE=GREGORIAN;SKIP=BACKWARD
[datetime.datetime(2014, 12, 31, 0, 0),
 datetime.datetime(2015, 1, 31, 0, 0),
 datetime.datetime(2015, 2, 28, 0, 0),
 datetime.datetime(2015, 3, 31, 0, 0)]
>>> rr=rrule(freq=MONTHLY, count=4, dtstart=start_date, rscale='GREGORIAN', skip='FORWARD'); print(rr); pprint(list(rr))
DTSTART:20141231T000000
RRULE:FREQ=MONTHLY;COUNT=4;RSCALE=GREGORIAN;SKIP=FORWARD
[datetime.datetime(2014, 12, 31, 0, 0),
 datetime.datetime(2015, 1, 31, 0, 0),
 datetime.datetime(2015, 3, 1, 0, 0),
 datetime.datetime(2015, 3, 31, 0, 0)]
>>> rr=rrule(freq=MONTHLY, count=4, dtstart=start_date, skip='FORWARD'); print(rr); pprint(list(rr))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/cygdrive/d/Projets/dateutil/dateutil/rrule.py", line 676, in __init__
    assert skip is None, 'SKIP is not allowed without RESCALE'
AssertionError: SKIP is not allowed without RESCALE

yoch · 2017-11-14T12:17:26Z

Important remark: this SKIP doesn't cover all the possibles cases.

The RFC give these cases :

The start date of the recurrence is a leap day in the specified
calendar system.

The start date of the recurrence is in a leap month in the
specified calendar system.

The start date of the recurrence has a day-of-month value greater
than the smallest day-of-month value for any month in any year in
the specified calendar system.

A "BYMONTHDAY" element in an "RRULE" has a day-of-month value
greater than the smallest day-of-month value for any month in any
year in the specified calendar system.

A "BYMONTH" element in an "RRULE" has a value corresponding to a
leap month in the specified calendar system.

A combination of a "BYMONTHDAY" element and a "BYMONTH" element
in an "RRULE" has a value corresponding to a leap day in the
specified calendar system.

A "BYYEARDAY" element in an "RRULE" has an absolute value greater
than the smallest number of days in any year in the specified
calendar system.

A "BYWEEKNO" element in an "RRULE" has an absolute value greater
than the smallest number of weeks in any year in the specified
calendar system.

cannot occurs because dtstart must be valid;
and 5. cannot occurs in Gregorian calendar (no leap month);
is adressed by this PR;
, 6. , 7. and 8. need to be adressed.

pganssle

First off, thank you very much for this contribution, it is much appreciated and a much-requested feature.

I have made a few specific notes in-line on the implementation, but with regards to rscale, I'm a bit torn as to whether that even needs to be included. From an API perspective, it seems easiest to just have None be equivalent to 'GREGORIAN' and enforce that rscale and skip must both be set when parsing with rrulestr and when emitting rrulestr. That would make the Python interface more convenient but less API-compliant, but I think we're better off with convenience here.

The biggest blocker I see on this PR, though is the lack of tests. Please add tests for this new feature that cover all new lines. For each new rrule test, also please add a _rrulestr_reverse_test that your rrule (I'd like to fix this so it's more automatic, but I don't think that's going to happen for this PR).

Thanks!

P.S. Not a blocker at all (I can do it myself), but if you can rebase against master before making changes that would be ideal).

pganssle · 2017-11-14T14:22:36Z

dateutil/rrule.py

@@ -658,6 +659,23 @@ def __init__(self, freq, dtstart=None,
            self._bysecond = tuple(sorted(self._bysecond))
            self._original_rule['bysecond'] = self._bysecond

+        # rscale and optional skip values
+        if rscale is not None:
+            assert rscale == 'GREGORIAN', 'unsupported RSCALE'


First off, I'm not comfortable with using assertions for error handling. This should raise an error of some sort unless you're just asserting some invariant of the code (consider that assertions are turned off if Python is run with python -O.

I'm of two minds about how to implement this. One thing we could do is have a fixed list of calendar types that are considered valid, even if unsupported, adding a class attribute like this:

# List taken from https://www.unicode.org/repos/cldr/tags/latest/common/bcp47/calendar.xml VALID_RSCALES = {'GREGORIAN', 'GREGORY', 'BUDDHIST', 'CHINESE', 'COPTIC', 'DANGI', ... }

Then change this check to:

if rscale is not None: rscale = rscale.upper() if rscale in self.VALID_RSCALES: if rscale not in {'GREGORY', 'GREGORIAN'}: raise NotImplementedError('Non-gregorian calendar ' '{} not supported.'.format(rscale)) else: raise ValueError('Invalid RSCALE component: {}'.format(rscale))

Note that I also support lower and mixed-case calendar names there with the upper call. That said, an alternative to using string comparison at all is to simply have an enum-type class that enumerates the supported calendars. That seems like it might have some advantages here, but depending on the implementation it might make it harder for people to extend dateutil to support other calendars. String comparison seems like a good start.

pganssle · 2017-11-14T14:24:54Z

dateutil/rrule.py

+            else:
+                self._skip = 'OMIT'
+
+            assert self._skip in ['OMIT', 'BACKWARD', 'FORWARD'], 'invalid SKIP'


I think in this case the possible values for SKIP are probably bounded such that an enum-like structure (can't use real enums because we still support Python 2.7) would be better here. In that case, I'd find an assertion of type or value more acceptable since it's probably a superfluous check that can be omitted in optimized code.

pganssle · 2017-11-14T14:31:06Z

dateutil/rrule.py

@@ -678,6 +696,14 @@ def __str__(self):
        dateutil-specific extension BYEASTER.
        """

+        def _fmt(obj):


What is the purpose of this function? The other two branches you added are never hit.

Without this function, strings are formated uncorrectly by: parts.append(partfmt.format(name=name, vals=(','.join(str(v) for v in value)))). (The second branch never hit because I didn't add tests, but the last one may not be used.)

pganssle · 2017-11-14T15:19:23Z

dateutil/rrule.py

+                if day > daysinmonth:
+                    if self._skip == 'BACKWARD':
+                        day = daysinmonth
+                        leap = True


Why is this called leap? Shouldn't it be called skipped? Or shifted or something?

pganssle · 2017-11-14T15:26:22Z

@yoch Are you intending to implement 4, 6, 7 and 8 in a separate PR, or is this a "WIP" PR that you plan on updating to include those cases, or is this just a partial implementation and you're not planning on completely implementing the skip option?

I think we can include a full implementation of skip (minus 1, 2 and 5) in dateutil 2.7.0, otherwise we can defer merging this until after the 2.7.0 release so that the full implementation can be ready for the 2.8.0 release.

pganssle · 2017-11-14T16:37:59Z

dateutil/rrule.py

+                        day = 1
+                        month += 1
+                        if month == 13:
+                            month = 1


I believe it's not actually possible to hit this case in the Gregorian calendar when only allowing skip condition 3, since December always has 31 days.

I'd have to check the RFC rules to see if, with the new extension, you can do something like bymonthday=32 and have it fall forward. I suspect that this is unnecessary, since that should be equivalent to a bymonthday=1 rule.

If this case is forbidden by the RFC (or if this set of conditions is only ever reached for end-of-month skip conditions), just drop this whole if condition and replace it with this:

# In the Gregorian calendar, December has 31 days, so we should never find # a skip condition that would roll over to the next year assert month <= 12, "Skip forward logic resulted in anomalous month"

dateutil/test/test_rrule.py

yoch · 2017-11-14T16:53:53Z

@pganssle With regard to the missing points, it turns out that it's more complex and probably need to modify the existing code more deeply, so I plan to make a separate PR, if I can.

pganssle · 2017-11-14T17:00:11Z

With regard to the missing points, it turns out that it's more complex and probably need to modify the existing code more deeply, so I plan to make a separate PR, if I can.

Feel free to continue using this branch or to create a new PR based off of this one. I don't want to have to release with a half-done feature, so until the feature is more or less complete I wouldn't want to merge it until after the 2.7.0 release, which likely won't be for a month or so. If you finish the other parts I can merge it as part of the 2.7.0 release.

yoch · 2017-11-15T20:27:46Z

Cases 4 and 6 are now correctly handled (no idea why appveyor fails, but the errors doesn't seems related to the commit).

pganssle · 2017-11-16T00:33:25Z

dateutil/test/test_rrule.py

@@ -431,6 +431,28 @@ def testMonthlySkipForward(self):
                          datetime(2015, 3, 1, 0, 0),
                          datetime(2015, 3, 31, 0, 0)])

+    def testMonthlyByMonthSkipBackward(self):


Does the new stuff cover the bymonthday=30 case as well? Can you add tests for that?

Not yet, in fact this was a fix for BYMONTH. But I've fixed it for BYMONTHDAY now.

Now I don't know how to deal with the negatives cases (do we also interpret FORWARD and BACKWARD in negative sense or not).

pganssle · 2017-11-16T14:25:00Z

Appveyor failure was due to pytest-dev/py#169, seems fixed now.

pganssle · 2017-11-16T14:36:16Z

@yoch I edited your first post to add a "to-do" list tracking the cases, because I was having a hard time remembering which cases were left to do. I wasn't sure if you would be able to update a separate post by me, so I edited your post so that you'd have permissions to update the list.

Thanks again for your hard work on this.

pganssle · 2017-11-16T19:16:16Z

@yoch If you rebase against master, the master branch has a fix for the Appveyor issue.

This adds both the SKIP and RSCALE options to rrule, per 7529. They are added together because SKIP requires RSCALE, but non-gregorian calendars are not currently implemented.

pganssle · 2019-02-02T19:20:17Z

I've started some refactoring on this, I'm not sure why the BYMONTHDAY and BYYEARDAY elements were checked off, because I think actually none of the byxxx rules are handled. I guess this has to be pushed to 2.9.0.

Since we do not actually have support for RSCALE in the rrule class, it is premature to allow users to specify it. Instead, we will validate it if present in strings and emit it if necessary to comply with the RFC but otherwise postpone an explicit API for handling RSCALE until after real support is added.

Rather than using hard-code strings, we will use dedicated objects for the skip options, similar to the way other special values are handled.

mojimi · 2020-07-17T14:57:44Z

What can be done to help move this forward? Highly needed feature

immerrr · 2021-03-31T18:02:34Z

@pganssle I have taken a stab at implementing the skipping in 2dfd4a7. It fixes the failing tests that were added to this branch in 39d51f6

My change is somewhat rough: it may raise errors on bymonthday=32 and does not handle negative bymonthdays at all, but it shows the general idea. WDYT?

pganssle requested changes Nov 14, 2017

View reviewed changes

pganssle reviewed Nov 14, 2017

View reviewed changes

pganssle added this to the Feature release milestone Nov 14, 2017

pganssle added enhancement rrule labels Nov 14, 2017

yoch force-pushed the rfc7529 branch from 2fa22f8 to 2172134 Compare November 14, 2017 16:30

pganssle reviewed Nov 14, 2017

View reviewed changes

dateutil/test/test_rrule.py Show resolved Hide resolved

yoch force-pushed the rfc7529 branch from 2172134 to 7925ece Compare November 14, 2017 16:55

pganssle reviewed Nov 16, 2017

View reviewed changes

pganssle mentioned this pull request Dec 5, 2017

Add in RFC7529 extensions to rrule. #285

Open

pganssle added this to Claimed in PyData NYC Sprint Apr 12, 2018

pganssle modified the milestones: Feature release, 2.8.0 Apr 23, 2018

pganssle removed this from Claimed in PyData NYC Sprint Apr 24, 2018

pganssle added this to Open in PyLondinium 2018 Sprint Jun 8, 2018

pganssle removed this from Open in PyLondinium 2018 Sprint Jun 18, 2018

pganssle mentioned this pull request Oct 3, 2018

rrule with YEARLY frequency starting on a leap day #823

Closed

pganssle force-pushed the rfc7529 branch from f558789 to 66f7b46 Compare October 25, 2018 16:33

pganssle added the release-blocker label Oct 25, 2018

pganssle force-pushed the rfc7529 branch from 66f7b46 to 15b1d9b Compare February 2, 2019 15:45

Initial implementation of RFC 7529 SKIP option

3d97b2d

This adds both the SKIP and RSCALE options to rrule, per 7529. They are added together because SKIP requires RSCALE, but non-gregorian calendars are not currently implemented.

pganssle force-pushed the rfc7529 branch from 15b1d9b to d68baf7 Compare February 2, 2019 16:48

pganssle added a commit to yoch/dateutil that referenced this pull request Feb 2, 2019

Add changelog for PR dateutil#522

2ec77fe

pganssle force-pushed the rfc7529 branch 2 times, most recently from d0dabc9 to 18ae390 Compare February 2, 2019 16:52

pganssle modified the milestones: 2.8.0, 2.9.0 Feb 2, 2019

pganssle added 4 commits February 3, 2019 08:18

Add changelog for PR dateutil#522

fbd4052

Use dedicated variables for skip options

4f9e5fa

Rather than using hard-code strings, we will use dedicated objects for the skip options, similar to the way other special values are handled.

Add xfailing test for bymonthday with skip

39d51f6

pganssle force-pushed the rfc7529 branch from 18ae390 to 39d51f6 Compare February 3, 2019 13:18

pganssle mentioned this pull request Sep 1, 2020

[bug] rrule.rrule with monthly freq is ignoring some months #1079

Closed

immerrr pushed a commit to immerrr/dateutil that referenced this pull request Mar 31, 2021

Add changelog for PR dateutil#522

c8407b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement the skip option #522

implement the skip option #522

yoch commented Nov 14, 2017 •

edited by pganssle

yoch commented Nov 14, 2017 •

edited

yoch commented Nov 14, 2017

pganssle left a comment •

edited

pganssle Nov 14, 2017

pganssle Nov 14, 2017

pganssle Nov 14, 2017

yoch Nov 14, 2017

pganssle Nov 14, 2017

pganssle commented Nov 14, 2017

pganssle Nov 14, 2017

yoch commented Nov 14, 2017

pganssle commented Nov 14, 2017

yoch commented Nov 15, 2017

pganssle Nov 16, 2017

yoch Nov 16, 2017

pganssle commented Nov 16, 2017

pganssle commented Nov 16, 2017

pganssle commented Nov 16, 2017

pganssle commented Feb 2, 2019

mojimi commented Jul 17, 2020

immerrr commented Mar 31, 2021

implement the skip option #522

Are you sure you want to change the base?

implement the skip option #522

Conversation

yoch commented Nov 14, 2017 • edited by pganssle

yoch commented Nov 14, 2017 • edited

yoch commented Nov 14, 2017

pganssle left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented Nov 14, 2017

Choose a reason for hiding this comment

yoch commented Nov 14, 2017

pganssle commented Nov 14, 2017

yoch commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented Nov 16, 2017

pganssle commented Nov 16, 2017

pganssle commented Nov 16, 2017

pganssle commented Feb 2, 2019

mojimi commented Jul 17, 2020

immerrr commented Mar 31, 2021

yoch commented Nov 14, 2017 •

edited by pganssle

yoch commented Nov 14, 2017 •

edited

pganssle left a comment •

edited