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 from() methods. #502

Merged
merged 17 commits into from
Apr 23, 2020
Merged

Update from() methods. #502

merged 17 commits into from
Apr 23, 2020

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Apr 8, 2020

No description provided.

@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #502 into main will increase coverage by 0.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   96.83%   97.56%   +0.72%     
==========================================
  Files          17       17              
  Lines        3953     3986      +33     
  Branches      582      583       +1     
==========================================
+ Hits         3828     3889      +61     
+ Misses        123       95      -28     
  Partials        2        2              
Flag Coverage Δ
#test262 51.99% <62.62%> (-0.48%) ⬇️
#tests 93.21% <97.31%> (+0.99%) ⬆️
Impacted Files Coverage Δ
polyfill/lib/absolute.mjs 100.00% <100.00%> (ø)
polyfill/lib/date.mjs 93.03% <100.00%> (+0.52%) ⬆️
polyfill/lib/datetime.mjs 95.10% <100.00%> (+3.41%) ⬆️
polyfill/lib/duration.mjs 98.18% <100.00%> (+4.49%) ⬆️
polyfill/lib/ecmascript.mjs 97.91% <100.00%> (-0.16%) ⬇️
polyfill/lib/monthday.mjs 98.83% <100.00%> (+0.20%) ⬆️
polyfill/lib/time.mjs 99.28% <100.00%> (+2.81%) ⬆️
polyfill/lib/timezone.mjs 87.91% <100.00%> (+1.20%) ⬆️
polyfill/lib/yearmonth.mjs 96.89% <100.00%> (+0.27%) ⬆️

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 9d17c18...d0d9567. Read the comment docs.

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.

Very few comments, only the one about naming.

Otherwise, maybe someone else should take a look too since I helped with some of the commits.

polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
Ms2ger and others added 15 commits April 23, 2020 13:12
Co-authored-by: Philip Chimento <[email protected]>
Co-authored-by: Philip Chimento <[email protected]>
Each type's from() method (excluding Absolute and TimeZone, which cannot take
property bags) now calls a new abstract operation, ToFooRecord, which takes a
property bag or an object of the actual type, and returns a Record with the
appropriate slots.

If the argument isn't an object, then from() calls ParseFooString directly. We
delete the FooFromString and ToFoo operations.

In either case, from() will then apply the disambiguation (if applicable) and
create the Temporal object.

Co-authored-by: Philip Chimento <[email protected]>
@Ms2ger Ms2ger force-pushed the 232-from-cloning branch from 71d1898 to 6703c26 Compare April 23, 2020 11:13
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 for the extensive PR. Regarding the "same object" comment in the docs, maybe you could clarify by saying that it returns a new object with the same values, but I don't feel strongly about it.

@Ms2ger Ms2ger force-pushed the 232-from-cloning branch from 4116676 to d0d9567 Compare April 23, 2020 12:42
@Ms2ger Ms2ger merged commit 7951727 into main Apr 23, 2020
@Ms2ger Ms2ger deleted the 232-from-cloning branch April 23, 2020 15:25
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