Should the API for creating the date components be updated?

[nekevss]

trafficstars

Currently, to create one of the structs, our general API is new with an overflow option. It may be better to have a more "rusty" API; i.e., we lack a try_new method.

General Background:

The Temporal specification provides the option to Constrain or Reject the creation of an object with RegulateX abstract methods. To capture this, the current new methods all have an option parameter that allows choosing the overflow option.

While the current new method provides those options, it also means the API doesn't follow typical conventions that may be expected by any Rust users and may also be a little vague.

Initial Proposal:

Essentially, my thinking is that we should add a try_new method and update the API to be a bit more explicit.

• new(fields..., overflow_option) => new_with_overflow_option(fields, overflow_option)
• Add try_new(fields ...) that calls new_with_overflow_option(fields..., REJECT)
• Add new(fields...) that calls new_with_overflow_option(fields..., CONSTRAIN)

That's sort of along the lines of what I was thinking, but I thought I'd bring it up for discussion. I'm not exactly in love with the idea of new being constrain by default, but I think it'd be fine as long as it's documented.

Any thoughts?

[jasonwilliams]

Are you referring to this? https://github.com/boa-dev/temporal/blob/a7fc946480683074d866490dc75c110688ffdd8e/src/iso.rs#L308-L314

Yeah I guess this could work, I don't have any issue with the idea, I'm not sure it's immediately obvious though so it would need some documentation around it. new_with_overflow might be enough (to keep the name shorter). If I just saw try_new and new on their own I probably wouldn't figure out the try_new is "attempt with constrain and fail if it doesn't fit".

That's sort of along the lines of what I was thinking, but I thought I'd bring it up for discussion. I'm not exactly in love with the idea of new being constrain by default, but I think it'd be fine as long as it's documented.

Yep same, it feels odd, with some documentation we may be OK. I actually don't mind how it is right now, its explicit

[nekevss]

Are you referring to this?

More so around the specific components like Date, Time, and DateTime, so each of these would end up with something akin to the below.

impl Date {
    // Creates a `Date`, constraining to the nearest valid Date if the values are not in a valid range.
    fn new(
        year: i32,
        month: i32,
        day: i32,
        calendar: Calendar,
    ) -> Self {
         Date::new_with_option(feilds, ArithmeticOverflow::Constrain)
             .expect("Constraining cannot return an error.")
    }

    /// Tries to create a new Date, throwing an error if the values are not in a valid Date range.
    fn try_new(
        year: i32,
        month: i32,
        day: i32,
        calendar: Calendar,
    ) -> TemporalResult<Self> {
         Date::new_with_option(feilds, ArithmeticOverflow::Reject)
    }

    /// Create a `Date` with a provided arithmetic overflow option.
    fn new_with_option(
        year: i32,
        month: i32,
        day: i32,
        calendar: Calendar,
        option: ArithmeticOverflow,
    ) -> TemporalResult<Self> {
         todo!()
    }
}

If I just saw try_new and new on their own I probably wouldn't figure out the try_new is "attempt with constrain and fail if it doesn't fit".

try_new wouldn't attempt to constrain first beforehand. It would be basically calling new_with_option with the option automatically set to reject, so it would reject instead of constraining.

[jasonwilliams]

Sorry yes that’s what I meant (that try_new just calls it with reject).

Would these be convenience methods with new_with_option being public also? Or does the user only have access to try/try_new?

[nekevss]

I think new_with_option would still be public, and the other methods would mostly be convenience for the native Rust API.

We could also feature flag them with a feature as default that then allows us to remove them when implementing the engine as they'd probably not be needed.

[jasonwilliams]

I would be fine with this change, if its documented well enough

[nekevss]

Closed with #97 and superseded to a degree by #165