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

Polyfill: Fix daysInYear and inLeapYear in Islamic calendars #2743

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Dec 21, 2023

Fixes #2740. There were two separate problems with the islamic calendar implementation in the Temporal polyfill, both fixed in this PR:

  • Previous implementation assumed that when the 12th month had 30 days, then it was a leap year. But the islamic-umalqura calendar appears to break this rule. New implementation calculates the number of days in the year; if 355 then it's a leap year.
  • Previous implementation had a bug where it was using ISO date instead of calendar date when calculating daysInYear.

@justingrant justingrant marked this pull request as draft December 21, 2023 22:40
@justingrant justingrant added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! no-spec-text PR can be ignored by implementors labels Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (af85ca8) 96.38% compared to head (59f8cae) 96.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2743      +/-   ##
==========================================
+ Coverage   96.38%   96.73%   +0.35%     
==========================================
  Files          21       21              
  Lines       12444    12444              
  Branches     2250     2255       +5     
==========================================
+ Hits        11994    12038      +44     
+ Misses        395      349      -46     
- Partials       55       57       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MohsenAlyafei
Copy link

Great

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.

Seems correct to me. I will try to review the tests soon. BTW, it seems like the new tests are not covering dayOfYear and that's partly why codecov is complaining, but I don't understand why it's also complaining about inLeapYear since the tests are definitely covering that.

@MohsenAlyafei
Copy link

@justingrant as pointed out by @ptomato, the dayOfYear is not yet fixed.

The day of the year ordinal integer is the difference in days between the date in question and the start of the year of that date + 1.

@justingrant
Copy link
Collaborator Author

BTW, it seems like the new tests are not covering dayOfYear and that's partly why codecov is complaining,

@justingrant as pointed out by @ptomato, the dayOfYear is not yet fixed.

Oh! I'll take a look.

justingrant added a commit to justingrant/test262 that referenced this pull request Jan 12, 2024
justingrant added a commit to justingrant/test262 that referenced this pull request Jan 12, 2024
justingrant added a commit to justingrant/test262 that referenced this pull request Jan 12, 2024
justingrant added a commit to justingrant/test262 that referenced this pull request Jan 12, 2024
@justingrant
Copy link
Collaborator Author

I added tests for dayOfYear (and the trivial tests for monthsOfYear too) but I am not able to repro any problems. All the tests pass.

@MohsenAlyafei could you provide an example test case where the code in this PR doesn't correctly calculate dayOfYear for a particular calendar?

Note that the Temporal polyfill uses Intl.DateTimeFormat for its source of calendar data (which in turn gets its calendar calculations from ICU) so if there's a problem with how Intl.DateTimeFormat (and hence ICU) is calculating dates, then there's not much we can do here... the fix will have to happen upstream.

ptomato pushed a commit to justingrant/test262 that referenced this pull request Jan 13, 2024
ptomato pushed a commit to tc39/test262 that referenced this pull request Jan 13, 2024
@ptomato
Copy link
Collaborator

ptomato commented Jan 13, 2024

dayOfYear seems to work correctly here:

d = Temporal.PlainDate.from({year:1445, month:1, day:1, calendar:'islamic-umalqura'})
  // => Temporal.PlainDate <2023-07-19[u-ca=islamic-umalqura]>
d.dayOfYear
  // => 1
d.subtract({days:1}).dayOfYear
  // => 354

@ptomato ptomato merged commit 8fc0dfc into tc39:main Jan 13, 2024
8 checks passed
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 non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Islamic Calendars incorrect dayOfYear
3 participants