add support for DTSTART;TZID= in rrules #619

ryanpetrello · 2018-01-26T21:18:23Z

ryanpetrello · 2018-01-26T21:19:55Z

@pganssle one oddity I've noticed - and this is why the tests are failing on Travis-CI but they pass for me locally - when you parse the string this way, tzkey gets set to "AMERICA/NEW_YORK", because of the entire string being uppercased here: https://github.com/dateutil/dateutil/blame/master/dateutil/rrule.py#L1504

This causes the actual tz.gettz call to fail (the test passes on my OS X file system due to case insensitivity).

Do you have any thoughts on the proper way to solve this?

pganssle · 2018-01-26T21:21:33Z

dateutil/rrule.py

                    for parm in parms:
+                        if parm.startswith("TZID="):
+                            tzkey = parm.split('TZID=')[-1]
+                            tzlookup = tzinfos.get if tzinfos else tz.gettz


I think it's best to explicitly compare to None, since I would think that tzinfos={} should not be equivalent to tzinfos=tz.gettz.

I guess I'm confused about when you would want to use tz.gettz. Wouldn't it be any time tzinfos wasn't a dictionary with explicit keys?

@ryanpetrello I am fine with tz.gettz being the default if you don't specify anything, but tzinfos can also be a callable, like this:

def my_tzinfos(tzid): if tzid in SOME_LOOKUP_TABLE: return tzid[SOME_LOOKUP_TABLE] else: try: return dateutil.tz.tzstr(tzid) except ValueError: return tz.tzutc()

I don't really know if that particular function is something someone would actually want to do, but the point is that tzinfos is basically a mapping between tzid and a tzinfo object, so any function that provides tzinfo as a function of tzid should work. As a convenience, you can also provide a dictionary directly, rather than passing mytzinfos.get.

if callable(tzinfos): tzlookup = tzinfos elif isinstance(tzinfos, dict): tzlookup = tzinfos.get else: tzlookup = tz.gettz

More like this?

pganssle · 2018-01-26T21:24:01Z

@ryanpetrello Hmmm.. That might be a tough one to fix if most of the rest of the function is assuming the string is already upper-cased.

ryanpetrello · 2018-01-26T21:31:52Z

@pganssle Here's one way we could solve this, but it's pretty gross:

diff --git a/dateutil/rrule.py b/dateutil/rrule.py
index 2f2511c..00edaff 100644
--- a/dateutil/rrule.py
+++ b/dateutil/rrule.py
@@ -1502,6 +1502,7 @@ class _rrulestr(object):
         if compatible:
             forceset = True
             unfold = True
+        orig_s = s
         s = s.upper()
         if not s.strip():
             raise ValueError("empty string")
@@ -1569,6 +1570,13 @@ class _rrulestr(object):
                     for parm in parms:
                         if parm.startswith("TZID="):
                             tzkey = parm.split('TZID=')[-1]
+                            # the string is uppercase, so we have to find the
+                            # TZID=value in its original case
+                            try:
+                                tzpos = orig_s.upper().index(tzkey)
+                                tzkey = orig_s[tzpos:tzpos+len(tzkey)]
+                            except IndexError:
+                                continue
                             tzlookup = tzinfos.get if tzinfos else tz.gettz
                             TZID = tzlookup(tzkey)
                             continue

Maybe an alternative would be to parse a mapping of the potential TZINFO=FOO; values prior to the upper() call:

diff --git a/dateutil/rrule.py b/dateutil/rrule.py
index 2f2511c..08b044c 100644
--- a/dateutil/rrule.py
+++ b/dateutil/rrule.py
@@ -8,6 +8,7 @@ including support for caching of results.
 import itertools
 import datetime
 import calendar
+import re
 import sys
 
 try:
@@ -1502,6 +1503,8 @@ class _rrulestr(object):
         if compatible:
             forceset = True
             unfold = True
+
+        TZID_NAMES = dict(map(lambda x: (x.upper(), x), re.findall('TZID=(?P<name>[^:]+):', s)))
         s = s.upper()
         if not s.strip():
             raise ValueError("empty string")
@@ -1568,7 +1571,10 @@ class _rrulestr(object):
                     valid_values = {"VALUE=DATE-TIME", "VALUE=DATE"}
                     for parm in parms:
                         if parm.startswith("TZID="):
-                            tzkey = parm.split('TZID=')[-1]
+                            tzkey = TZID_NAMES[parm.split('TZID=')[-1]]
                             tzlookup = tzinfos.get if tzinfos else tz.gettz
                             TZID = tzlookup(tzkey)
                             continue

ryanpetrello · 2018-01-29T15:18:34Z

@pganssle is this current patch looking any better to you?

