Various bugfixes and making things consistent #1393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This must have been left over from moving draft spec text out into the Intl.DurationFormat proposal.
We added a fallback for mergeFields() several months ago but forgot to note it in the documentation.
PlainTime doesn't have a calendar argument in its constructor.
This doesn't match the spec, and has different observable effects, so should not be in the polyfill.
Previously, the returned object would always be an instance of Temporal.ZonedDateTime even if the from() method was called on a subclass of ZonedDateTime. This is a bug. No change needed in the spec text, which already is correct here.
The spec text performs RequireInternalSlot on the returned value to ensure that it's a Temporal.Instant. Do the same in the polyfill in order to be spec-compliant.
The spec text does not say that toString() may be called here. Use the internal slot if present to get the identifier. If that's not possible, make the message generic.
We already call toString() once on the time zone in extractOverrides(), and we don't need the actual time zone object subsequently, so return the result of toString() from extractOverrides() instead of the time zone object. As far as I can tell this is already done correctly in the spec text.
The polyfill was validating the returned array in some cases but not consistently. Create an abstract operation to call this method and validate the result consistently, in order to be compliant with the spec text. Requires a small bug fix in the spec text (omitted argument on CreateListFromArrayLike.)
This is a bug in the spec text. The calendar era() and eraYear() methods take an object with [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots, which ZonedDateTime does not have. No change needed to the polyfill which does this correctly.
This introduces a bunch of abstract operations, and brings the existing ones in line with each other, to consistently use GetMethod (which throws if the method isn't callable) to get the calendar methods off of the calendar object. Any methods for which we have agreed that Temporal core (and not the calendars) should validate the return value, are now also consistently validated in these abstract operations. Bring the spec text in line with this as well. Some of these abstract operations existed, but weren't consistently used. (The exception to this is where the method was fetched only once from the calendar object to be called multiple times. This will be resolved one way or the other in issue #1294.)
Codecov Report
@@ Coverage Diff @@
## main #1393 +/- ##
==========================================
+ Coverage 95.36% 95.70% +0.33%
==========================================
Files 19 19
Lines 11137 11140 +3
Branches 1829 1811 -18
==========================================
+ Hits 10621 10661 +40
+ Misses 511 476 -35
+ Partials 5 3 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cjtenny
approved these changes
Feb 26, 2021
There was a problem hiding this comment.
All looks good, thanks for fixing these! I'm surprised it didn't require any tests changes. I don't think it's worth adding test/test262 changes right now for all of these (e.g. verifying that exceptions don't call timeZone.toString), but I think we should add tests (doesn't have to be now; test262 seems fine) for:
- Properly checking callable on retrieved methods
- That timeZone.getInstantFor properly throws on non-Temporal.Instant values
(eventually). Do we have a list of 'things missing from the test262 tests' somewhere?
|
Cam and I discussed that we do need to start keeping a checklist of things that need to be test262'd. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Encountered while investigating #1294