Implement `MonthCode`, `PartialDate`, and `Date::with` #89

nekevss · 2024-07-23T01:34:09Z

So the title basically says it all as far as implementation. That being said, there's a lot of interpretation going into this that I'd like feedback on if possible, and it could be the right direction or it could be the wrong direction (although I'm leaning the former over the latter).

The main culprit is basically PrepareTemporalFields.

Background for discussion/sanity check:

Up until recently, I basically thought it would be an implementation detail on the engine/interpreter side, but the thing that always bugged me was the requiredFields parameter, being either a List or PARTIAL. We could probably do that to specification, but we might be providing something like Some(Vec::default()) as an argument, and it basically just felt clunky.

After the recent TemporalFields update, I went to implement the TemporalFields portion of the toX abstract ops in Boa and realized that PARTIAL is never called in the toX operations, and it's actually exclusively called in with methods. We already have a sort of precedence for partials with PartialDuration.

There's some benefits to this: we can have a with method on the native rust side, ideally the complexity that exists in PrepareTemporalFields can be made a bit easier to reason about.

Potential negatives: we might end up deviating from the specification as far as the order of when errors are thrown and observability (TBD...potentially a total non-issue) and this is probably opening up a can of worms around what would be the ideal API for a PartialDate, PartialDateTime, and PartialTime.

That all being said, I think the benefits do most likely outweigh any negatives, and it would be really cool to have with method implementations. I'm just not entirely sure around the API.

Also, there's an addition of a MonthCode enum to make From<X> for TemporalFields implementations easier.

ptomato · 2024-07-23T18:36:24Z

PrepareTemporalFields has always been slightly weird. I am trying to see if it can be simplified if custom calendars are not a thing.

nekevss · 2024-07-23T18:53:36Z

I think it definitely can be split apart. Does the split between a "PartialDateLike" (so to speak) in with methods vs. a required "DateLike" JsObject in the to_temporal_x abstract make sense to the general intent of the proposal? Or am I overlooking something from those codepaths.

ptomato · 2024-07-23T18:57:43Z

Those two operations were combined into PrepareTemporalFields because in both cases, you need to do property Gets on the object (partial date, date-like property bag, or PlainDate). Without custom calendars, you don't need to do the property Gets anymore on PlainDate. But the handling of partial date vs date-like property bag is still very similar. I'm not yet sure of what is going to be the best way to simplify it.

nekevss · 2024-07-23T19:18:40Z

Yeah, the issue I'm running into is that it has to be representable via the type system, if possible, in order to implement in temporal_rs over an implementation specific method. So where the specification may have two JsObjects that can be handled differently in an abstract op because they're functionally different, we would have to treat as a separate struct in temporal_rs. Again, at least if it were to be in temporal_rs vs. a specific engine/interpreter implementation.

Honestly, assuming I'm readying the functionality correctly. This specific split might be easier to reason about than the previous PrepareTemporalFields. Or it could be off base of the intended functionality. I should probably add some unit tests 😅

jasonwilliams · 2024-07-23T21:25:01Z

src/components/mod.rs

+}
+
+impl MonthCode {
+    pub fn as_str(&self) -> &str {


Will these not change in different calendars? Aren't there more month codes in the chinese calendar? such as https://tc39.es/proposal-intl-era-monthcode/

Yeah, I realized that earlier. I was sort of thinking we could just expand the enum, but that's probably not sufficient either.

Maybe this structure is fine for now and we can refactor it to support more in future? What do you think?

I think if you don't have a "leap" flag in the month code then it probably doesn't make sense to have M13... AFAIK no calendars have an M13, if there is a thirteenth month it's always one of M00L through M12L (or maybe M01L through M11L; it's unclear whether M00L and M12L actually exist)

jasonwilliams · 2024-07-23T21:37:35Z

src/components/calendar.rs

    ) -> TemporalResult<Vec<TemporalFieldKey>> {
+        let mut ignored_keys = Vec::default();


Should this vec have a minimal capacity? I think right now it will hold 3 keys before reallocating

Oooh, potentially. I'll check that!

ptomato · 2024-07-25T17:41:05Z

src/components/date.rs

+pub struct PartialDate {
+    pub(crate) year: Option<i32>,
+    pub(crate) month: Option<i32>,
+    pub(crate) month_code: Option<MonthCode>,
+    pub(crate) day: Option<i32>,
+}


What I said in the Temporal meeting about the requirement being "day AND (month OR monthCode) AND (year OR (era AND eraYear))" reminded me that this should probably have optional fields for era and era_year.

Oh, good point. I had meant to add those in one the update and forgot too.

…hods. (#92) This continues the work on implementing `with` methods with the approach from #89 by adding the same type of records to `DateTime` and `Time`.

@sffc

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](https://tc39.es/proposal-intl-era-monthcode/#sec-temporal-isvalidmonthecodeforcalendar). 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

nekevss requested review from jasonwilliams and jedel1043 July 23, 2024 01:46

jasonwilliams reviewed Jul 23, 2024

View reviewed changes

jasonwilliams approved these changes Jul 23, 2024

View reviewed changes

nekevss force-pushed the impl-partial-date branch from 68a2dd5 to 18173dc Compare July 25, 2024 00:20

ptomato reviewed Jul 25, 2024

View reviewed changes

nekevss added 4 commits July 31, 2024 22:07

Implement partial records

58d8e85

Add MonthCode and PartialDate, implement Date::with

653be4f

Add capacity to vec based on feedback

8fb88f4

Apply review feedback

b348d78

nekevss force-pushed the impl-partial-date branch from f9b54e8 to b348d78 Compare August 1, 2024 03:08

nekevss and others added 2 commits July 31, 2024 23:18

Add test and fix bugs found from test

8be2a2e

Merge branch 'main' into impl-partial-date

6613317

nekevss merged commit a7fc946 into main Aug 10, 2024
5 checks passed

jedel1043 deleted the impl-partial-date branch August 10, 2024 18:33

nekevss mentioned this pull request Aug 11, 2024

Add PartialTime and PartialDateTime with corresponding with methods. #92

Merged

nekevss mentioned this pull request Aug 22, 2024

Refactor TemporalFields into CalendarFields #95

Merged

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

Implement `MonthCode`, `PartialDate`, and `Date::with` #89

Implement `MonthCode`, `PartialDate`, and `Date::with` #89

nekevss commented Jul 23, 2024

ptomato commented Jul 23, 2024

nekevss commented Jul 23, 2024 •

edited

Loading

ptomato commented Jul 23, 2024

nekevss commented Jul 23, 2024 •

edited

Loading

jasonwilliams Jul 23, 2024

nekevss Jul 23, 2024

jasonwilliams Jul 24, 2024

ptomato Jul 25, 2024

jasonwilliams Jul 23, 2024

nekevss Jul 23, 2024

ptomato Jul 25, 2024

nekevss Jul 26, 2024

		) -> TemporalResult<Vec<TemporalFieldKey>> {
		let mut ignored_keys = Vec::default();

Implement MonthCode, PartialDate, and Date::with #89

Implement MonthCode, PartialDate, and Date::with #89

Conversation

nekevss commented Jul 23, 2024

ptomato commented Jul 23, 2024

nekevss commented Jul 23, 2024 • edited Loading

ptomato commented Jul 23, 2024

nekevss commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `MonthCode`, `PartialDate`, and `Date::with` #89

Implement `MonthCode`, `PartialDate`, and `Date::with` #89

nekevss commented Jul 23, 2024 •

edited

Loading

nekevss commented Jul 23, 2024 •

edited

Loading