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

Rounding, Part 2 #907

Merged
merged 3 commits into from
Sep 16, 2020
Merged

Rounding, Part 2 #907

merged 3 commits into from
Sep 16, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Sep 15, 2020

This adds rounding options to Temporal.Absolute.prototype.difference, Temporal.DateTime.prototype.difference, and Temporal.Time.prototype.difference.

See: #827.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #907 into main will increase coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
+ Coverage   93.18%   93.54%   +0.35%     
==========================================
  Files          17       17              
  Lines        5373     5671     +298     
  Branches      826      856      +30     
==========================================
+ Hits         5007     5305     +298     
  Misses        359      359              
  Partials        7        7              
Flag Coverage Δ
#test262 50.15% <0.00%> (-2.66%) ⬇️
#tests 88.98% <98.73%> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
polyfill/lib/absolute.mjs 97.29% <100.00%> (+0.38%) ⬆️
polyfill/lib/datetime.mjs 94.97% <100.00%> (+0.27%) ⬆️
polyfill/lib/ecmascript.mjs 95.46% <100.00%> (+0.49%) ⬆️
polyfill/lib/time.mjs 97.69% <100.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d0cba4...181793d. Read the comment docs.

docs/absolute.md Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the 827-difference-rounding branch 2 times, most recently from 929ab41 to 8814097 Compare September 15, 2020 19:24
const largestUnit = ES.ToLargestTemporalUnit(options, 'seconds', ['years', 'months', 'weeks', 'days']);
const disallowedUnits = ['years', 'months', 'weeks', 'days'];
const smallestUnit = ES.ToSmallestTemporalDurationUnit(options, 'nanoseconds', disallowedUnits);
let defaultLargestUnit = 'seconds';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let defaultLargestUnit = 'seconds';
const defaultLargestUnit = 'seconds';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's reassigned in the next line if smallestUnit is larger than defaultLargestUnit.

spec/absolute.html Outdated Show resolved Hide resolved
docs/time.md Outdated Show resolved Hide resolved
Includes several new abstract operations:

- ToSmallestTemporalDurationUnit, which is distinct from
ToSmallestTemporalUnit because it prefers plural unit names and does have
a fallback value;

- ValidateTemporalDifferenceUnits, which makes sure that largestUnit is
not smaller than smallestUnit;

- and MaximumTemporalDurationRoundingIncrement, which computes the maximum
(not inclusive) value for roundingIncrement given the value of
smallestUnit, for rounding a Temporal.Duration.

All of these will be reused for Temporal.DateTime.difference and
Temporal.Time.difference in subsequent commits.

See: #827
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
@ptomato ptomato merged commit 1f4a210 into main Sep 16, 2020
@ptomato ptomato deleted the 827-difference-rounding branch September 16, 2020 22:01
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.

3 participants