Skip to content
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

Inconsistency in Duration days in duration.total() vs instant.add(). #2768

Closed
pauldraper opened this issue Feb 8, 2024 · 6 comments
Closed
Labels

Comments

@pauldraper
Copy link

duration.total():

Interpreting years, months, or weeks requires a reference point.

instant.add():

The years, months, weeks, and days fields of duration must be zero. Temporal.Instant is independent of time zones and calendars, and so years, months, weeks, and days may be different lengths depending on which calendar or time zone they are reckoned in.

This is inconsistent. Why should instant.add() recognize day length variability, but not duration.total()?

@justingrant
Copy link
Collaborator

justingrant commented Feb 10, 2024

The reasoning was that if a developer wants to add days to a Temporal.Instant, then they should be using Temporal.ZonedDateTime to perform the addition, so that DST changes are taken into account. The exception serves to push developers towards the correct Temporal type. Also, it's a subtle reminder that Temporal.Instant has no concept of dates whatsoever, which is also why there's no month, day, year properties on Temporal.Instant.

In the Duration case, there's no other type to push users to use instead. This was one of the compromises we made when we decided to offer a single duration type, rather than do what Java and some others do where there's a "date duration" and a "time duration". Merging all units into the same duration type had some advantages (e.g. simpler for users) but also some disadvantages, like the inconsistency you found.

With only a single Duration type, it's a valid (and very common) use case to create durations that contain only dates, e.g. {days: 30} or the result of Temporal.PlainDate.prototype.until. For those durations, it's a valid operation to add or subtract days without worrying about DST, because the original duration was date-only. And requiring a starting point might actually result in the wrong answer if the user uses a ZonedDateTime starting point with a DST transition during the date range.

Therefore, we decided to make Temporal.PlainDate.Duration.prototype.total (and add and subtract) more flexible. If a Temporal.ZonedDateTime reference point is provided, then DST is taken into account. If not (meaning no starting point or a Temporal.PlainDate or Temporal.PlainDateTime reference point) then DST is ignored and all days are assumed to be 24 hours.

Hopefully this explains the compromise solution we chose.

@pauldraper
Copy link
Author

pauldraper commented Feb 11, 2024

(As an aside, while I understand Duration was a deliberate decision, IMO it was a poor one. Java/Joda/js-joda absolutely got this right by (1) distinguishing between Period and Duration and (2) not worrying about balancing. I predict Duration to be extremely troublesome forever.)


The oddity is that add/subtract/total needs a reference point for weeks, months, years, (because their lengths vary) but not for days (even though its length likewise varies).

For consistency, a reference point should be required for days as well.

(Although...I would argue that add/subtract don't need to balance. So they shouldn't need a reference, while total and round do need a reference.)

@BurntSushi
Copy link

I've gotten some feedback for Jiff (a Rust datetime library heavily inspired by Temporal) that has essentially echoed this point, including an example of a footgun: they parsed some ISO 8601 durations from some data file, wanted to convert them to seconds and assumed that duration rounding would return an error if there was any ambiguity about what the units meant. But since days are implicitly treated as 24 hours even when there is no relative datetime, it looks like it might have wound up doing the wrong thing in this case. (I don't have the full use case mapped out yet.)

I'm considering what the impact would be of, in Jiff at least, always requiring a relative date when durations contain non-zero units of days for operations like compare, total and round. (For Temporal, I think that would mean erroring for Duration.add with non-zero day units, but Jiff still accepts a relative date its analogous API.) I think it make dealing with the spans returned by PlainDateTime and PlainDate APIs like since and until more annoying, but otherwise, I believe all other durations returned use units of hours or less.

@BurntSushi
Copy link

(Also, meta note, please let me know if my use of Temporal's issue tracker to help get clarity on design points as a way to influence Jiff's design is something that is unwanted.)

@ptomato
Copy link
Collaborator

ptomato commented Jul 26, 2024

Using the issue tracker for this is fine!

I think you already summarized it really well in the issue thread that you linked:

If you're working with a Zoned, for example, it is impossible to accidentally treat "1 day" as "always 24 hours." The APIs won't get it wrong precisely because a Zoned is involved. But when you're working with civil time, you want to be able to operate on days, yet civil time is unaware of time zones. So days are always 24 hours.

If I understand the issue correctly, it's about the case when you don't pass a relativeTo for operations that accept one. IIRC in discussion threads like #686 we intentionally accepted that there is a footgun here because the convenience of not making a fake relativeTo outweighs it; at least, we think that's the case for the environment of JS.

I think it'd be justified if a different library made a different choice for a different environment. You could, for example, make opting into this case more explicit by requiring some sort of marker, like the Rust equivalent of relativeTo: 'civil-time-without-calendar' or something like that. Or you could have multiple Duration types, as several other libraries do.

(Having only one Duration type is one of the design decisions I'm most conflicted about in hindsight, FWIW; but given the constraints on Temporal's size, we certainly wouldn't add a second one now even if the proposal was still at a stage where that'd be possible.)

@BurntSushi
Copy link

The "marker" idea is interesting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants