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

Rename Temporal.Absolute to Temporal.Instant #936

Closed
wants to merge 0 commits into from
Closed

Rename Temporal.Absolute to Temporal.Instant #936

wants to merge 0 commits into from

Conversation

zolotyh
Copy link

@zolotyh zolotyh commented Sep 23, 2020

@ryzokuken said that I can help here. Looks like the pull request belongs to #602 but I am not sure. Feel free to decline

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #936 into main will not change coverage.
The diff coverage is 98.01%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #936   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          17       17           
  Lines        5671     5671           
  Branches      856      856           
=======================================
  Hits         5305     5305           
  Misses        359      359           
  Partials        7        7           
Flag Coverage Δ
#test262 50.15% <49.42%> (ø)
#tests 88.98% <71.28%> (ø)

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

Impacted Files Coverage Δ
polyfill/lib/timezone.mjs 95.16% <93.54%> (ø)
polyfill/lib/datetime.mjs 94.97% <100.00%> (ø)
polyfill/lib/ecmascript.mjs 95.46% <100.00%> (ø)
polyfill/lib/instant.mjs 97.29% <100.00%> (ø)
polyfill/lib/intl.mjs 99.19% <100.00%> (ø)
polyfill/lib/now.mjs 100.00% <100.00%> (ø)
polyfill/lib/temporal.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 d11e8c8...a60809b. Read the comment docs.

@ptomato
Copy link
Collaborator

ptomato commented Sep 24, 2020

Thanks very much, this is very helpful! I'll take a look at it ASAP.

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.

Thanks very much for doing this giant rename! I've looked through it and I saw a few missed ones, which is entirely understandable with such a huge rename! If you don't have time to do those, no worries. The fact that the bulk of it is done already in this PR has already been incredibly helpful.

We still need to rename everything in the spec as well. Are you interested in doing that or should we save that for a follow up PR?

docs/instant.md Outdated Show resolved Hide resolved
docs/instant.md Outdated Show resolved Hide resolved
docs/instant.md Outdated Show resolved Hide resolved
docs/instant.md Outdated Show resolved Hide resolved
docs/instant.md Outdated Show resolved Hide resolved
polyfill/test/now/date/calendar-function.js Outdated Show resolved Hide resolved
polyfill/test/now/date/calendar-object.js Outdated Show resolved Hide resolved
polyfill/test/usercalendar.mjs Outdated Show resolved Hide resolved
polyfill/test/usertimezone.mjs Outdated Show resolved Hide resolved
polyfill/test/validStrings.mjs Outdated Show resolved Hide resolved
@zolotyh
Copy link
Author

zolotyh commented Sep 24, 2020

Thx for so detailed review. I've changed all places which you mentioned and merged conflicts. Waiting for the automatic tests

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.

Thanks! I found a few more leftovers, but in general I think we should just merge this even if there are still a few places that it says "absolute" and fix the rest later. Otherwise rebasing it will get painful.

We might decide to refer to "exact time" instead of "instant time" (only in the documentation text, not the API!) but that's not decided yet, it's being discussed in #926.

docs/datetime.md Outdated
@@ -719,7 +719,7 @@ This method overrides `Object.prototype.valueOf()` and always throws an exceptio
This is because it's not possible to compare `Temporal.DateTime` objects with the relational operators `<`, `<=`, `>`, or `>=`.
Use `Temporal.DateTime.compare()` for this, or `datetime.equals()` for equality.

### datetime.**toAbsolute**(_timeZone_ : object | string, _options_?: object) : Temporal.Absolute
### datetime.**toAbsolute**(_timeZone_ : object | string, _options_?: object) : Temporal.Instant
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be renamed as well:

Suggested change
### datetime.**toAbsolute**(_timeZone_ : object | string, _options_?: object) : Temporal.Instant
### datetime.**toInstant**(_timeZone_ : object | string, _options_?: object) : Temporal.Instant

docs/instant.md Outdated
Comment on lines 541 to 542
startAbsolute: Temporal.Instant.from('2020-03-30T15:00-04:00[America/New_York]'),
endAbsolute: Temporal.Instant.from('2020-03-30T16:00-04:00[America/New_York]')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
startAbsolute: Temporal.Instant.from('2020-03-30T15:00-04:00[America/New_York]'),
endAbsolute: Temporal.Instant.from('2020-03-30T16:00-04:00[America/New_York]')
startInstant: Temporal.Instant.from('2020-03-30T15:00-04:00[America/New_York]'),
endInstant: Temporal.Instant.from('2020-03-30T16:00-04:00[America/New_York]')

docs/instant.md Outdated
@@ -555,14 +554,14 @@ console.log(str);

// To rebuild from the string:
function reviver(key, value) {
if (key.endsWith('Absolute')) return Temporal.Absolute.from(value);
if (key.endsWith('Absolute')) return Temporal.Instant.from(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (key.endsWith('Absolute')) return Temporal.Instant.from(value);
if (key.endsWith('Instant')) return Temporal.Instant.from(value);

@@ -87,15 +87,15 @@ But since it represents an unambiguous moment in time (like Absolute, and unlike
To create one, you use `Temporal.LocalDateTime.from`, or the `toLocalDateTime()` method of another Temporal type.
- From a string: `from(string)`
- From raw DateTime fields: `from({ year, month, day, etc., timeZone, timeZoneOffsetNanoseconds })`
- From an Absolute: `absolute.toLocalDateTime(timeZone)`
- From an Absolute: `instant.toLocalDateTime(timeZone)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- From an Absolute: `instant.toLocalDateTime(timeZone)`
- From an Instant: `instant.toLocalDateTime(timeZone)`

polyfill/lib/now.mjs Outdated Show resolved Hide resolved
polyfill/test/now.mjs Outdated Show resolved Hide resolved
@Ms2ger Ms2ger closed this Sep 25, 2020
@Ms2ger
Copy link
Collaborator

Ms2ger commented Sep 25, 2020

Github did something stupid - I'll deal with it.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Sep 25, 2020

Merged as #940 - thanks @zolotyh !

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