Misc editorial improvements #1505
Merged
+2,279
−415
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
Closed
Codecov Report
@@ Coverage Diff @@
## main #1505 +/- ##
==========================================
- Coverage 95.58% 92.91% -2.67%
==========================================
Files 19 17 -2
Lines 11139 11111 -28
Branches 1736 1639 -97
==========================================
- Hits 10647 10324 -323
- Misses 485 774 +289
- Partials 7 13 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
6c1f7ac to
d06b4c2
Compare
ljharb
reviewed
Apr 29, 2021
d06b4c2 to
ff399a3
Compare
ljharb
reviewed
Apr 29, 2021
ff399a3 to
c6fb16d
Compare
Ms2ger
reviewed
May 3, 2021
3abf67c to
09c157e
Compare
GetOptionalTemporalCalendar → GetTemporalCalendarWithISODefault ToOptionalTemporalCalendar → ToTemporalCalendarWithISODefault See: #1411
We are going to rename all the ValidateFoo operations in the spec text to IsValidFoo, and remove the RejectFoo operations. This doesn't apply to the polyfill because the form where throwing the exception is part of the operation is more convenient in code. But, still rename RejectInstantRange to ValidateEpochNanoseconds because we are making the same Instant → EpochNanoseconds change to IsValidEpochNanoseconds, because that is what is actually passed to the function.
4864d70 to
f94e662
Compare
|
What's changed since the last time: I removed the GetOption commit since I'm still working on ensuring that all the entry points that use GetOption are covered by test262 tests. I added test262 tests that cover "Remove NonNegativeModulo abstract operation". This is ready for merging if the test262 tests pass review. I'll open a separate PR for the GetOption changes so that this one isn't delayed on that any longer. |
Ms2ger
approved these changes
Jun 1, 2021
spec/plaindatetime.html
Outdated
| 1. Assert: _month_ ≥ 1 and _month_ ≤ 12. | ||
| 1. Assert: _day_ ≥ 1 and _day_ ≤ ! ISODaysInMonth(_year_, _month_). | ||
| 1. Assert: ! ValidateTime(_hour_, _minute_, _second_, _millisecond_, _microsecond_, _nanosecond_) is *true*. | ||
| 1. Assert: ! IsValidISODate(_year_, _month_, _day_). |
In the spec text, rename ValidateFoo operations to IsValidFoo, and remove the RejectFoo operations (since they can all be replaced by something like "If ! IsValidFoo(_args_) is *false*, throw a *RangeError* exception." ValidateInstant, RejectInstant → IsValidEpochNanoseconds (Instant → EpochNanoseconds because that's what's actually passed to the operation) ValidateISODate, RejectISODate → IsValidISODate ValidateTime → IsValidTime ValidateISODateTime → remove in favour of IsValidISODate and IsValidTime ValidateISOYearMonth → IsValidISOMonth (year unused) ValidateTemporalDuration → ValidateDuration (drop Temporal because the argument is not a Temporal.Duration object) Make some formatting consistent around this. See: #1411 Co-authored-by: Ms2ger <Ms2ger@gmail.com>
Based on the removal of the other ValidateFoo operations, rename this one to correspond to ISODateTimeWithinLimits.
This operation is only called on Duration fields that have just been returned by ToLimitedTemporalDuration, which calls ValidateTemporalDuration, so it never has any effect and is redundant everywhere it's used. See: #1411
This is not only the mergeFields operation for the ISO calendar, it's the fallback one in case the method is not present on a calendar. See: #1411
This is not needed. x modulo y already has this meaning. (Unlike the % operator in JS, which does not.) See: #1411
This is to be done comprehensively in #1413, but may as well fix these cases that were identified in review. (Comma separators are currently not used in the spec text, so prefer scientific notation if the numbers are large.)
f94e662 to
23f287f
Compare
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.
Marked as draft, because I would like to write tests where appropriate before merging.See: #1411