Skip to content
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

Editorial: Change TimeDuration to use time durations instead of internal durations #3026

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

anba
Copy link
Contributor

@anba anba commented Oct 28, 2024

Changes:

  • Move handling for day units from RoundTimeDuration to Temporal.Duration.prototype.round.
  • Correct argument type for DifferenceInstant to only allow time units.
  • RoundTimeDuration is now only called with time units and its input and output is a time duration.
  • Remove the no rounding fast path from DifferenceTemporalPlainTime to match DifferenceInstant.

Fixes:
TemporalDurationFromInternal is fallible in Temporal.Duration.prototype.toString:

Temporal.Duration.from({
  days: 1,
  seconds: 2**53 - 1 - (24*60*60),
  nanoseconds: 999_999_999
}).toString({
  roundingMode: "ceil",
  fractionalSecondDigits: 7
});

anba and others added 2 commits November 1, 2024 09:48
…nal durations

Changes:
- Move handling for `day` units from `RoundTimeDuration` to `Temporal.Duration.prototype.round`.
- Correct argument type for `DifferenceInstant` to only allow time units.
- `RoundTimeDuration` is now only called with time units and its input and output is a time duration.
- Remove the no rounding fast path from `DifferenceTemporalPlainTime` to match `DifferenceInstant`.

Fixes:
`TemporalDurationFromInternal` is fallible in `Temporal.Duration.prototype.toString`:
```js
Temporal.Duration.from({
  days: 1,
  seconds: 2**53 - 1 - (24*60*60),
  nanoseconds: 999_999_999
}).toString({
  roundingMode: "ceil",
  fractionalSecondDigits: 7
});
```
Brings the reference code in line with the editorial changes in the spec
text from the previous commit.
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I made the corresponding changes in the reference code. We should add test262 coverage for the case you identified, but I'll open a separate issue for that.

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.97%. Comparing base (99f67ee) to head (c957836).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3026   +/-   ##
=======================================
  Coverage   95.96%   95.97%           
=======================================
  Files          21       21           
  Lines        9677     9684    +7     
  Branches     1746     1745    -1     
=======================================
+ Hits         9287     9294    +7     
  Misses        337      337           
  Partials       53       53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptomato ptomato merged commit 5cb74c3 into tc39:main Nov 1, 2024
10 checks passed
@anba anba deleted the round-time-duration branch November 5, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants