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 obsolete legacy Date cookbook examples #1930

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Nov 18, 2021

I found these 3 cookbook examples were out of date, and two of them were
actually not included in the cookbook (but still executed together with
the cookbook tests.)

The examples were written before we had ZonedDateTime and also before we
had Date.prototype.toTemporalInstant().

I've consolidated them into 2 files, "fromLegacyDate" and "toLegacyDate",
and updated the accompanying explanations.

@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #1930 (530fa45) into main (1bcbb69) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1930   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files          19       19           
  Lines       10940    10940           
  Branches     1732     1732           
=======================================
  Hits        10395    10395           
  Misses        530      530           
  Partials       15       15           
Flag Coverage Δ
test262 81.79% <ø> (ø)
tests 89.85% <ø> (ø)

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


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 1bcbb69...530fa45. Read the comment docs.

@ptomato ptomato added the no-spec-text PR can be ignored by implementors label Nov 19, 2021
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.

OK, I have some opinionated comments. 😄 The TL;DR is that a bad pattern from legacy Date is confusing UTC and local-TZ values, and we want to be really careful in our own docs (including cookbook) that we don't accidentally encourage the same confusion with Temporal.

docs/cookbook.md Outdated Show resolved Hide resolved
docs/cookbook.md Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/toLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
docs/cookbook/fromLegacyDate.mjs Outdated Show resolved Hide resolved
I found these 3 cookbook examples were out of date, and two of them were
actually not included in the cookbook (but still executed together with
the cookbook tests.)

The examples were written before we had ZonedDateTime and also before we
had Date.prototype.toTemporalInstant().

I've consolidated them into 2 files, "fromLegacyDate" and "toLegacyDate",
and updated the accompanying explanations.
@ptomato
Copy link
Collaborator Author

ptomato commented Nov 23, 2021

@justingrant I didn't incorporate all of the suggestions literally but instead rewrote the examples, and I think I got the spirit of your comments. I'd appreciate another look if you have time.

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.

Much better! Looks great. Thanks!

@ptomato ptomato merged commit fb69aeb into main Nov 24, 2021
@ptomato ptomato deleted the update-legacy-date-cookbook branch November 24, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants