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

Update validation, coercion for all built-in fields. #1319

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Conversation

cjtenny
Copy link
Collaborator

@cjtenny cjtenny commented Jan 21, 2021

Do not merge; RFC only in this state

This change does a few other things as well:

  • Implement era, eraYear for all remaining objects.
  • Implement eraYear/year split behavior for 'gregory' and 'japanese' calendars (NB: choice of anchor/default era in 'japanese' should be revisited with more cultural context).

I only included the specification for the new PrepareTemporalFields abstract operation; I'm still refactoring the spec text into all the other ToTemporal*Fields, ToRelativeTemporalObject, etc abstract operations and that's messy / incomplete at the moment. However, PrepareTemporalFields closely matches how ES.ToRecord was used throughout the polyfill already, so this brings the specification and polyfill much closer together.

There are a number of questions and FIXME comments; I'm also uncertain if the language I used around abstract operation references in a table is valid specification. Bear with me :) Will finish this up over the next day and change.

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #1319 (a8c6dd5) into main (f8fa242) will decrease coverage by 0.82%.
The diff coverage is 71.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
- Coverage   95.61%   94.78%   -0.83%     
==========================================
  Files          19       19              
  Lines        9341     9561     +220     
  Branches     1440     1493      +53     
==========================================
+ Hits         8931     9062     +131     
- Misses        404      489      +85     
- Partials        6       10       +4     
Flag Coverage Δ
test262 54.41% <33.14%> (-1.13%) ⬇️
tests 90.69% <60.84%> (-1.06%) ⬇️

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

Impacted Files Coverage Δ
polyfill/lib/plainyearmonth.mjs 91.66% <41.66%> (-3.09%) ⬇️
polyfill/lib/plaindatetime.mjs 93.38% <42.85%> (-2.08%) ⬇️
polyfill/lib/plainmonthday.mjs 88.82% <63.63%> (-1.82%) ⬇️
polyfill/lib/calendar.mjs 86.95% <72.11%> (-4.41%) ⬇️
polyfill/lib/plaindate.mjs 93.67% <78.57%> (-1.00%) ⬇️
polyfill/lib/zoneddatetime.mjs 97.31% <78.57%> (-0.60%) ⬇️
polyfill/lib/ecmascript.mjs 95.54% <84.88%> (-0.30%) ⬇️

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 f8fa242...3ff286b. Read the comment docs.

@cjtenny
Copy link
Collaborator Author

cjtenny commented Jan 21, 2021

(Validation in getter prototypes is also TODO, but shouldn't be anything surprising given the table)

polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
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.

I think the removal of ToRecord is an improvement in particular!

docs/duration.md Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

Partial review here-- will finish reviewing later today but wanted to get you initial notes ASAP.

polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Show resolved Hide resolved
polyfill/lib/plaindate.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Show resolved Hide resolved
@cjtenny cjtenny changed the title WIP: Update validation, coercion for all built-in fields. Update validation, coercion for all built-in fields. Jan 25, 2021
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.

Would it be possible to separate out the validation changes from the addition of era/eraYear? I think since the latter is newer in this PR, it might take another round of review, whereas the validation changes are basically ready to go with minor comments. The changes for monthCode in #1203 also need the validation, so it would be good to get that merged sooner rather than later.

polyfill/index.d.ts Show resolved Hide resolved
polyfill/index.d.ts Show resolved Hide resolved
polyfill/lib/calendar.mjs Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
spec/intl.html Outdated Show resolved Hide resolved
spec/intl.html Show resolved Hide resolved
spec/plaindatetime.html Outdated Show resolved Hide resolved
spec/plainmonthday.html Outdated Show resolved Hide resolved
spec/plainyearmonth.html Outdated Show resolved Hide resolved
@cjtenny
Copy link
Collaborator Author

cjtenny commented Jan 26, 2021

It would be harder to pull out era/eraYear because the current implementation contains era and year used in inconsistent ways; trying to revert to a base state would have to implement some validation with those, or break things. The era & eraYear portions of this change are small, so hopefully they don't take up too much review surface - I'll address the outstanding bug in the polyfill type lib separately, since it's not related to validation.

@cjtenny cjtenny linked an issue Jan 26, 2021 that may be closed by this pull request
polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
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.

A few minor comments.

spec/abstractops.html Outdated Show resolved Hide resolved
spec/plainmonthday.html Show resolved Hide resolved
spec/plainyearmonth.html Show resolved Hide resolved
spec/plaindatetime.html Outdated Show resolved Hide resolved
spec/zoneddatetime.html Outdated 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.

Safer handling of negative years in non-ISO calendars
4 participants