Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Legacy date conversion #957

Merged
merged 4 commits into from
Oct 5, 2020
Merged

Legacy date conversion #957

merged 4 commits into from
Oct 5, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Oct 2, 2020

Closes: #515

Includes a few stragglers from the Absolute→Instant rename and from the addition of round(), that I noticed.

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #957 into main will decrease coverage by 0.04%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
- Coverage   93.93%   93.88%   -0.05%     
==========================================
  Files          17       18       +1     
  Lines        5739     5743       +4     
  Branches      862      863       +1     
==========================================
+ Hits         5391     5392       +1     
- Misses        341      344       +3     
  Partials        7        7              
Flag Coverage Δ
#test262 50.11% <25.00%> (-0.04%) ⬇️
#tests 89.46% <ø> (ø)

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

Impacted Files Coverage Δ
polyfill/lib/legacydate.mjs 0.00% <0.00%> (ø)
polyfill/lib/shim.mjs 100.00% <100.00%> (ø)

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 53a1704...94091a1. Read the comment docs.

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. Perhaps we also need to have a diffed version of the 262 and 402 specs somewhere at some point.

@ryzokuken
Copy link
Member

There's a linter error that needs to be fixed. Also, should we rename initialise to initialize since we tend to prefer American spellings in TC39? 🙈 Not sure, perhaps init is a more natural alternative.

This was forgotten in the rename.
It seems appropriate to do this, now that we have a round() method to
demonstrate.
This is in response to feedback that this conversion should be less
verbose.

No unit tests currently included because I found out that the test code
doesn't import the shim but instead imports individual objects from
lib/temporal.mjs, and I didn't see a quick way to fix that. Anyway, the
method is tested in the cookbook already.

Closes: #515
As suggested in code review, this avoids confusion between different
regional spellings.
@ptomato ptomato force-pushed the 515-legacy-date-conversion branch from a688745 to 94091a1 Compare October 5, 2020 17:05
@ptomato ptomato merged commit d2fcc8b into main Oct 5, 2020
@ptomato ptomato deleted the 515-legacy-date-conversion branch October 5, 2020 17:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better conversion between legacy Date and Temporal
2 participants