-
Notifications
You must be signed in to change notification settings - Fork 157
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
Proposed Temporal changes required to adopt RFC 5545 (iCalendar) meaning of durations #759
Comments
It's worth adding 1c, throw an exception when the Temporal.Duration has illegal fields when operating on a Temporal.DateTime. This is the currently existing behavior when you give a Temporal.Duration with illegal fields to a Temporal.Absolute. There would be no |
By "illegal fields" do you mean any time units for |
@sffc - Ping! (If possible, I want to wrap up feedback on this issue before this week's meeting)
|
Yes, that's what I meant for |
OK, I added (1c) with this suggestion. That said, an API that sometimes throws and sometimes doesn't based on the time portion of its arguments feels like it will be easy for bugs slip through that would only be caught in production. With 1b and 1a, failures would tend to happen on the first run of the code or could be caught at compile/lint time. Also (from #686 (comment)):
|
I agree that it would be weird for Can you change 1c to have the stated behavior for |
Sounds good. 1c is now updated to remove |
So far this sounds ok to me. As for reversibility, what is meant us that for any given datetime A and duration D the statements:
should be true. In short date maths should remain commutative/reversible as mentioned above. I also agree that we need to keep months/years supported. Otherwise we’ll need to start a whole new secondary duration object. One of the big reasons why I’m OK with this is that because durations don’t balance (by default) and you can apply this to Absolutes it is still possible to get any other behaviour by carefully crafting the durations. And as a default behaviour: doing what every calendaring app on the planet does seems quite reasonable. |
@pdunkel - how would you rank 1a (add TimeZone arg to all Date math methods) vs. 1b (remove math from DateTime; use Date or LocalDateTime math instead) vs. 1c (remove
Does this mean that you like 5.5 (copied below)? Or do you mean something else? 5.5 A nice side effect of the changes above would be that we could support calendar-aware balancing, which implies that we could do Duration math for months and years too, and could offer a Duration.prototype.difference method. |
Ok in more detail:1.a: Remove maths from DateTime: why??? the 1.b: Same thing. DateTime does not need a TimeZone parameter for maths. Time-Zones do NOT apply. And this distinction is very important, because without it the teachability and clarity of the whole suffers. 1.c: WHY??? Their behaviour is well defined and entirely predictable. As a matter of fact NOT doing 1 and keeping things on 1 2: Restricting Absolute Maths: There is no way to do it correctly without a calendar. This is the case already so nothing new here. And also not what we suggested agreement on. 3: This is what I was mainly referring to and what I thought had achieved a form of rough agreement: RFC 5545 durations, which are basically means: RFC5545 does the larger units given a calendar which defaults to ISO and the sub-day units as in an absolute way thereby ignoring DST changes making the need to have a TimeZone entirely superfluous. 5: I see this a complimentary / orthogonal to everything else. The reason this would matter at all is because NOT balancing would let you control exactly how your maths are done, which in my view is essential to make 3 even feasible. In short I believe we need to have 3 and 5 to make this work. We can then optionally add (And I believe we should) a LocalDateTime like Justin proposed which combines DateTime/TimeZone/Calendar/Absolute into one. What we would be left with is:
So to answer the question directly: I rank all of the 1s as entirely unsuitable, and yes I think 5 has merit given that I understand it as durations do not balance by default, but can be balanced explicitly given a calendar. What that implies for |
(Just to respond to the ping, I haven't followed this closely enough to have an opinion, and I'm happy to defer to you all.) |
@pdunkel - Glad I asked for more detail! My understanding of the agreement from the last meeting was that we were going to enforce that all use of durations in Temporal adhered to RFC 5545 definition of durations. Let's discuss further in the meeting. |
I was convinced by @pipobscure's point that keeping DateTime and Absolute arithmetic as they currently are, already adheres to the RFC 5545 definition. (Except for the point about changing the order in which the fields are processed.) (I'm less convinced by the idea of explicitly changing our explanation from "Absolute is just a number" to "Absolute is in UTC" and adding weeks/months/years to Absolute arithmetic.) I think it's a very clean solution that we have 3 Temporal types, each with a different kind of arithmetic (absolute, wall calendar, and DST-aware) and therefore don't need to have 3 kinds of durations as well. I did quite like the explanation of "Temporal.Duration is basically a property bag with some methods for convenience" and would have been sad to lose that. As for the open question about balancing durations, I'm fine with moving balance to a dedicated method, and making |
I am also unconvinced. Everywhere we expose date/time unit getters, we introduce the possibility for bugs where a user thinks that they're getting one kind of Another advantage of the current design is that if a developer accidentally passes an Absolute to a function that expects a DateTime or LocalDateTime, that call will probably throw an exception because Absolute doesn't have any of the same getters. This behavior seems like a feature, not a bug.
@ptomato - could you clarify what you mean by this? Do you mean the And what do you think about the solutions to this problem proposed in 5.3-5.6 above?
I think it's OK to leave DateTime and Absolute math as they are, but I don't think that this behavior matches RFC 5545, because RFC 5545 requires dates to be considered calendar dates and times to be considered Absolute time. Neither DateTime nor Absolute can possibly do that with durations that have both date and time parts, because neither type has awareness of actual time zones which would be required to do that calculation properly. So I think what @pipobscure is recommending is closest to option 1d from #686:
|
Both.
I agree that subtraction needs balancing, and therefore addition with negative durations as well. Instead of any of the solutions in 5.3 I'd rather just throw if the result of the operation was something like "3 days and –2 hours". My opinion is that we should not have calendar-aware balancing. It seems easy enough to do on a case-by-case basis in userland, there are several examples of it in the documentation already, yet a general-case API for it would be complicated. It's also separate enough from the set of operations that we currently have, and it seems uncommon enough that the lack of it doesn't detract from Temporal, so it could easily be introduced in a follow-up proposal.
OK, maybe that was a bit optimistic of me. You are right that DateTime math doesn't match RFC 5545. I was thinking if you add 1 day then it's a calendar day, but I didn't consider that if you add 24 hours then that effectively adds 1 calendar day, which is not RFC 5545. On the other hand, I do think Absolute math does match if you consider it to be in UTC, but I had already said above that I didn't want to consider it to be in UTC. |
What's left to discuss on this issue? I have the feeling that we've addressed some of these concerns, and accepted others as necessary complexity for supporting enough common use cases. |
I think we can indeed close this |
At the end of the 2020-07-09 meeting, we (@sffc, @gibson042, @pdunkel, and I) unexpectedly reached tentative consensus in favor of Option 3c from #686, which is to define
Temporal.Duration
to mean the same as durations are defined in iCalendar/RFC5545. @ptomato, @littledan, @ryzokuken - you guys were in the meeting but I'm not sure if you were still there at the end, so you should weigh in!Anyway, this writeup attempts to flesh out the changes this decision would imply for Temporal APIs. It's OK to have buyer's remorse; if you agreed in the meeting but are skeptical after seeing the implications, then now is a good time to speak up.
Requirements
Here's a summary of how RFC 5545 durations differ from current Temporal durations:
DateTime
difference and a time portion that represents anAbsolute
difference. SoPT2H
would be considered absolute time,P2D
would be two calendar days, andP2DT2H
would mean 2 calendar days plus 2 real-world hours.-P2DT2H
. A no-op plus-sign prefix is also supported.plus
andminus
#653'compatible'
disambiguation algorithm is used.IMHO, I don't think it makes sense to adopt the units limitations (no months/years/partial-seconds) in Temporal. There are many common use-cases (e.g. localized formatting, stopwatch timing, etc.) that require these units. Also, being unable to parse valid ISO 8601 durations seems like a bad idea. Other than those unit limitations, my assumption is that "Adopt RFC 5545 durations" means that we'd adopt the rest of the requirements above.
At the end of this issue are excerpts from the RFC 5545 spec that describe the above in more detail.
Specific Changes
Below are one possible implementation of the requirements above. Note that each of the top-level numbered points below are intentionally independent. We could PR them separately even if we choose not to do all of them.
Feel free to suggest other ways to achieve the same results.
1a.
DateTime
math methods should require a time zone parameterDateTime.prototype.plus
andDateTime.prototype.minus
should add a REQUIRED second parameter for the time zone. e.g.dt.plus({hour: 1}, 'Europe/Paris', {disambiguation: 'reject'})
. Developers who want the current clock-time-onlyDateTime
math semantics can pass'utc'
for the time zone. (Ornull
we we adopt @sffc's "null time zone" idea from LocalDateTime fields in from and with #706 wherenull
impliesDateTime
behavior.)DateTime.prototype.difference
should add a REQUIRED second parameter for the time zone. In theory it's only required if the times of both instances are different, but IMHO it's cleaner to just make it always required. Users who want date-only behavior can get it via a'utc'
(ornull
) time zone ordt.toDate().difference(other)
.this
(for all math methods) and/orother
(fordifference
) are ambiguous due to DST? Should we have twodisambiguation
options on each call? One option that applies to both endpoints? Or should we just usecompatible
disambiguation because this is what RFC 5545 mandates for its equivalent ofDateTime.prototype.toAbsolute
? Note that if we offered DST disambiguation control, then presumably we'd need to rename the existingdisambiguation
option, e.g. tooverflow
per Suggestion: split option name intodisambiguation
(for DST) vs.overflow
(for ranges) #607.P2DT12H
to2020-03-06T02:30
inAmerica/Los_Angeles
, after the date portion is added the result is2020-03-08T02:30
which is in the middle of an hour skipped by DST. DST disambiguation will be needed for that intermediate value. I've been working through this problem forLocalDateTime
and I have some tentative solutions, of which the most appealing is simply to usecompatible
disambiguation which is what RFC 5545 relies on for its equivalent ofDate.prototype.toAbsolute
. BTW, I'd prefer to get a consensus recommendation on how we should solve this from the IETF owners of RFC 5545. See RFC 5545 vs DST questions for IETF calendar standards ("calext") group #702.======== OR ======== (1a/1b/1c are mutually exclusive)
1b. Remove
DateTime
math methods (or even remove the entireDateTime
type?) and rely onLocalDateTime
for non-Absolute date/time mathLocalDateTime
for math instead? Here's an example ofDateTime
math and itsLocalDateTime
equivalent:DateTime
's current timezone-less, DST-unsafe math behavior could use'utc'
or an offset timezone when creating the LocalDateTime.this
(see 1a.3) would be handled on the "convert toLocalDateTime
" step rather than being part of the add/subtract/difference operation. This may be clearer than trying to disambiguate multiple DateTime values in one call per (1a.3) and (1a.4).DateTime
completely and just rely onLocalDateTime
. @sffc has encouraged considering this option. I admittedly haven't thought through the implications of this and don't yet have an opinion for or against it. But I wanted to mention it here for completeness. BTW, if we did remove theDateTime
type, then we'd probably want to renameLocalDateTime
toDateTime
for brevity.DateTIme
type but remove its math methods, then should we make its name more obscure (e.g.ZonelessDateTime
) to avoid disappointing developers who expect to find math capability in a type calledDateTime
?ZonedDateTime
(Instant + TimeZone + Calendar) #700. If we decide not to use that type, then this option is moot.======== OR ======== (1a/1b/1c are mutually exclusive)
1c. Limit
DateTime
'splus
/minus
methods to days or larger units, and remove itsdifference
method.DateTime
.DateTime.prototype.plus
andDateTime.prototype.minus
would throw if the duration has any non-zero hour (or smaller) units. Developers who want to add durations with both date and time units would have to useLocalDateTime
math methods. (Or roll their own implementation, which is hard to get right because of DST disambiguation in the middle of the calculation.)DateTime.prototype.difference
will be removed, because it's likely to be unexpected (and therefore bug-inducing) for it to throw only when the arguments have different local times. Developers can usetoDate().difference(other)
for date-only values. Developers who haveDateTime
s with different times who wanted DST-ignoring math would use.toLocalDateTime('utc').difference()
. Otherwise, developers would usetoLocalDateTime
into a real time zone and calculate the difference there.DateTime.prototype.plus
andDateTime.prototype.minus
would be revised to accept aDuration | DateDurationLike
instead ofDuration | DurationLike
currently. This would enable compile-time detection of object literals with illegal units.2.
Absolute
math methods should be limited to hours or smaller units.Absolute.prototype.plus
andAbsolute.prototype.minus
should throw if the duration has any non-zero day (or larger) units. Developers who really want to calculate (24-hour) days can use.toDateTime('utc').plus()
(unless we choose 1c) or.toLocalDateTime('utc').plus()
.Absolute.prototype.difference
should throw iflargestUnit
isdays
or larger. Developers who really want Temporal to calculate the number of 24-hours days can use.toDateTime('utc').difference()
(unless we choose 1c) or.toLocalDateTime('utc').difference()
.Absolute
math methods have an optional time zone parameter to allow math with days and larger units without requiring a round trip toDateTime
? My inclination is "no" to simplify the ergonomics for working with absolute time, but I admittedly haven't though this through very carefully. Feedback welcome.Absolute
.Absolute.prototype.plus
andAbsolute.prototype.minus
would be revised to accept aDuration | TimeDurationLike
instead ofDuration | DurationLike
currently. This would prevent object literals with illegal units.3. Temporal math methods should match RFC 5545 expectations around negative durations and (maybe) order-of-operations
plus
in Temporal already works that way;minus
doesn't. In our previous champions-meeting discussion about negative durations, @pdunkel was concerned that using largest-first order-of-operations for subtraction would break reversibility, e.g.A - B + B === A
. After more reading of the RFC 5545 spec, I'm now unsure about whether the spec actually defines subtraction behavior, because all its discussion is about "addition" and all its examples of negative durations are under 24 hours where order of operations is irrelevant anyways. My suggestion: let's discuss this problem with the calext/calconnect working group that owns the iCalendar spec, and see if we can come to consensus about what the subtraction order-of-operations should be, and then adopt that behavior in Temporal. And if they don't care, we're probably free to keep @pdunkel's preferred reversibility. This is already on the list of questions for that working group in RFC 5545 vs DST questions for IETF calendar standards ("calext") group #702. I just need an email intro!("4" was removed but I forgot to renumber the "5" section, so leaving numbering as-is)
5. The
Duration
type's math and balancing methods will get more complicatedfrom
,with
,plus
, andminus
, this implies a lot more complexity for those methods OR we could remove balancing from at least some of them. I'll discuss each below.from
andwith
seem like the easiest cases: we could simply remove the balance feature and offer balancing via abalance
method. Note that there's already been discussion about a separate balancing method to handle rounding too. See @sffc's idea at Round a Temporal.Duration to the nearest small unit #337 (comment).minus
is the tough one because subtraction inherently needs balancing. Even if we support negative durations, RFC 5545 doesn't support mixed-sign units; durations are either negative or positive as a whole. So balancing is always needed forminus
. Here's a few possible solutions:Duration.prototype.plus
doesn't have a balancing option today (see Suggestion: consistent balancing behavior for Duration'splus
andminus
methods #645), but it'd need it if we did negative durations. So however we solved this problem forminus
, we'd want to clone the same solution forplus
.Duration
math for months and years too, and could offer aDuration.prototype.difference
method.'utc'
. This is the same as 5.6.1 except that the TS types would treatstart
as optional.null
. This is the same as 5.6.3 except TS would have an easier time detecting thenull
type vs'utc'
.TimeDuration
class without YMWD fields and whose math and balance methods wouldn't accept timezone, starting point, and calendar options. This type would be returned byAbsolute.prototype.difference
andTime.prototype.difference
.Relevant Specification Excerpts
From https://tools.ietf.org/html/rfc5545#section-3.3.6:
From https://tools.ietf.org/html/rfc5545#section-3.3.5
The text was updated successfully, but these errors were encountered: