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

WIP: Add proposals for resolving questions with from()/Temporal.parse() #503

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Apr 8, 2020

See: #428

@ptomato ptomato mentioned this pull request Apr 8, 2020

Use cases we identified:

1. Parse an ISO 8601 string and get the exact information that is mandated by ISO 8601, which means that the whole string is relevant. (24:00 is midnight the following day, :60 seconds are coerced to :59).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This already gums up the works a bit, because seconds 60 is valid ISO 8601 but not representable in Temporal, and hours 24 is invalid but accepted by ECMAScript Date. So there are arguments for (at least sometimes, perhaps under author control?) rejecting the first as out of range and rejecting the second as invalid. Being liberal without opt-in strictness could stymie well-intentioned code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, hours 24 is valid ISO 8601 (and was added to the RFC 3339 grammar in an erratum.) I should elaborate here to say that what I mean by "parse an ISO 8601 string" is really "parse a string that conforms to our particular subset and interpretation of ISO 8601". In another pull request (#404, which I think can't be merged until this issue is resolved) I've made an attempt at specifying that format exactly.

Copy link
Collaborator

@gibson042 gibson042 Apr 10, 2020

Choose a reason for hiding this comment

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

FWIW, hours 24 is valid ISO 8601 (and was added to the RFC 3339 grammar in an erratum.)

My understanding is that it's only valid in time intervals.

The end of day representation, where [hh] has a value of [24], shall not be used for a single time point.

I should elaborate here to say that what I mean by "parse an ISO 8601 string" is really "parse a string that conforms to our particular subset and interpretation of ISO 8601". In another pull request (#404, which I think can't be merged until this issue is resolved)

That makes sense, but I'm pointing out that there can be problems when the ECMAScript interpretation differs from author requirements in a way that is difficult or impossible to correct.

time24 = '2020-04-07T24:00:00-07:00[America/Vancouver]';

// Invalid time zone (although valid ISO 8601 string)
invalidZone = '2001-09-08T18:46:40+01:00[America/Vancouver]';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case is particularly interesting, because there are cases where you can't know if the offset is valid or invalid for the time zone (or where you can "know" but be wrong), e.g. 2019-12-15T10:23-02:00[America/Sao_Paulo] was valid per 2019a tz data (announced March 2019), but not valid per 2019b (announced July 2019) because Brazil stopped observing Daylight Saving Time. Being strict without an escape hatch could also stymie well-intentioned code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I originally thought that the escape hatch would be Temporal.parse. But I agree with you, that is too cumbersome when parsing serialized strings that may have been invalidated by political changes since the time when they were serialized. I'll change this.

Temporal.Absolute would still need to consider both the offset and the IANA name, to be able to disambiguate in the case of a wall-clock time that is repeated in a DST transition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was the motivation for the disambiguation parameter in the original Temporal.Absolute.fromString.

Because there are three valid parsing scenarios:

  1. IANA & Offset have to match (default)
  2. Use the IANA-Zone and only use offset to disambiguate
  3. Use the Offset and ignore IANA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, (3) is not covered in this proposal for how Temporal.parse() works ... I'll revise it to make sure the offset is accessible separately.

@ryzokuken
Copy link
Member

@ptomato could you please rebase this on top of #506 or master if it gets merged by the time you read this? Thanks.

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #503 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #503   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files          17       17           
  Lines        3953     3953           
  Branches      582      582           
=======================================
  Hits         3828     3828           
  Misses        123      123           
  Partials        2        2           
Flag Coverage Δ
#test262 52.47% <ø> (ø)
#tests 92.22% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d8a475...17cd30f. Read the comment docs.

@ptomato ptomato force-pushed the 428-from-parse branch 2 times, most recently from 3406d43 to 369add2 Compare April 16, 2020 20:08
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation around our flavor of ISO strings and the code examples!

@ptomato
Copy link
Collaborator Author

ptomato commented Apr 22, 2020

Thanks for the review. I think the remaining open comments are on the topic which of these alternatives we should choose, not disagreeing on the contents of this document, so I'll merge this now. The issue remains open, because we still have to pick an alternative 😄

@ptomato ptomato merged commit 9d17c18 into main Apr 22, 2020
@ptomato ptomato deleted the 428-from-parse branch April 22, 2020 21:54
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.

4 participants