-
Notifications
You must be signed in to change notification settings - Fork 157
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: Refactor common alg of since/until into AOs #2167
Conversation
6e5027a
to
406c8cc
Compare
Codecov Report
@@ Coverage Diff @@
## main #2167 +/- ##
=======================================
Coverage 80.47% 80.47%
=======================================
Files 17 17
Lines 10886 10886
Branches 1468 1468
=======================================
Hits 8760 8760
Misses 2083 2083
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Philip Chimento <philip.chimento@gmail.com>
Co-authored-by: Philip Chimento <philip.chimento@gmail.com>
Co-authored-by: Philip Chimento <philip.chimento@gmail.com>
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Same story here, I spotted a few elements out of place, I'll just accept those suggestions right away and merge the PR in order not to hold it up any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few issues, otherwise looks great. I'm really pleased with how this PR has turned out. I really like how ~since~
and ~until~
... makes it really clear why the sign value is what it is.
Also it looks like some lint errors to resolve?
Thanks Frank for putting in the time to refactor these complex AOs... they're some of the most difficult parts of Temporal and it's good to have them more DRY now.
spec/instant.html
Outdated
</dl> | ||
<emu-alg> | ||
1. Set _other_ to ? ToTemporalInstant(_other_). | ||
1. If _operation_ is ~until~, then, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, can we use the same logic for until vs. since handling that's used in all other AOs (especially DifferenceTemporalPlainTime
whose behavior should be almost identical to this one except for overflow handling) ?
Specifically, removing this swapping of _other_
and _instant_
and then using logic like we use elsewhere. Like this:
1. If _operation_ is ~since~, let _sign_ be -1. Otherwise, let _sign_ be 1.
then...
1. If _operation_ is ~since~, then
1. Set _roundingMode_ to ! NegateTemporalRoundingMode(_roundingMode_).
and finally:
1. Let _result_ be ! BalanceDuration(0, 0, 0, 0, 0, 0, `sign` × _roundedNs_, _largestUnit_).
If there's a perf or other reason that this AO should use the swapping logic, then why doesn't DifferenceTemporalPlainTime
have the same optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you are asking is not a refactoring of the current logic, but rather a different math. I am not opposing to do that but I lack of confidence to that kind of change. Just look at the current spec:
https://tc39.es/proposal-temporal/#sec-temporal.instant.prototype.since
https://tc39.es/proposal-temporal/#sec-temporal.instant.prototype.round
I simply copy the alg there and factor out. What you suggest is way more complicated than that and I do not feel I have the time to ensure such change is correct.
I am also trubled by the "consistency" ideology. "consistency" will need to end somewhere while the two different type are differeent. It is not avoidable some "consistency" will be broken to deal with type specific issue. WHER the "consistency" should end is the question and I feel we push too deep on this. I think "consistency" should be kept on public API level and someone want it to be in the AO function level. But inside alg of AO? I think that simply go too far.
How about we first land this AO with if/swap . And if you or anyone feel comfortable of changing that logic w/o introduce error (which I do not feel confident) than you can do that later in a separate editorial PR.
I understand the desire for consistency. My desire is to reduce redundancy. The current spec text is both inconsistent and redundant. This PR is aim to reduce only the redundancy w/o introduce additional inconsistency but I do not feel compentent to address pre-existing inconsistency and anyone who desire to improve inconsistency please feel free to submt your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can file a follow-up PR for Instant inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Most of the operations between until and since are the same except negative value. Refactor to shared AOs between them with a direction which either 1 or -1.