Update FiniteDatetimeRange.days to require passing a TZ
#195
+37
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 30, 2025
It's possible to create mixed TZ FiniteDatetimeRange's. It probably shouldn't be, but it currently is and this is unlikely to change too soon. See #192 This makes the FiniteDatetimeRange.days poorly defined. For example, should the below range be 31 or 30 days? RANGE = ranges.FiniteDatetimeRange( # This is also 2024-03-01T00:00:00 in TZ_UTC datetime.datetime(2024, 3, 1, tzinfo=TZ_LONDON), # This is 2024-04-01T00:00:00 in TZ_LONDON datetime.datetime(2024, 3, 31, hour=23, tzinfo=TZ_UTC), ) This PR updates FiniteDatetimeRange.days to require passing a TZ, to remove this ambiguity.
In #187 the implementation of FiniteDatetimeRange.intersection was optimised. This introduced a very subtle change in behaviour. Previously when left.end was equal to right.end then the right.end was favoured. In #187 the implementation was (unintentionally) changed to favour left.end. This change reverses that, to again favour right.end. This really shouldn't matter, but it can be signidicant when the TZ of left and right are different. Eventually we'd like to prevent this kind of mixed TZ behaviour (see #192), but in the short term it seems reasonable to make this tiny change to make things consistent again with how they were before.
I was surprised to find that we don't already have this function.
Peter554
force-pushed
the
dt-range-days-require-tz
branch
from
ad83af4 to
0d88cbd
Compare
benthorner
reviewed
Jan 30, 2025
| """ | ||
| Return the number of days between the start and end of the range. | ||
| """ | ||
| return (self.end - self.start).days | ||
| range_ = self.localize(tz) |
There was a problem hiding this comment.
❓ How about disallowing differing zones, just for this property?
Requiring tz means we're going to need to update all call sites anyway, so it's easy enough to do period.localize(tz).days instead of period.days(tz). That keeps this property simple and makes the caller a little more explicit with its intent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's possible to create mixed TZ FiniteDatetimeRange's. It probably shouldn't be, but it currently is and this is unlikely to change too soon. See #192
This makes the
FiniteDatetimeRange.daysproperty ambiguous. For example, should the below range be 31 or 30 days?Getting this wrong can lead to bugs in e.g. computation of per-day standing costs.
This PR updates
FiniteDatetimeRange.daysto require passing a TZ, to remove this ambiguity. This is a bit more verbose, but it's much more explicit and less error prone.