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

Remove Instant.toDateTime{,ISO}(), DateTime.toInstant() #1088

Merged
merged 3 commits into from
Nov 2, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Nov 2, 2020

These conversions are redundant, they either go through ZonedDateTime or
TimeZone.

Cookbook examples that used these conversions are changed, where possible,
to use ZonedDateTime. Where that is not possible because of unimplemented
ZonedDateTime methods, we add a FIXME note and fall back to the more
verbose TimeZone conversion methods.

Updating some of the cookbook examples is taken from #700.

Closes: #1026

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #1088 into main will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1088      +/-   ##
==========================================
- Coverage   93.59%   93.55%   -0.05%     
==========================================
  Files          19       19              
  Lines        7702     7683      -19     
  Branches     1224     1221       -3     
==========================================
- Hits         7209     7188      -21     
- Misses        486      488       +2     
  Partials        7        7              
Flag Coverage Δ
test262 41.86% <ø> (+0.75%) ⬆️
tests 89.52% <ø> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
polyfill/lib/datetime.mjs 95.65% <ø> (-0.04%) ⬇️
polyfill/lib/instant.mjs 96.79% <ø> (-0.12%) ⬇️
polyfill/lib/ecmascript.mjs 96.47% <0.00%> (-0.08%) ⬇️

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 e0ce75b...9b182c5. Read the comment docs.

These conversions are redundant, they either go through ZonedDateTime or
TimeZone.

Cookbook examples that used these conversions are changed, where possible,
to use ZonedDateTime. Where that is not possible because of unimplemented
ZonedDateTime methods, we add a FIXME note and fall back to the more
verbose TimeZone conversion methods.

Updating some of the cookbook examples is taken from #700.

Co-authored-by: Justin Grant <[email protected]>

Closes: #1026
@ptomato ptomato force-pushed the 1026-remove-instant-plaindatetime-conversion branch from 6f5ac9c to 0129d4f Compare November 2, 2020 01:11
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.

Nice!

@Ms2ger Ms2ger merged commit fddb208 into main Nov 2, 2020
@Ms2ger Ms2ger deleted the 1026-remove-instant-plaindatetime-conversion branch November 2, 2020 12:46
@Ms2ger
Copy link
Collaborator

Ms2ger commented Nov 2, 2020

Travis is having issues, so I tested locally before merging.

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.

Consider removing Instant-DateTime conversion methods
3 participants