Disallow datetime ranges with mixed tzinfo #194
Conversation
Peter554
force-pushed
the
disallow-datetime-ranges-with-mixed-tzinfo
branch
from
b14a4b5 to
21ef3e2
Compare
Peter554
force-pushed
the
disallow-datetime-ranges-with-mixed-tzinfo
branch
from
21ef3e2 to
2b982f0
Compare
| dt_range = ranges.FiniteDatetimeRange( | ||
| datetime.datetime(2020, 1, 1, tzinfo=zoneinfo.ZoneInfo("Asia/Dubai")), | ||
| datetime.datetime( | ||
| 2020, 1, 10, tzinfo=zoneinfo.ZoneInfo("Australia/Sydney") | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Removing this test case since it's no longer possible to construct such a range!
| @@ -867,12 +867,21 @@ def intersection( | |||
| # We're deliberately overriding the base class here for better performance. | |||
| # We can simplify the implementation since we know we're dealing with finite | |||
| # ranges with INCLUSIVE_EXCLUSIVE bounds. | |||
| if self.tzinfo != other.tzinfo: | |||
There was a problem hiding this comment.
The if isinstance(other, FiniteDatetimeRange): branch gets its own logic to aid performance. other.tzinfo is simply looking up a field, where as calling get_tzinfo(other) has to execute that entire function.
Peter554
force-pushed
the
disallow-datetime-ranges-with-mixed-tzinfo
branch
from
2b982f0 to
1fab13d
Compare
Peter554
force-pushed
the
disallow-datetime-ranges-with-mixed-tzinfo
branch
from
1fab13d to
949ead8
Compare
| @@ -1019,6 +1019,15 @@ def test_finite_range(self): | |||
|
|
|||
|
|
|||
| class TestFiniteDatetimeRange: | |||
| def test_cannot_construct_with_inconsistent_tzinfo(self): | |||
There was a problem hiding this comment.
It would be nice to have similar logic for DatetimeRange and HalfFiniteDatetimeRange, but those two are currently not independent classes but just type aliases. So there is nowhere to insert the logic 🤔 Is there any reason not to make DatetimeRange and HalfFiniteDatetimeRange actual classes (if we decide to do this I might save that for a separate PR, or do it as a preparation PR)?
Context: #192
Update: I've tried this in Kraken but the number of errors was rather daunting. I think it's unlikely we find time to prioritise this change right now, so I'm proposing #195 as an alternative/first-step to unblock things.