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

Normative: Change date AOs to accept mathematical values #2518

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

Aditi-1400
Copy link
Collaborator

This PR fixes issue #2315, by redefining following Date operations to calculate in mathematical values instead of Number values:

  • MakeDay -> ISODateToEpochDays
  • MakeDate -> EpochDaysToEpochMs
  • DaysInYear -> MathematicalDaysInYear
  • DayFromYear -> EpochDayNumberForYear
  • TimeFromYear -> EpochTimeForYear
  • YearFromTime -> EpochTimeToEpochYear
  • InLeapYear -> MathematicalInLeapYear
  • MonthFromTime -> EpochTimeToMonthInYear
  • DayWithinYear -> EpochTimeToDayInYear
  • DateFromTime -> EpochTimeToDate
  • WeekDay -> EpochTimeToWeekDay

These operations would be unified when tc39/ecma262#1087 is fixed

@Aditi-1400 Aditi-1400 requested a review from ptomato March 10, 2023 17:29
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #2518 (cd3ec37) into main (1aa6b80) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2518   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files          20       20           
  Lines       11285    11289    +4     
  Branches     2163     2163           
=======================================
+ Hits        10852    10856    +4     
  Misses        369      369           
  Partials       64       64           

see 1 file with indirect coverage changes

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!

spec/intl.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
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.

Looks good, thanks!.

I might suggest being more specific about what is meant by "(epoch) time", since it could be confusing that we mean epoch milliseconds here, whereas in the rest of Temporal we use epoch nanoseconds. But on the other hand, maybe this is fine — it's very clear already that these are MV duplicates of the ECMA-262 operations, and the names are quite long already without adding "milliseconds" to them.

@Aditi-1400 Aditi-1400 requested a review from gibson042 April 18, 2023 19:15
spec/abstractops.html Outdated Show resolved Hide resolved
Comment on lines +89 to +95
<emu-eqn id="eqn-mathematicalinleapyear" aoid="MathematicalInLeapYear">
MathematicalInLeapYear(_t_)
= 0 if MathematicalDaysInYear(EpochTimeToEpochYear(_t_)) is 365
= 1 if MathematicalDaysInYear(EpochTimeToEpochYear(_t_)) is 366
</emu-eqn>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hate seeing this redundancy, but I don't currently have enough time to determine which parts of it are necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the usage of EpochTimeToEpochYear (vs YearFromTime in ECMA-262) makes it necessary, until someone can go through ECMA-262 and prove that using the MV version exclusively would be unobservable. If this turns out to really be redundant, then removing it will by definition be an unobservable change.

@ptomato
Copy link
Collaborator

ptomato commented Apr 26, 2023

@Aditi-1400 One last thing, would you mind removing .DS_Store and rebasing this on main so that the test262 update is not necessary? (I think it updates test262 to the same commit that it already has on main)

@justingrant
Copy link
Collaborator

Also, please squash into one commit please!

@ptomato
Copy link
Collaborator

ptomato commented Apr 28, 2023

For future reference, this PR achieved consensus at the March 2023 TC39 meeting.

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.

4 participants