pganssle · 2018-01-29T15:33:34Z

@ryanpetrello Sorry, just had a baby. It will be a bit before I can review this PR.

ryanpetrello · 2018-01-29T15:35:57Z

👶👶👶 congrats!

Don't let me bother you, then - get to this whenever you find time 😄

pganssle · 2018-02-08T20:35:33Z

dateutil/rrule.py

+                                tzlookup = tzinfos
+                            elif isinstance(tzinfos, dict):
+                                tzlookup = tzinfos.get
+                            else:


Once again I don't think this is the right thing to do. tzlookup should be set to tz.gettz if and only if the user requested the default behavior.

I would probably do it this way:

if tzinfos is None: tzlookup = tz.gettz elif callable(tzinfos): tzlookup = tzinfos else: tzlookup = getattr(tzinfos, 'get', None) if tzlookup is None: raise ValueError('tzinfos must be a callable or mapping')

pganssle · 2018-02-08T20:40:04Z

dateutil/rrule.py

            forceset = True
            unfold = True
+
+        TZID_NAMES = dict(map(


Both dict and map are very slow compared to a dictionary comprehension in this case:

TZID_NAMES = {x.upper(): x for x in re.findall('TZID=(?P<name>[^:]+):', s)}

That said, I don't love this approach (though it may be the best option, unfortunately). I'd fix the comprehension part but don't worry about the regex part - we can fix that with a bigger refactor later I think.

pganssle · 2018-02-08T20:43:22Z

dateutil/rrule.py

+                        if dtstart.tzinfo is None:
+                            dtstart = dtstart.replace(tzinfo=TZID)
+                        else:
+                            raise ValueError('DTSTART specifies multiple timezones')


Did you happen to check of the RFC mentions whether or not this is allowed?

This is one of the two reasonable options - the other being that if DTSTART is timezone-aware (e.g. 20180204T0800Z) and the rule has a specific time zone, you convert DTSTART into that time zone and generate dates starting from there. If the RFC is not silent on this point, we could choose either one, really, this one being more conservative and so probably preferable for a first release.

pganssle · 2018-02-08T20:46:00Z

dateutil/test/test_rrule.py

                          datetime(1998, 9, 2, 9, 0),
                          datetime(1999, 9, 2, 9, 0)])

+    def testStrWithTZID(self):


I think you should add some tests for actually specifying the tzinfos parameter, and especially the case of tzinfos={} (which should fail to parse any TZID specified).

pganssle · 2018-03-11T00:33:10Z

@ryanpetrello I was going to push directly to your branch, but you made this PR from the master branch of your fork, which is a PITA because I have my own master branch on my local fork, and it means that I can't easily track your upstream branch. In the future it's best to create a branch on your fork before making a PR.

My updated version of this TZID support is in #624, and is slated for inclusion in 2.7.0 (which should go out this weekend).

add support for DTSTART;TZID= in rrules

438a50a

see: dateutil#614

ryanpetrello mentioned this pull request Jan 26, 2018

add support for DTSTART;TZID= in rrules #615

Closed

pganssle reviewed Jan 26, 2018

View reviewed changes

properly detect callable tzinfos, and respect TZID case insensitivity

115d746

ryanpetrello force-pushed the master branch from 397405e to 115d746 Compare January 26, 2018 21:44

add another test for ambiguous TZID= in combination w/ UTC Z notation

8ed31cf

pganssle requested changes Feb 8, 2018

View reviewed changes

pganssle mentioned this pull request Mar 11, 2018

Support for TZID= in rrulestr #624

Merged

pganssle closed this Mar 11, 2018

pganssle added this to the 2.7.0 milestone Mar 11, 2018

pganssle added enhancement rrule labels Mar 11, 2018

ryanpetrello mentioned this pull request Mar 16, 2018

pin botocore to avoid dependency hell re: latest python-dateutil ansible/awx#1598

Merged

add support for DTSTART;TZID= in rrules #619

add support for DTSTART;TZID= in rrules #619

Uh oh!

Conversation

ryanpetrello commented Jan 26, 2018

Uh oh!

ryanpetrello commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pganssle Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

ryanpetrello Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

ryanpetrello Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

pganssle commented Jan 26, 2018

Uh oh!

ryanpetrello commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanpetrello commented Jan 29, 2018

Uh oh!

pganssle commented Jan 29, 2018

Uh oh!

ryanpetrello commented Jan 29, 2018

Uh oh!

pganssle Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

pganssle Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

pganssle Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

pganssle Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

pganssle commented Mar 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryanpetrello commented Jan 26, 2018 •

edited

Loading

ryanpetrello Jan 26, 2018 •

edited

Loading

ryanpetrello commented Jan 26, 2018 •

edited

Loading