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

Decide whether ISO string with bracketed time zone name but no offset is valid #933

Closed
ptomato opened this issue Sep 22, 2020 · 21 comments · Fixed by #1024
Closed

Decide whether ISO string with bracketed time zone name but no offset is valid #933

ptomato opened this issue Sep 22, 2020 · 21 comments · Fixed by #1024
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@ptomato
Copy link
Collaborator

ptomato commented Sep 22, 2020

The champions group is not all agreed on whether the string '2020-09-18T14:26[America/New_York]' is well-formed.

Various notes:

  • The [America/New_York] part is not standard according to ISO either way. We are trying to standardize it. We would want to avoid accepting something that might not be standardized.
  • What do userland libraries and other languages do with a string like this?
@ptomato ptomato added this to the Complete design decisions milestone Sep 22, 2020
@stebogit
Copy link

@ptomato would you mind giving some context of why this is of interest?
I have never seen or used this format before and as you mentioned it is not covered by ISO, so I wonder why you guys are concern about it.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 24, 2020

Sure! My personal opinion is that it is not well-formed, but I think @pipobscure was in favour of it - can you provide more context?

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 24, 2020

Note there is a bug in the polyfill either way.

> Temporal.DateTime.from('2020-09-18T14:26[America/New_York]')
Temporal.DateTime <2020-09-18T14:00>

Either this should throw or the minute should be 26.

@justingrant
Copy link
Collaborator

justingrant commented Oct 8, 2020

Meeting 2020-10-08 (plus some more notes I added afterwards):

  • We'll wait to see what gets standardized for the one-string serialization of iCalendar/JSCalendar's FORM #3: DATE WITH LOCAL TIME AND TIME ZONE REFERENCE type in https://tools.ietf.org/html/rfc5545#section-3.3.5, and then reconsider whether to allow one-step parsing for that format.
  • In the meantime, we'll document how to parse this format using this code:
s = '2020-09-18T14:26[America/New_York]';
Temporal.DateTime.from(s).toZonedDateTime(s);

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 8, 2020

Good catch on the inconsistency. We do indeed intend to throw on invalid strings (or valid strings with unknown junk after them.)

@ptomato ptomato added the documentation Additions to documentation label Oct 8, 2020
@justingrant
Copy link
Collaborator

  • There is an inconsistency we need to resolve: I believe that the intent was that ISO strings were only allowed to parse if they're valid. If '2020-09-18T14:26[America/New_York]' is an invalid string, then why is it valid to parse it with DateTime.from in the line of code above?

Meeting 2020-10-09: We weren't able to resolve this discussion without @pipobscure in the meeting. We'll discuss it next time.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 9, 2020

We did have a tentative conclusion though! Which was, that we'd in any case write a different cookbook example since the above one wouldn't work.

@justingrant
Copy link
Collaborator

Oh sorry I didn't remember that! We're still going to talk about this with Philipp next week, right?

@justingrant
Copy link
Collaborator

I just re-read the notes and think it's worth chatting with Philipp about this because I think he was pretty adamant that the solution to this was parsing the full string. I'd like to understand more about how he was thinking this should work.

@pipobscure
Copy link
Collaborator

2020-09-18T14:26[America/New_York] is only an invalid string in current spec because of the tz. Without that it would be valid but incomplete (unqualified) time. https://en.m.wikipedia.org/wiki/ISO_8601#Time_zone_designators

So I don’t by the premise.

@justingrant
Copy link
Collaborator

justingrant commented Oct 15, 2020

Meeting 2020-10-15:

  • 2020-09-18T14:26[America/New_York] will be considered a valid string format for parsing into any Temporal type, including TimeZone.from (which currently throws with this format). This will be helpful because the iCalendar data model is DateTime+TimeZone and having a string format that mirrors that data model would be helpful.
  • When parsed into a ZonedDateTime, this string format will act just like the property bag format (and like DateTime.toZonedDateTime) which is to use the disambiguation option to determine the offset if there's any ambiguity.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 15, 2020

In more detail, the status quo is that irrelevant "parts" of an ISO string are ignored by from() functions. So, an ISO string with a date part and time part passed to Temporal.PlainDate.from() ignores the time part.

