-
Notifications
You must be signed in to change notification settings - Fork 149
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
Different duration balancing PlainDate::until
vs Duration::round
#2563
Comments
Thanks for reporting this. This looks like it's a bug, indeed. We need to verify whether it's actually a bug in the spec (in which case it'd need to be presented for consensus in TC39) or just a mistake in the polyfill. |
This is unfortunately a bug in the spec text and PlainDate.until is not the only place it occurs. It's caused by RoundDuration not being followed by BalanceDuration and/or BalanceDurationRelative as it is in const roundingMode = 'ceil';
// PlainDate.until and since (original example)
const pdPast = Temporal.PlainDate.from('2022-01-01');
const pdFuture = Temporal.PlainDate.from('2023-12-25');
pdPast.until(pdFuture, {largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P1Y12M
pdPast.until(pdFuture).round({relativeTo: pdPast, largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P2Y
// PlainDateTime.until and since
// NB. The problem exists for date units, but time units work correctly
const pdtPast = Temporal.PlainDateTime.from('2022-01-01T00');
const pdtFuture = Temporal.PlainDateTime.from('2023-12-25T00');
pdtPast.until(pdtFuture, {largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P1Y12M
pdtPast.until(pdtFuture).round({relativeTo: pdPast, largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P2Y
// ZonedDateTime.until and since
const zdtPast = Temporal.ZonedDateTime.from('2022-01-01T00[UTC]');
const zdtFuture = Temporal.ZonedDateTime.from('2023-12-25T00[UTC]');
zdtPast.until(zdtFuture, {largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P1Y12M
zdtPast.until(zdtFuture).round({relativeTo: zdtPast, largestUnit: 'year', smallestUnit: 'month', roundingMode})
// => P2Y
// PlainYearMonth.until and since
const pymPast = Temporal.PlainYearMonth.from('2022-01');
const pymFuture = Temporal.PlainYearMonth.from('2023-12');
pymPast.until(pymFuture, {largestUnit: 'year', smallestUnit: 'month', roundingIncrement: 3, roundingMode})
// => P1Y12M
pymPast.until(pymFuture).round({relativeTo: pdPast, largestUnit: 'year', smallestUnit: 'month', roundingIncrement: 3, roundingMode})
// => P2Y
// Duration.toString
const duration = Temporal.Duration.from('PT1H59M59.9S');
duration.toString({smallestUnit: 'second', roundingMode: 'ceil'})
// => 'PT1H59M60S'
duration.round({smallestUnit: 'second', roundingMode}).toString()
// => 'PT2H' With time units, it's generally already implemented correctly (except for Duration.toString): Temporal.PlainTime.from('00:00').until('01:59:59', {largestUnit: 'hour', smallestUnit: 'minute', roundingMode})
// => PT2H
Temporal.Instant.fromEpochSeconds(0).until(Temporal.Instant.fromEpochSeconds(7199), {largestUnit: 'hour', smallestUnit: 'minute', roundingMode})
// => PT2H |
I'm going to make some changes to TemporalDurationToString in order to fix bug #2563. It will be helpful to have it do everything in the same order as the spec text, to verify that the changes work.
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
I'm going to make some changes to TemporalDurationToString in order to fix bug #2563. It will be helpful to have it do everything in the same order as the spec text, to verify that the changes work.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
Meeting 2023-08-17: We'll fix this and the fix has already been approved. Will be fixed in #2571 which is stacked on other PRs at the moment. |
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
In order to fix tc39/proposal-temporal#2563, we added invocations of BalanceDurationRelative after some invocations of RoundDuration. These cause observable calendar calls, which must be accounted for in some existing tests.
See tc39/proposal-temporal#2563 The old behaviour was encoded in one test in staging, but the behaviour of largestUnit in duration rounding has changed since that test was written. Therefore I'm assuming that toString() should've been updated when that happened.
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
…ng() In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the `largestUnit` option in the same way that Duration.prototype.round() would. To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly). In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceTimeDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from the previous discussions, `toString()` was "controls rounding, works same as `round()`", but the status quo was that it worked differently from `round()`.) Closes: #2563
Rounding-up and balancing-up works differently depending whether
PlainDate::until
is used orCalendar::round
is used withrelativeTo
. This seems very similar to this recently-closed issue.The text was updated successfully, but these errors were encountered: