TimeType subtraction using promote

[baumgold]

No description provided.

[Seelengrab]

I don't think this is necessarily a good change - conversion to a DateTime IMO should be explicit, and not implicitly default to midnight. Due to this PR, there also is now a worse error for -(::Date, ::Time), which complains that "promotion failed". It would be better if it informed the user that they need to decide at what time their Date exists (I know that Date + Time already makes the same assumption, which I think is just as bad).

[baumgold]

I don't think this is necessarily a good change - conversion to a DateTime IMO should be explicit, and not implicitly default to midnight. Due to this PR, there also is now a worse error for -(::Date, ::Time), which complains that "promotion failed". It would be better if it informed the user that they need to decide at what time their Date exists (I know that Date + Time already makes the same assumption, which I think is just as bad).

That's fair. Maybe this should be limited to AbstractDateTime rather than TimeType. #49805. @Seelengrab - what do you think?

[Seelengrab]

I'm not sure. It depends on what you intended to do with the change originally - you gave no reasoning for it after all 🤷

[baumgold]

Here’s my reasoning: I use an internal type that subtypes AbstractDateTime with some extra functionality not currently available in DateTime. I would like to be able to subtract DateTime and my custom internal type that subtypes AbstractDateTime. We might want this to be handled more generally for all TimeTypes, or may want to limit it to AbstractDateTime. I don’t have a strong opinion here.

[Seelengrab]

You're free to implement -(::AbstractDateTime, ::MyDateTime) on your type, as long as you use the API of AbstractDateTime to interact with it - that doesn't necessarily imply that -(::AbstractDateTime, ::AbstractDateTime) is good to have in Base either (perhaps it is? At least this shouldn't have timezone interpretation issues..), but such a discussion should generally happen within a PR defining semantics before it's merged.

@quinnj can you clarify why you merged this? Do we have an official stance on which time a Date "sits" at? It's hard to follow this without a review comment..

[quinnj]

Why is it a problem to promote to midnight? Do you have any concrete examples where that would be "bad" by doing it implicitly and why it would be better to do it explicitly? I merged because I feel like I remember that we already have places where we do this kind of "midnight assumption" and I've never heard of it biting anyone or there being anything wrong in practice, so I'm just curious why the pushback? I'm certainly up for discussing if this is causing real problems for people somewhere.

[Seelengrab]

I feel like I remember that we already have places where we do this kind of "midnight assumption"

We have it in the DateTime constructor:

julia> DateTime(today())
2023-05-13T00:00:00

which shouldn't exist.

The issue is that in timezone aware code, there is no one "midnight", especially not in DST aware code, where there may be more than one midnight (or no midnight at all - the whole list is a good source, and links to more sources). There's also boatloads of discussions (and controversial & differing opinions) in this python discussion. There's also the issue that ISO-8601 actually defines two midnights per day (see here): 00:00, for the start of the day, and 24:00, for the end of the day. I don't think we handle either correctly right now (I'd have to check - I checked, we change 24:00 to 00:00 the next day... which is incorrect), but this sort of distinction is completely lost when just saying "midnight is the start of the day". What, for example, is today() plus 24 hours? That doesn't make sense - it's a timespan and another timespan, whose ultimate duration depends on the timezone.

I recognize that Base itself is oblivious to timezones (which I think is a bit of a mistake), but I think it would be a bigger mistake to encourage coding patterns that encourage being oblivious to the complexity of time.

[quinnj]

I'm not sure I'm really following the arguments/logic here. The approach in Dates stdlib is to keep things simple, and the Date/DateTime are actually super useful in lots of applications because they're timezone-unaware. Java wrestled with this for years until they finally realized that JodaTime had this nice thing called LocalDate/LocalDateTime that most users actually wanted most of the time. We patterned Date/DateTime off of those and by and large, I don't think we've had a lot of major complaints about that.

I also realize there are tons of these more esoteric micro-cases where time/dates/timezones can be super weird. We purposely didn't want to have to deal w/ as much of that as possible in the stdlib because they introduce so much complexity and 99% of use-cases don't want/need to deal with that.

And that's also why the package ecosystem is so great! You can have all these super-specific, nuanced Date packages that take all this stuff into account. Does the stdlib need improvement? Sure. And we should open specific issues for concrete things to improve. But on a high-level, I'm still of the opinion that the Dates module has fared pretty well with its pragmatic approach to things.

[Seelengrab]

All of that is fine and A-Ok - I'm not saying we should get rid of Date/DateTime entirely. It has its purpose! What I AM saying is that all this doesn't mean that we should encourage handling timezones/time conversions incorrectly/with more implicit assumptions. To that end, I think introducing codepaths that make assumptions such as "midnight will be fine" is a bad idea ™️ . That way lies future pain and complaints of footguns, which I'd rather like to avoid.

And that's also why the package ecosystem is so great! You can have all these super-specific, nuanced Date packages that take all this stuff into account. Does the stdlib need improvement? Sure. And we should open specific issues for concrete things to improve.

Great, let's discuss the stance of Base about at what time Date lies in an issue then and revert this change for now, seeing as this wasn't discussed at all before being merged :) The original motivation of the PR can be done better by using dispatch after all, so that shouldn't block the development of a package.

[quinnj]

Sounds good; thanks.

[Seelengrab]

Done #49821 - should this PR be reverted directly, or should we just merge #49805, which seems much more reasonable/tame to me?

[baumgold]

I vote to merge #49805 but I’m okay reverting too if needed.