Implementation of ZonedDateTime.prototype.with

[lockels]

Fixes #260

[lockels]

Still unfinished, but this mostly works i think.

[nekevss]

suggestion: change this to a normal method over a macro.

Since this is a one off and can't really take advantage of the other macro, it would probably be better for this to be a direct method implementation on PartialDate so that it's easy to read and reason about in the future.

[lockels]

31/38 out of the current tests pass with my last commit. I'm unsure why 4 of the tests are saying this

Uncaught Error: Not yet implemented

This one for example

overflow-options.js

const zdt = new Temporal.PlainDateTime(1976, 11, 18, 15, 23, 30, 123, 456, 789).toZonedDateTime("UTC");

const overflow = "constrain";
TemporalHelpers.assertZonedDateTimesEqual(
    zdt.with({ month: 29 }, { overflow }),
    Temporal.ZonedDateTime.from("1976-12-18T15:23:30.123456789+00:00[UTC]"));
TemporalHelpers.assertZonedDateTimesEqual(
    zdt.with({ day: 31 }, { overflow }),
    Temporal.ZonedDateTime.from("1976-11-30T15:23:30.123456789+00:00[UTC]"));
TemporalHelpers.assertZonedDateTimesEqual(
    zdt.with({ hour: 29 }, { overflow }),
    Temporal.ZonedDateTime.from("1976-11-18T23:23:30.123456789+00:00[UTC]"));
TemporalHelpers.assertZonedDateTimesEqual(
    zdt.with({ nanosecond: 9000 }, { overflow }),
    Temporal.ZonedDateTime.from("1976-11-18T15:23:30.123456999+00:00[UTC]"));

and this one

overflow-reject-throws.js

const zdt = new Temporal.PlainDateTime(1976, 11, 18, 15, 23, 30, 123, 456, 789).toZonedDateTime("UTC");

const overflow = "reject";
assert.throws(RangeError, () => zdt.with({ month: 29 }, { overflow }));
assert.throws(RangeError, () => zdt.with({ day: 31 }, { overflow }));
assert.throws(RangeError, () => zdt.with({ hour: 29 }, { overflow }));
assert.throws(RangeError, () => zdt.with({ nanosecond: 9000 }, { overflow }));

[lockels]

I haven't inspected it yet, but some overflow option tests are not doing what they should, might be an erorr in the implementation of 5.5.5 InterpretTemporalDateTimeFields

Overflow reject option here for example doesn't give the error variant.

        let provider = &FsTzdbProvider::default();
        let zdt =
            ZonedDateTime::try_new(217178610123456789, Calendar::default(), TimeZone::default())
                .unwrap();

        let overflow = ArithmeticOverflow::Reject;

        let result_2 = zdt.with(
            PartialZonedDateTime {
                date: PartialDate {
                    day: Some(31),
                    ..Default::default()
                },
                time: PartialTime::default(),
                offset: None,
                timezone: None,
            },
            None,
            None,
            Some(overflow),
            provider,
        );

        assert!(result_2.is_err()); // Fails

[nekevss]

overflow-options.js

These are easy to answer. It looks like the implementation hasn't been "turned on" in Boa. That's partially my bad. I had a PR open yesterday to uncomment one of these methods, and I really should have searched for others. If you uncomment the lines in Boa, then it should hopefully pass 😄

The last case may genuinely be a bug that could be addressed. I haven't looked into it too much yet. If you want to dig into it, feel free to.

[lockels]

I haven't inspected it yet, but some overflow option tests are not doing what they should, might be an error in the implementation of 5.5.5 InterpretTemporalDateTimeFields

Managed to fix this bug, was a simple error in my own code. I was previously always passing Constriain to date_from_partial rather than the overflow parameter 😅

[lockels]

overflow-options.js

These are easy to answer. It looks like the implementation hasn't been "turned on" in Boa. That's partially my bad. I had a PR open yesterday to uncomment one of these methods, and I really should have searched for others. If you uncomment the lines in Boa, then it should hopefully pass 😄

The last case may genuinely be a bug that could be addressed. I haven't looked into it too much yet. If you want to dig into it, feel free to.

Thanks a lot, i honestly didn't think of that being the issue at all! But that seemed to fix it

[nekevss]

I have one thought / question, it's not necessarily blocking (although I guess it is softly blocking due to curiosity on my part), but I'd be curious to hear your thoughts prior to approval / merge.

[nekevss]

thought (non-blocking): this strikes me as a bit curious

Now that I'm looking at this in a bit more detail, I'm noticing that we have with_fallback_zoneddatetime, but then we call get_iso_datetime_for immediately after. Did you consider using the return value of get_iso_datetime_for at all to calculate the fallback at all?

[nekevss]

Looks great! Thanks again for working on this! 😄