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

Remove {disambiguation: 'balance'} in with() and from() of non-Duration types #642

Merged
merged 9 commits into from
Jun 4, 2020

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 2, 2020

Fixes #609. Removes use of {disambiguation: 'balance'} in non-Duration classes' with and from methods, because all known use cases are better served by using plus() instead. Usage is removed from docs, cookbook, polyfill, tests, TS types, and spec.

Note that balancing behavior is still present, because it's needed for plus and minus. it's just the balance disambiguation option for with and from that's being removed for non-Duration types, because instead of shoehorning math operations into with or from, we want to guide developers to use real math operations like plus or minus.

Duration still retains this option for its with and from because Duration (unlike other types) can create and persist unbalanced Duration instances, so there needs to be some way to create a balanced clone of an existing duration.

With this PR, non-Duration arithmetic options now have the same disambiguation options as from and with. Abstract operations were renamed accordingly:

  • The old abstract operation ToTemporalDisambiguation with balance is now used only in Duration, so it's renamed to ToDurationTemporalDisambiguation
  • ToArithmeticTemporalDisambiguation now matches the same disambiguation options as are used by non-Duration with and from methods, so it's been renamed to ToTemporalDisambiguation and this combined abstract operation it's now used for all types' plus and minus as well as non-Duration with and from.

Fixes tc39#609. Removes non-`Duration` use of `{disambiguation: 'balance'}`
from polyfill, because all known use-cases are better served by using
`plus()` instead.

Note that balancing _behavior_ is still present, because it's needed for
`plus` and `minus`.  it's just the `balance` disambiguation option for
`with` and `from` that's being removed for non-Duration types.

`Duration` still retains this option, because (unlike other types),
there's a need to create and persist unbalanced `Duration` instances.

Now, non-Duration arithmetic options have the same disambiguation
options as `from` and `with`. Abstract operations were renamed
accordingly.
Converts the cookbook's single non-Duration use of
`{disambiguation: 'balance'}` to use `plus` instead.
@justingrant justingrant force-pushed the non-duration-balance-option branch from b1760b6 to eab70dd Compare June 2, 2020 16:20
@justingrant
Copy link
Collaborator Author

Fixed merge conflicts and force-pushed.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #642 into main will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
- Coverage   96.15%   96.11%   -0.04%     
==========================================
  Files          17       17              
  Lines        4027     3988      -39     
  Branches      650      645       -5     
==========================================
- Hits         3872     3833      -39     
  Misses        153      153              
  Partials        2        2              
Flag Coverage Δ
#test262 54.02% <100.00%> (+<0.01%) ⬆️
#tests 91.36% <100.00%> (-0.09%) ⬇️
Impacted Files Coverage Δ
polyfill/lib/date.mjs 93.88% <100.00%> (ø)
polyfill/lib/datetime.mjs 95.65% <100.00%> (ø)
polyfill/lib/duration.mjs 98.97% <100.00%> (ø)
polyfill/lib/ecmascript.mjs 97.47% <100.00%> (-0.07%) ⬇️
polyfill/lib/time.mjs 99.33% <100.00%> (ø)
polyfill/lib/yearmonth.mjs 97.43% <100.00%> (ø)

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 d623af6...ca883a8. Read the comment docs.

Fixes tc39#609. Removes non-`Duration` use of `{disambiguation: 'balance'}`
from spec, because all known use-cases are better served by using
`plus()` instead.

Note that balancing _behavior_ is still present, because it's needed for
`plus` and `minus`.  it's just the `balance` disambiguation option for
`with` and `from` that's being removed for non-Duration types.

`Duration` still retains this option, because (unlike other types),
there's a need to create and persist unbalanced `Duration` instances.

Now, non-Duration arithmetic options have the same disambiguation
options as `from` and `with`. Abstract operations were renamed
accordingly.
@justingrant justingrant force-pushed the non-duration-balance-option branch from eab70dd to 11d5f3b Compare June 2, 2020 16:28
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.

With one small change, I think this is OK. Might be good to see if anyone else weighs in since we've only heard from me and @pipobscure

docs/date.md Outdated Show resolved Hide resolved
@sffc
Copy link
Collaborator

sffc commented Jun 3, 2020

I have no strong opinion on this and am happy to defer to the other champions.

@sffc sffc removed their request for review June 3, 2020 01:00
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Just a few comments.

docs/date.md Outdated Show resolved Hide resolved
spec/duration.html Show resolved Hide resolved
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.

Suggestion: remove disambiguation: 'balance' for non-Duration types
4 participants