-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Modified rrulewraper to handle timezone-aware datetimes. #9028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modified rrulewraper to handle timezone-aware datetimes. #9028
Conversation
2ce1c44
to
68d43e5
Compare
Ping @dstansby - Are you the one reviewing this? Just a gentle prod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still wrapping my head around this, so I have some initial questions.
Another question is if any of this dependent upon a minimum version of dateutil?
@@ -732,20 +733,105 @@ def __call__(self, x, pos=None): | |||
|
|||
|
|||
class rrulewrapper(object): | |||
def __init__(self, freq, tzinfo=None, **kwargs): | |||
kwargs['freq'] = freq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have no clue if it is always safe to modify a **kwargs
. Does it never modify a passed in dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be pretty weird if it could modify a passed in dictionary, since it's an expansion of the keywords. I assume that the only way there would even be a dictionary to pass in would be if you passed it like f(**kwargs)
. Testing it out on Python 3 seems to indicate that does not modify the dictionary. I can try and read up on it later, though.
def test_func(*args, **kwargs):
kwargs['demo'] = 9
if __name__ == "__main__":
kwargs = {}
test_func(**kwargs)
print(kwargs) # {}
kwargs2 = {'demo': 6}
test_func(**kwargs)
print(kwargs) # {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked into this a bit and I don't see anyone actually saying this is safe, but it also doesn't seem like the question has come up before - I would guess that it would be surprising if it were unsafe and someone would have commented on it by now. My understanding of how these things are supposed to work makes me fairly certain that this will not be a problem, but I've opened a question on SO just in case.
Either way, if we find that there's some weird situation where it could be a problem, we can just have a second PR that adds kwargs = kwargs.copy()
at the top of the function, so I don't see any reason why uncertainty in this regard should be blocking.
Edit: I should have waited a few more minutes, the first answer on the SO says that it's always safe based on the spec.
# datetimes are returned | ||
if 'dtstart' in kwargs: | ||
dtstart = kwargs['dtstart'] | ||
if dtstart.tzinfo is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do dtstart = kwargs.get('dtstart', None)
, and therefore be able to reduce all of this by an indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is fine. I think the main reason I did it this way is that it avoids the unnecessary replace(tzinfo=None)
call at the end, but I'm fine with it either way (I really doubt anyone will be bogged down by any potential performance hit in that case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WeatherGod Actually, looking at this, I don't think you can reduce all this by an indent, because if dtstart
is None
, the dtstart.tzinfo is None
and dtstart.replace()
calls will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, it'll fail for a different reason. dtstart would be None
and it wouldn't have the tzinfo
attribute. Don't know why I didn't see that originally.
|
||
if 'until' in kwargs: | ||
until = kwargs['until'] | ||
if until.tzinfo is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: until = kwargs.get('until', None)
def _attach_tzinfo(self, dt, tzinfo): | ||
# pytz zones are attached by "localizing" the datetime | ||
if hasattr(tzinfo, 'localize'): | ||
return tzinfo.localize(dt, is_dst=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to hard-code is_dst
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WeatherGod The default for pytz.localize
is essentially the same as hard-coding is_dst=False
, which is inconsistent with PEP-495, since rrule
will never set the fold
attribute.
In the end I think very few people will need the feature of choosing which side of a DST fold their ticks are on, but it would not be difficult to add in a later version.
@WeatherGod I am not sure if it requires a specific version of I don't know how I would install |
Ping @WeatherGod . Any other changes required? |
PR Summary
dateutil.rrule.rrule
will more or less work just fine withdateutil
zones, but does not work withpytz
zones. In general, I'd say that for the current (and past) version(s) ofdateutil
, it's best to use naive zones for yourrrule
and then attach your originaltzinfo
to the naive zones.This modifies
rrulewrapper
with shims that strips off time zones going in torrule
and then adds them back on the way out ofrrule
.Fixes #9018
PR Checklist