Refactor `TemporalFields` into `CalendarFields` #95

nekevss · 2024-08-21T23:30:13Z

This PR refactors TemporalFields into CalendarFields. I'm leaving it as a draft for now as I want to run plug this into Boa and run some tests beforehand.

General Context:

This builds on the work from #89 and #92.

With the partials implemented for the with methods. The values that would be provided can now be accounted for. Primarily, the time fields really didn't need to exist in the TemporalFields representation anymore (along with the TimeZone and offset which will probably be living on the PartialZonedDateTime). Furthermore, the partials were now handling the undefined case of the fields, so the calendar specific fields no longer needed to be an Option. In general, I also just think that this is a better native representation of the fields values in comparison to the previous version.

Furthermore, this PR also implements better handling for Era and Month Codes according to the Intl era and monthCode proposal.

There are a couple points where feedback/bike-shedding would definitely be appreciated. I think the method names in calendar_types.rs are getting a bit unyielding. I am also a bit worried about introducing the CalendarMethods trait, but I do think it makes the ergonomics around passing Date, DateTime, and, eventually, ZonedDateTime as a fallback convenient.

cc: @sffc

src/components/calendar/era.rs

ptomato

I had a look, mostly I lack context though, so take what I say with a grain of salt. It's cool to see the parallel evolution of CalendarFields with the new Calendar Fields Records in the spec.

ptomato · 2024-08-22T22:30:27Z

src/components/calendar.rs

+    /// Returns the calendar week of year value.
+    fn week_of_year(&self) -> TemporalResult<u16>;
+
+    /// Returns the calendar year of week value.
+    fn year_of_week(&self) -> TemporalResult<i32>;


I think these should be Options

ptomato · 2024-08-23T00:41:19Z

src/components/calendar_types.rs

+    pub fn as_month_integer(&self) -> TemporalResult<u8> {
+        ascii_four_to_integer(self.0)
+    }


Not sure I'm understanding what the integer is used for, but if it's the ordinal month number - you can't determine that in lunisolar calendars without knowing the year.

nekevss · 2024-09-11T22:16:09Z

~~Marking this as ready for review. There still may be a couple fixes needed, but I think I'd prefer to move forward with this for now, and do fixes in the follow up.~~

Overall, this is only -6 test262 tests vs. the current version. The primary cause is most likely due to the constraining needed around month and day. (e.g., { year: 2021, month: 99999, day: 500 })

I'm currently thinking of two approaches: switch MonthCode to Month, or CalendarFields will need to take the overflow argument and handle the constrain behavior when constructed (not the biggest fan of the latter).

EDIT: This actually needs a little more work I think

nekevss · 2024-09-26T19:11:41Z

This is more or less ready for review. Would rather move forward with this as is and address the broken tests in the future.

Below are the diff results of running the current version in Boa vs. this PR in Boa.

