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

Restrict Time to temporal range, remove arithmetic #6002

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robertbastian
Copy link
Member

Fixes #5987

@robertbastian robertbastian requested a review from sffc as a code owner January 15, 2025 16:00
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me but want @zbraniecki sign-off on this.

I don't really see the "problem" with allowing the larger values, especially if we aren't doing arithmetic with them. We allow them in formatting, which seems fine.

@sffc sffc requested a review from zbraniecki January 15, 2025 17:40
@robertbastian
Copy link
Member Author

I think we shouldn't format times like 30:489:12

@zbraniecki
Copy link
Member

This makes us not handle 24:00:00 or 12:60:00, correct?

@robertbastian
Copy link
Member Author

yes

@zbraniecki
Copy link
Member

This will break compatibility with ISO 8601. Are we ok with this?

@robertbastian
Copy link
Member Author

ISO 8601 requires only 24:00:00, right?

@zbraniecki
Copy link
Member

I thiiink so? I requested access to ISO 8601, but I haven't read it yet myself. I also tried to deduct from Temporal if they target strict set or subset of ISO8601. No luck. @sffc ?

@robertbastian
Copy link
Member Author

Temporal only requires 00:00:00..23:59:59, Shane linked to it in the issue.

@zbraniecki
Copy link
Member

Ok, so Temporal supports a subset of ISO 8601? Are we ok anchoring on Temporal instead of ISO 8601? I think I'm fine with that, but prefer to have this be a group alignment. :)

@robertbastian
Copy link
Member Author

I think we should add 24:00:00 back. If the string "24:00:00" doesn't parse anymore, then our IXDTF parser isn't compliant anymore (unless it special-cases 24:00:00 to become 00:00:00 on the next day, but that's more arithmetic than I like during parsing).

@sffc
Copy link
Member

sffc commented Jan 15, 2025

Temporal uses RFC 9557 which is based on RFC 3339 which states

   date-fullyear   = 4DIGIT
   date-month      = 2DIGIT  ; 01-12
   date-mday       = 2DIGIT  ; 01-28, 01-29, 01-30, 01-31 based on
                             ; month/year
   time-hour       = 2DIGIT  ; 00-23
   time-minute     = 2DIGIT  ; 00-59
   time-second     = 2DIGIT  ; 00-58, 00-59, 00-60 based on leap second
                             ; rules

https://www.rfc-editor.org/rfc/rfc3339.html#section-5.6

Whereas Temporal states

... Leap seconds are not taken into account. ...

1. If hour < 0 or hour > 23, then
   a. Return false.
2. If minute < 0 or minute > 59, then
   a. Return false.
3. If second < 0 or second > 59, then
   a. Return false.
...

https://tc39.es/proposal-temporal/#sec-temporal-isvalidtime

ISO 8601-1 (2019) states

In the 24-hour clock, a clock second is represented by a two-digit ordinal number from ‘00’, identifying
the first second, to ‘58’, ‘59’ or ‘60’, identifying the last second of a clock minute (‘58’ with a negative
leap second, ‘59’ without a leap second, ‘60’ with a leap second).

and

For information interchange there is no representation of end of day. It is recognized that the expression
‘24:00:00’ is used as a natural language expression to denote end of a day; but for the benefit of clarity,
‘24’ shall not be used to represent hour in accordance with this document.

and

Note 2 to entry: An inserted second is called a positive leap second and an omitted second is called a negative leap
second. A positive leap second is inserted after [23:59:59Z] and can be represented as [23:59:60Z]. A negative
leap second is achieved by the omission of [23:59:59Z]. Insertion or omission takes place as determined by the
International Earth Rotation and Reference Systems Service (IERS), normally on 30 June or 31 December, but if
necessary on 31 March or 30 September.

In other words, there is no provision in any of these specifications for the "exclusive bound" to be represented (24:00:00, 00:60:00, or 00:00:61). So I think it's safe to remove those.

But probably we should allow seconds with a value of 60. Our job is to represent things that clients may reasonably want to represent when formatting.

We are in the business of formatting. I think temporal_rs can and should make different decisions than the ones we make here.

@sffc
Copy link
Member

sffc commented Jan 15, 2025

RFC 3339 states

Although ISO 8601 permits the hour to be "24", this profile of ISO
8601 only allows values between "00" and "23" for the hour in order
to reduce confusion.

But, my copy of ISO 8601-1 (2019) specifically calls out that 24 is not allowed. Perhaps previous versions of ISO 8601-1 allowed this.

@zbraniecki
Copy link
Member

Yes, I'm aligned. I got a hold of the ISO. 4.3.8 - 4.3.10 make it relatively clear that we do not need to support 24:00:00, but we need to support 12:00:60 if I'm reading the spec correctly

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

Successfully merging this pull request may close these issues.

Time allows construction with 24:60:61.999999999
3 participants