[Temporal] relativeto cleanup and move Duration staging old/accepts-...#4924
Open
[Temporal] relativeto cleanup and move Duration staging old/accepts-...#4924
Conversation
…m old/accepts-datetime-strings-or-fields-for-relativeTo Some tests in `test/staging/Temporal/Duration/old/accepts-datetime-strings-or-fields-for-relativeTo.js` are covered by tests in - `test/built-ins/Temporal/Duration/prototype/total/relativeto-string-datetime.js` - `test/built-ins/Temporal/Duration/prototype/total/relativeto-string-plaindatetime.js` - `test/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-no-time-units.js` - `test/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-optional-properties.js` Merge `test/built-ins/Temporal/Duration/prototype/total/relativeto-string-plaindatetime.js` into `test/built-ins/Temporal/Duration/prototype/total/relativeto-string-datetime.js`, rename the latter to `test/built-ins/Temporal/Duration/prototype/total/relativeto-string.js`, refactor the latter using `forEach`, moving the throwing cases into `relativeto-string-invalid.js`, removing the test case covered by `relativeto-string-zoneddatetime-wrong-offset.js`, and finally add the only not covered staging test case "20200101" to it (similarly for `round` and create such a file for `compare`). Remove the `test/built-ins/Temporal/Duration/prototype/round/accepts-datetime-strings-for-relative-to.js` which is very similar to the staging test this PR removes, because its tests are covered by - `test/built-ins/Temporal/Duration/prototype/round/relativeto-string-datetime.js` - `test/built-ins/Temporal/Duration/prototype/round/relativeto-string-date.js` (just renamed from `relativeto-string-plaindatetime.js`) - `test/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-no-time-units.js` - `test/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-optional-properties.js` Additionally, for `total`: - Merge `relativeto-string-zoneddatetime.js` into `relativeto-string.js. + Same also for `round` and `compare`. - Merge `relativeto-string-plaindatetime-invalid.js` into `relativeto-string-limits.js`. - Delete `relativeto-number.js` and `does-not-accept-non-string-primitives-for-relative-to.js` because they are covered by `relativeto-wrong-type.js`. + Same also for `round`. - Merge `throws-on-wrong-offset-for-zoneddatetime-relativeto.js` into `relativeto-string-zoneddatetime-wrong-offset.js`, and rename the latter to `relativeto-string-wrong-offset.js`. + Similarly for `round`, and `compare`. - Rename all `*relative-to*`, `*relativeTo*`, and `*relative` test files with `relativeto-*` names (also for `round` and for `compare`). This to to make reviewing and cleanup easier, by bunching up the relativeto tests.
ptomato
approved these changes
Feb 12, 2026
Contributor
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Nice cleanup! Approved with one minor adjustment.
| const relativeTo = "2000-01-01T00:00+05:30[UTC]"; | ||
| assert.throws(RangeError, () => Temporal.Duration.compare(duration1, duration2, { relativeTo })); | ||
| assert.throws(RangeError, () => Temporal.Duration.compare(duration1, duration2, { relativeTo: "2000-01-01T00:00+05:30[UTC]" })); | ||
| assert.throws(RangeError, () => Temporal.Duration.compare(duration1, duration2, { relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" })); |
Contributor
There was a problem hiding this comment.
One suggestion for improvement here. The existing test was probably written when sub-minute precision was still allowed in brackets. But it is no longer valid so this string fails to parse before the offset is computed. Something like this would be better:
Suggested change
| assert.throws(RangeError, () => Temporal.Duration.compare(duration1, duration2, { relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" })); | |
| assert.throws(RangeError, () => Temporal.Duration.compare(duration1, duration2, { relativeTo: "1971-01-01T00:00+02:00[-00:44]" })); |
|
|
||
| assert.throws(RangeError, () => instance2.round({ | ||
| smallestUnit: "seconds", | ||
| relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" |
Contributor
There was a problem hiding this comment.
Same here
Suggested change
| relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" | |
| relativeTo: "1971-01-01T00:00+02:00[-00:44]" |
|
|
||
| assert.throws(RangeError, () => instance2.total({ | ||
| unit: "months", | ||
| relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" |
Contributor
There was a problem hiding this comment.
Suggested change
| relativeTo: "1971-01-01T00:00+02:00[-00:44:30]" | |
| relativeTo: "1971-01-01T00:00+02:00[-00:44]" |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Some tests in
test/staging/Temporal/Duration/old/accepts-datetime-strings-or-fields-for-relativeTo.jsare covered by tests intest/built-ins/Temporal/Duration/prototype/total/relativeto-string-datetime.jstest/built-ins/Temporal/Duration/prototype/total/relativeto-string-plaindatetime.jstest/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-no-time-units.jstest/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-optional-properties.jsMerge
test/built-ins/Temporal/Duration/prototype/total/relativeto-string-plaindatetime.jsintotest/built-ins/Temporal/Duration/prototype/total/relativeto-string-datetime.js, rename the latter totest/built-ins/Temporal/Duration/prototype/total/relativeto-string.js, refactor the latter usingforEach,moving the throwing cases into
relativeto-string-invalid.js, removing the test case covered byrelativeto-string-zoneddatetime-wrong-offset.js, and finally add the only not covered staging test case "20200101" to it (similarly forroundand create such a file forcompare).Remove the
test/built-ins/Temporal/Duration/prototype/round/accepts-datetime-strings-for-relative-to.jswhich is very similar to the staging test this PR removes, because its tests are covered bytest/built-ins/Temporal/Duration/prototype/round/relativeto-string-datetime.jstest/built-ins/Temporal/Duration/prototype/round/relativeto-string-date.js(just renamed fromrelativeto-string-plaindatetime.js)test/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-no-time-units.jstest/built-ins/Temporal/Duration/prototype/round/relativeto-propertybag-optional-properties.jsAdditionally, for
total:relativeto-string-zoneddatetime.jsinto `relativeto-string.js.roundandcompare.relativeto-string-plaindatetime-invalid.jsintorelativeto-string-limits.js.relativeto-number.jsanddoes-not-accept-non-string-primitives-for-relative-to.jsbecause they are covered byrelativeto-wrong-type.js.round.throws-on-wrong-offset-for-zoneddatetime-relativeto.jsintorelativeto-string-zoneddatetime-wrong-offset.js, and rename the latter torelativeto-string-wrong-offset.js.round, andcompare.*relative-to*,*relativeTo*, and*relativetest files withrelativeto-*names (also forroundand forcompare). This to to make reviewing and cleanup easier, by bunching up the relativeto tests.