Throw if difference() is out-of-order & stop limiting Time.prototype.difference to 12 hours
#667
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #667 +/- ##
==========================================
- Coverage 94.43% 94.42% -0.01%
==========================================
Files 17 17
Lines 4760 4757 -3
Branches 749 752 +3
==========================================
- Hits 4495 4492 -3
Misses 262 262
Partials 3 3
Continue to review full report at Codecov.
|
Closed
justingrant
force-pushed
the
difference-out-of-order-exception
branch
from
2c9b046
to
6ff4c9a
Compare
ptomato
requested changes
Jun 12, 2020
|
@ptomato - Would you prefer me to merge or rebase+force-push to resolve the conflicts with your latest calendar checkins? Also I think I've resolved all your review comments except the one waiting for a reply. |
|
Rebase and force push is what I usually do! |
justingrant
force-pushed
the
difference-out-of-order-exception
branch
from
c589c47
to
8984f3a
Compare
Fixes tc39#663. Changes `difference()` methods to throw if `other`is larger than `this`. Goal is encourage feedback about long-term solution: option 1 - previous behavior: `difference()` returns an absolute value option 2 - behavior in this commit: throw if other is larger option 3 - negative durations (tc39#558) Tests are updated to make it easy to choose any of the three options. Only a few tests were functionally changed (e.g. to add throws() tests). The rest of test changes were reversing arguments so that choosing any of the options later will break relatively few tests. Because `difference` is now unidirectional, it doesn't make sense to restrict `Time.prototype.difference` to 12 hours max. Fixes tc39#597.
justingrant
force-pushed
the
difference-out-of-order-exception
branch
from
8984f3a
to
9bec1d4
Compare
ptomato
approved these changes
Jun 15, 2020
This was referenced Aug 8, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR:
a) Changes
difference()to throw ifotheris larger thanthis, which prevents users from accidentally treating invalid negative differences as valid positive differences. Fixes #663.b) Because
difference()is now unidirectional, it doesn't make sense to restrictTime.prototype.differenceto 12 hours max. Fixes #597. Ideally (a) and (b) would be separate PRs, but the implementation was hard to split so I combined into one PR.Although (a) will likely be considered non-ideal ergonomics, the exception will likely be helpful to generate feedback from polyfill early adopters about what they want the behavior to be and what they think is the best long-term option, e.g.:
difference()returns an absolute valueThis PR updates tests to make it easy to choose any of the three options in a later release. The (few) tests for option 1 are commented out but still checked in to make it easy to choose option 1 later. The rest of test changes were reversing arguments which are compatible with all three options. This means that choosing either (1) or (3) later will break few existing tests.