What changes: previously a time zone part consisted of an offset and an optional time zone name annotation. So a time zone name annotation by itself was not a valid part. After this change, we consider offset parts and time zone name parts to be separate, so Temporal.PlainDate.from('2020-09-18T14:26[America/New_York]') parses the date part, and ignores the time part and time zone name part.

@ptomato ptomato added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved and removed meeting-agenda labels Oct 15, 2020
@ptomato ptomato self-assigned this Oct 20, 2020
ptomato added a commit that referenced this issue Oct 20, 2020
Previously we considered a "time zone part" to be an offset plus an
optional bracketed name. Now an ISO string may have either of an "offset
part" and "time zone name part", independently of each other.

Closes: #933
Ms2ger pushed a commit that referenced this issue Oct 21, 2020
Previously we considered a "time zone part" to be an offset plus an
optional bracketed name. Now an ISO string may have either of an "offset
part" and "time zone name part", independently of each other.

Closes: #933
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 21, 2020
@sffc
Copy link
Collaborator

sffc commented Oct 29, 2020

I think it's okay for Temporal to allow strings with brackets but no offsets, but I want to point out why that is quirky behavior as it relates to RFC 3339 and the calendar annotation (#293).

The calendar annotation is a projection: the data model remains expressed in the ISO calendar, but it should be interpreted in the context of the calendar system. I see the IANA time zone in the same way: it projects an instant into a time zone.

In both cases, you can drop the annotation while still retaining the meaning of the core ISO-8601 string. If you have a ZonedDateTime string with offset and time zone annotation, you can drop the time zone annotation, and the instant remains the same. If you have a ZonedDateTime or Plain[Date][Time] string with a calendar annotation, you can drop the calendar annotation, and the civil date[time] remains the same.

However, if you have a string with a bracketed time zone but no offset, this invariant doesn't hold. If you drop the time zone annotation from such a string, you downgrade from a ZonedDateTime to a PlainDateTime, losing the instant.

@pipobscure
Copy link
Collaborator

Is the Instant really the core meaning of the ISO string? Or is it the unqualified time that is there and should still be projected into the given timezone?

@justingrant
Copy link
Collaborator

Yep. A similar case is 2020-01-01[Asia/Tokyo], which means Jan 1 2020 interpreted in Tokyo. So the general idea is that a time zone annotation could be added to any date or time unit. That doesn't mean that Temporal needs to allow parsing all those variations, but the IETF spec seems like it should allow them because there are valid use cases for them.

@justingrant
Copy link
Collaborator

I think the rule for ZDT.from should be that we'll allow parsing of a date+timzeone, a date+time+timezone, or a date+timezone+offset+timezone.

@pipobscure
Copy link
Collaborator

Ok except for date+timezone because what time would you pick. In ZonedDateTime I think you have to have a datetime to start with.

@justingrant
Copy link
Collaborator

Ok except for date+timezone because what time would you pick. In ZonedDateTime I think you have to have a datetime to start with.

If you're using an object property bag for from of both PlainDateTime and ZonedDateTime, the hour/minute/second are optional, which IMHO is the correct behavior because "start of day" is a reasonable default. Are you saying that the string variant of from for both of those types should require a time? Currently neither does, which I also think is good.

@sffc
Copy link
Collaborator

sffc commented Oct 30, 2020

"2020-09-18T14:26[America/New_York]" is a ZonedDateTime, which is defined as an instant that happened in a place. A time zone database is required in order to figure out what the instant is, which is not good for interop. Furthermore, dropping the time zone completely changes the meaning of the string (from an instant-with-annotation to a civil time).

@justingrant
Copy link
Collaborator

A time zone database is required in order to figure out what the instant is, which is not good for interop

The interop problem already exists because of iCalendar's data model is DateTime+TimeZone. I think the general question for the industry (not just Temporal) is how should iCalendar-compliant apps serialize a DateTime+TimeZone into a single string? IMHO it's better to have a single, standardized string representation for this data model instead of individual developers making one up.

"2020-09-18T14:26[America/New_York]" is a ZonedDateTime

I would state it differently: "2020-09-18T14:26[America/New_York]" can be parsed into a ZonedDateTime, with the caveat that this particular string format is not canonical (which is why the default toString() output has an offset) but parsing is typically lenient of omitted data (e.g. seconds) if a sensible default can be applied, which I think is true in this case.

@justingrant
Copy link
Collaborator

I opened #1082 to track the questions in the last few comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants