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

Start filling in the specification around RegulateDate. #364

Merged
merged 2 commits into from
Feb 17, 2020
Merged

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Feb 14, 2020

No description provided.

@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #364 into main will decrease coverage by 0.04%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   84.37%   84.32%   -0.05%     
==========================================
  Files          17       17              
  Lines        3462     3458       -4     
  Branches      408      407       -1     
==========================================
- Hits         2921     2916       -5     
- Misses        505      506       +1     
  Partials       36       36
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 93.71% <72.72%> (-0.13%) ⬇️

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 de41898...07bf2c7. Read the comment docs.

@@ -264,6 +264,13 @@ <h1>NonNegativeModulo ( _x_, _y_ )</h1>
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-constraintorange" aoid="ConstrainToRange">
<h1>ConstrainToRange ( _x_, _minimum_, _maximum_ )</h1>
Copy link
Member

Choose a reason for hiding this comment

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

This kind of “ClampToRange” operation seems like it’d be generally useful in the spec; there's 9 instances of min(max(x, minimum), maximum) in there (and 1 of max(min(x, maximum), minimum)). any interest in making an editorial PR, in advance of Temporal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No interest on my side, but feel free to take the idea and run with it.

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 optional comments

<h1>ConstrainDate ( _year_, _month_, _day_ )</h1>
<emu-alg>
1. Assert: _year_, _month_, and _day_ are integer Number values.
1. Set _year_ to ConstrainToRange(_year_, -999999, 999999).
Copy link
Collaborator

Choose a reason for hiding this comment

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

See #198, I'm not sure it's settled that this is the range of years. Then again, I think this is what the polyfill currently does, so no need to block merging on that.

</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-balancedate" aoid="BalanceDate">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a BalanceDate draft in e79b53a which is in my pull request #333, consider cherry picking that commit into this pull request if it looks reasonable?

[[Month]]: _month_,
[[Day]]: _day_
}.
1. Let _leapYear_ be 1972.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for 1972 in particular?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any leap year will do; this one was close to 1970 which was used in the polyfill before (presumably based on the epoch).

Ms2ger and others added 2 commits February 17, 2020 13:08
The BalanceDate operation was missing from the spec until now. While
translating the current polyfill implementation into a spec, I also
noticed some opportunities for simplification.
@Ms2ger Ms2ger merged commit cf4edf7 into main Feb 17, 2020
@Ms2ger Ms2ger deleted the regulatedate branch February 17, 2020 14:03
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