Batch of fixes for editorial issues #2735
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 language should hopefully make it clearer that _offsetOption_ only has any effect when _offsetBehaviour_ is ~option~.
Also, document the 24-hour UTC offset limitation, which led us to remove this example, in the docs for getOffsetNanosecondsFor. Closes: #2725
This exact same assertion is also repeated at the top of the AO. Closes: #2731
The calendar record may come from either zonedRelativeTo or plainRelativeTo. Closes: #2734
This step was missing a field from the returned Record. Also reorder the fields for consistency with the other steps. Closes: #2732
The calendar is a String here, so the method can be looked up infallibly. Closes: #2730
GetUTCEpochNanoseconds has an assertion that MakeDate, MakeTime, and MakeDay all result in a finite value. However, several places call GetUTCEpochNanoseconds with a year value that may be excessively large, notably from parsing an ISO string such as +999999-01-01, which would cause that assertion to be hit. Add checks in the appropriate places to make sure that the year value is not excessively large. Closes: #2729
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2735 +/- ##
=======================================
Coverage 96.38% 96.38%
=======================================
Files 21 21
Lines 12444 12444
Branches 2250 2250
=======================================
Hits 11994 11994
Misses 395 395
Partials 55 55 ☔ View full report in Codecov by Sentry. |
Ms2ger
approved these changes
Dec 13, 2023
| @@ -1122,10 +1122,10 @@ <h1> | |||
| 1. Assert: TimeZoneMethodsRecordHasLookedUp(_timeZoneRec_, ~getOffsetNanosecondsFor~) is *true*. | |||
| 1. Assert: TimeZoneMethodsRecordHasLookedUp(_timeZoneRec_, ~getPossibleInstantsFor~) is *true*. | |||
| 1. Let _dateTime_ be ? CreateTemporalDateTime(_year_, _month_, _day_, _hour_, _minute_, _second_, _millisecond_, _microsecond_, _nanosecond_, *"iso8601"*). | |||
| 1. If _offsetBehaviour_ is ~wall~ or _offsetOption_ is *"ignore"*, then | |||
There was a problem hiding this comment.
It's not obvious to me why this is correct.
There was a problem hiding this comment.
This was something that came up in a review of the test262 tests with Cam. It's confusing that offsetOption has no effect, unless offsetBehaviour is ~option~. (To realize that, you have to look long and hard at all callers of this operation, which is not good for reading comprehension.) This change was intended to make it clearer on a first read.
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.
Most of these were recently spotted by @anba (cc and thanks!)