< Fetching latest test262 commits...
< Test262 switching to commit dde3050bdbfb8f425084077b6293563932d57ebc...
< Reset test262 to commit: dde3050bdbfb8f425084077b6293563932d57ebc...
< HEAD is now at dde3050bdb Fix test regression in asyncHelpers.js (#4197)
8c4
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal:
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/:
1570,1572c1566
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from/one-of-era-erayear-undefined.js`: Passed
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from/one-of-era-erayear-undefined.js` (strict mode): starting
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from/one-of-era-erayear-undefined.js` (strict): Passed
---
> `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from/one-of-era-erayear-undefined.js`: Failed
1724c1718
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from results: total: 56, passed: 42, ignored: 0, failed: 14 , conformance: 75.00%
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/from results: total: 56, passed: 41, ignored: 0, failed: 15 , conformance: 73.21%
2096,2098c2090
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with/overflow-undefined.js`: Passed
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with/overflow-undefined.js` (strict mode): starting
< `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with/overflow-undefined.js` (strict): Passed
---
> `/home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with/overflow-undefined.js`: Failed
2132c2124
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with results: total: 18, passed: 15, ignored: 0, failed: 3 , conformance: 83.33%
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype/with results: total: 18, passed: 14, ignored: 0, failed: 4 , conformance: 77.78%
3438c3430
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype results: total: 460, passed: 281, ignored: 0, failed: 179 , conformance: 61.09%
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate/prototype results: total: 460, passed: 280, ignored: 0, failed: 180 , conformance: 60.87%
3512c3504
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate results: total: 573, passed: 370, ignored: 0, failed: 203 , conformance: 64.57%
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/PlainDate results: total: 573, passed: 368, ignored: 0, failed: 205 , conformance: 64.22%
11590c11582
< Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal results: total: 3855, passed: 1568, ignored: 0, failed: 2287 , conformance: 40.67%
---
> Suite /home/nekevss/Projects/boa/test262/test/built-ins/Temporal/ results: total: 3855, passed: 1566, ignored: 0, failed: 2289 , conformance: 40.62%
11595c11587
< Passed tests: 1568
---
> Passed tests: 1566
11597,11598c11589,11590
< Failed tests: 2287 (0 panics)
< Conformance: 40.67%
---
> Failed tests: 2289 (0 panics)
> Conformance: 40.62%

jasonwilliams · 2024-09-29T15:47:06Z

src/components/calendar/types.rs

+}
+
+// NOTE: This is a greedy function, should handle differently for all calendars.
+pub(crate) fn ascii_four_to_integer(mc: TinyAsciiStr<4>) -> TemporalResult<u8> {


It still looks like this could return the wrong value on lunisolar calendars?

Yeah 😕 it's a rather greedy function that I'd like to come back to in the future. It should be isolated to being called from solely ISO functions so it should be fine.

EDIT: I'd like to come back to it in the future as I'm not exactly sure I'm 100% comfortable yet with implementing month/monthCode logic for ALL calendars and need to look into it more.

jasonwilliams

LGTM nice changes

nekevss added 4 commits August 21, 2024 18:32

Initial work on migrating TemporalFields to CalendarFields

dbc9fed

Further work on CalendarFields concept

9623f90

Complete work on CalendarFields draft

b6cc303

MonthCodeV2 -> MonthCode

955649a

nekevss force-pushed the update-fields-to-calendar-fields branch from 490e190 to 955649a Compare August 21, 2024 23:38

nekevss requested review from jasonwilliams and jedel1043 August 21, 2024 23:43

Move era constants to a separate file

e6a3350

nekevss commented Aug 22, 2024

View reviewed changes

src/components/calendar/era.rs Outdated Show resolved Hide resolved

ptomato reviewed Aug 23, 2024

View reviewed changes

nekevss added 3 commits August 28, 2024 18:11

Adjust according to feedback

8e8e679

Cargo fmt

0a0ff90

Fixes from running branch in boa

30a22e7

nekevss marked this pull request as ready for review September 11, 2024 22:09

nekevss marked this pull request as draft September 12, 2024 13:40

Complete rework of concept into calendar methods - testing TBD

0ce1bef

nekevss marked this pull request as ready for review September 26, 2024 19:09

jasonwilliams reviewed Sep 29, 2024

View reviewed changes

jasonwilliams approved these changes Sep 29, 2024

View reviewed changes

jasonwilliams reviewed Sep 29, 2024

View reviewed changes

nekevss merged commit efa1df0 into main Oct 14, 2024
5 checks passed

jedel1043 deleted the update-fields-to-calendar-fields branch October 16, 2024 15:42

nekevss added the C-api Changes related to the public API label Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor `TemporalFields` into `CalendarFields` #95

Refactor `TemporalFields` into `CalendarFields` #95

Uh oh!

nekevss commented Aug 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

ptomato left a comment

Uh oh!

ptomato Aug 22, 2024

Uh oh!

ptomato Aug 23, 2024

Uh oh!

nekevss commented Sep 11, 2024 •

edited

Loading

Uh oh!

nekevss commented Sep 26, 2024

Uh oh!

jasonwilliams Sep 29, 2024

Uh oh!

nekevss Sep 29, 2024 •

edited

Loading

Uh oh!

jasonwilliams left a comment

Uh oh!

Uh oh!

Uh oh!

Refactor TemporalFields into CalendarFields #95

Refactor TemporalFields into CalendarFields #95

Uh oh!

Conversation

nekevss commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

ptomato Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

ptomato Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

nekevss commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nekevss commented Sep 26, 2024

Uh oh!

jasonwilliams Sep 29, 2024

Choose a reason for hiding this comment

Uh oh!

nekevss Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Refactor `TemporalFields` into `CalendarFields` #95

Refactor `TemporalFields` into `CalendarFields` #95

nekevss commented Aug 21, 2024 •

edited

Loading

nekevss commented Sep 11, 2024 •

edited

Loading

nekevss Sep 29, 2024 •

edited

Loading