ariebovenberg / whenever

⏰ Modern datetime library for Python, available in Rust or pure Python
https://whenever.rtfd.io
MIT License
831 stars 13 forks source link

OffsetDateTime should support add and subtract #67

Closed bxparks closed 6 months ago

bxparks commented 7 months ago

https://whenever.readthedocs.io/en/latest/overview.html#offsetdatetime

It’s less suitable for future events, because the UTC offset may change (e.g. due to daylight saving time). For this reason, you cannot add/subtract a timedelta — the offset may have changed!

I am confused why OffsetDateTime cannot support add and subtract. An OffsetDateTime has a fixed UTC offset. The add and subtraction operations are well-defined under the fixed UTC offset.

ariebovenberg commented 7 months ago

You're not the first to be unconvinced by this small side-note. It probably warrants a thorough paragraph with reasoning.

Lets give it a try:

OffsetDateTime does not support addition or subtraction of time deltas. This is a deliberate decision to avoid an infamous pitfall. In practice, fixed-offset datetimes are commonly used to express a time at which something occurs at a specific location. But for many locations, the offset changes throughout the year (due to DST or political decisions). Allowing users to add/subtract from fixed-offset datetimes gives them the impression that they are doing valid arithmetic, while in actuality they are setting themselves up for DST-bugs (which, again, are rampant!). Mathematically of course, addition and subtraction are well-defined for fixed offsets. The problem is that fixed-offset datetimes are almost never used in this abstract sense. Almost always, there is an implicit location that must be taken into account when shifting the time. For whenever, preventing a damaging pitfall weighs heavier than supporting a rare, theoretical, usage pattern. This is consisent with other libraries that emphasize correctness, such as NodaTime.

Note that in the future, we can always add an .shift_ignoring_dst() method which allows shifting OffsetDateTime while making it super explicit that DST is not taken into account.

bxparks commented 7 months ago

Yeah... I'm still not convinced. I get the impression that you are fixated on linking the AwareDateTime (and its subclasses) to a location and its IANA timezone. To me, an OffsetDateTime is just a DateTime that has a fixed UTC offset. It has no relationship to a location, timezone, or any complicated DST transition rules.

One example. In the past, I needed to perform import, transform, and analysis of a data dump from a 3rd party processor. The data dump used a date-time format with a fixed UTC offset (I think it was UTC-05:00, but I can't remember). There was no IANA timezone info in the data dump. I don't know if their system used "America/New_York" or "America/Toronto". Or maybe they just used UTC, but converted the data to UTC-05:00 for the purposes of the data dump. Maybe their internal people were used to thinking in UTC-05:00. I don't know. But it didn't matter, because these were events that happened in the past, all I needed was the UTC offset of the events.

For my needs, I needed to transform the data to UTC-08:00, so that it was easier for me to read, and correlate the events to another set of logs and data dumps which were in UTC-08:00. I needed to do subtractions between different dates, so that I could figure out the intervals between the dates. I didn't want to do a full conversion to "America/Los_Angeles", because I did not want the complexity and ambiguity of DST transitions during my analysis.

If I were using the whenever library for this type of analysis, I would want to use OffsetDateTime. And I would want to add and subtract.

ariebovenberg commented 7 months ago

I get the impression that you are fixated on linking the AwareDateTime (and its subclasses) to a location and its IANA timezone. To me, an OffsetDateTime is just a DateTime that has a fixed UTC offset. It has no relationship to a location, timezone, or any complicated DST transition rules.

I haven't been clear then. OffsetDateTime is just a DateTime with a fixed UTC offset. Only ZonedDateTime deals in IANA tzs.

I needed to do subtractions between different dates

Subtracting between times is supported. It's subtracting a duration that isn't.

# this is just fine
offset1 - offset2
# this is not
offset1 - hours(24)

One example. In the past, I needed to perform import, transform, and analysis of a data dump from a 3rd party processor. The data dump used a date-time format with a fixed UTC offset (I think it was UTC-05:00, but I can't remember). There was no IANA timezone info in the data dump. I don't know if their system used "America/New_York" or "America/Toronto". Or maybe they just used UTC, but converted the data to UTC-05:00 for the purposes of the data dump. Maybe their internal people were used to thinking in UTC-05:00. I don't know. But it didn't matter, because these were events that happened in the past, all I needed was the UTC offset of the events.

Correct: you shouldn't have to introduce IANA tz IDS if you don't want to. The use case you mention should be 100% supported. The only limitation is not being able to shift these timestamps into the past or future. Like you say: you don't know if -5 offset is Toronto or Panama. Thus, you don't know the DST rules! You don't know whether 2024-03-31 00:00:00-05:00 plus 12 hours equals 2024-03-31 12:00:00-05:00, it could just as well be 2024-03-31 13:00:00-04:00 due to a timezone transition.

You could argue: "I don't care about DST transitions, I just want to abstractly add/subtract". However, this leaves a huge footgun for the majority of users who do need to account for DST transitions. You could retort: "if you want DST transitions, you should use IANA ZonedDateTime, not a fixed offset", but this ignores the (unfortunate) reality that fixed-offset timestamps are often used to express localized times. In a perfect world, people would know what they're doing, and not use OffsetDateTime for arithmetic on localized times. However, in the world we live in, it falls to libraries to steer (unknowing) developers away from common mistakes.

If you "know what you're doing" and want to abstractly add/subtract: convert to UTC and do your operations there.

bxparks commented 7 months ago

This is so odd. There is no universe that I can imagine where OffsetDateTime cannot support addition and subtraction of a Duration. I want to get a new OffsetDateTime with the same fixed offset. You are correct, I literally don't care about DST transitions. Because DST transitions exist only in the IANA timezones, and you already admitted that OffsetDateTime has nothing to do with IANA timezones.

So to perform the simple addition that I want, I have to do this?

odt = OffsetDateTime(2023, 4, 21, hour=9, offset=hours(-6))
# add 5h to odt
new_odt = OffsetDateTime.from_timestamp(odt.timestamp() + 5*3600, odt.offset())

Anyway, this is minor usability issue in the grand scheme of things. If I were to use this library, I would either write a helper function to do the above, or fork the library to remove the restriction on __add__ and __sub__ on OffsetDateTime.

I'm going to let this issue go...

ariebovenberg commented 7 months ago

So to perform the simple addition that I want, I have to do this?

It need not be so ugly. Here's a cleaner solution:

(odt.as_utc() + hours(5)).as_offset(odt.offset)
# or, as of version 0.4:
odt.as_utc().add(hours=5).as_offset(odt.offset)

Perhaps in the future:

odt.add_ignoring_dst(hours=5)
# or
odt.add(hours=5, ignore_dst=True)  # ignore_dst would be a required parameter, only allowing `True`
ariebovenberg commented 6 months ago

I've now added a section in the docs (FAQ) about this.