Simplify Temporal APIs #3653
Simplify Temporal APIs #3653
Conversation
Test262 conformance changes
|
| assert_eq!(zdt.hour().unwrap(), 1); | ||
| assert_eq!(zdt.minute().unwrap(), 49); | ||
| assert_eq!(zdt.second().unwrap(), 12); | ||
| assert_eq!(zdt.year(&mut ()).unwrap(), 2023); |
There was a problem hiding this comment.
issue: my main concern with this PR would be exactly this sort of API, which may be also where the faults in the previous API was.
My idea approaching this was eventually to have two distinct APIs. One for engine/custom calendar implementors that needed a context and another for someone who would just want to use Temporal as their datetime library. I wasn't in love with contextual_year, but it was also meant to abstract as much of that API away from any potential users in the future that would just want to use the built-in calendars or a custom without a context. These base API's could then be feature flagged down the line (something like #[cfg(not(feature ="custom"))] to remove them from the binary.
contextual_* was meant to be a signal for any engine/custom calendar implementor. Maybe not in the best way, but I was definitely hoping to iron it out before we released this.
Granted, this all may be overthinking/premature abstraction or a wrong approach on my part. But I do think something of the sort should be considered especially if we were going to externalize the crate. Any thought regarding this?
There was a problem hiding this comment.
Yeah, I'm also shifting between the two approaches. The main rationale for doing this was that many C and C++ APIs take a void* data parameter to pass context, and it's just second nature for users to pass NULL if they don't need additional context. However, (as far as I'm aware) this is also uncommon to do in Rust libraries, which is why I'm still unsure about this.
Though, I also think we can offer high level types that are just wrappers around the "complex" date types:
struct Date(CustomDate<()>);
impl Date {
pub fn add_date(&self, duration: &Duration, overflow: ArithmeticOverflow) -> TemporalResult<Self> {
self.0.add_date(duration, overflow, &mut ()).into()
}
}I'll unroll those changes, but I'll probably experiment a bit with this API in a separate branch to see what results in following this alternative design.
There was a problem hiding this comment.
Oh wait, I remembered why I removed the methods instead of just changing them! Since now all methods take &mut CalendarProtocol::Context, we cannot pass a &mut () to them on the general case. An idea I had was to have the current API but with a C::Context: Default bound on the impl, but that seemed very iffy because we could accidentally call any of the context-free methods on Date<JsObject> and it would just create a brand new Context for each call...
EDIT: Nope, just some confusion from my side. The old methods should be up in a sec.
There was a problem hiding this comment.
The main rationale for doing this was that many C and C++ APIs take a
void* dataparameter to pass context, and it's just second nature for users to passNULLif they don't need additional context.
That is a good point. Hmmmmm.
As a counter, it may also be worth just avoiding that passing NULL convention as a whole, and just make sure it is extremely well documented that they should use the context API for that. In other words, I'm not sure making a Rust API more C-like is necessarily the best way. And over time, more and more Rust-based APIs will probably have a FFI with C/C++ so making something more "Rust-like" so to speak isn't necessarily bad and will probably be adapted to.
Granted, we should also make sure explicitly document that choice.
I also think we can offer high level types that are just wrappers around the "complex" date types:
Yeah, wrappers is another viable approach I had thought about. I think I'd actually prefer a wrapper approach even more as it would be nice to abstract away Date<()> for any users that wouldn't want or care about a custom calendar.
That sounds good to me overall. As said above, I really want to have the API in a bit more of a stable, agreed upon place. But I've mostly been trying to focus on implementing as much of the spec as possible.
I think there's probably 3 layers to consider:
Date.year()(fully abstracted)Date<Custom>.year()(custom calendar without needed context)Date<Custom>.year_with_context(&mut Context)(custom calendar with context)
| /// A Custom `Date` Type for an associated `CalendarProtocol`. Default `Date<C>` | ||
| type Date: IsoDateSlots + GetCalendarSlot<C> + Clone + core::fmt::Debug; | ||
| type Date: IsoDateSlots + GetCalendarSlot<Self> + Clone + core::fmt::Debug; |
There was a problem hiding this comment.
Praise: this is so much better! Let's go with this! 😄
I'm aware that this mostly undoes some API design decisions @nekevss and I did, but I was experimenting with simplifying the API and I think I prefer the simplicity of having a single contextual low level call for all methods, instead of a contextual and a non-contextual variant. If we do want to offer a protocol free implementation, I think it would be better to just make simple wrappers around the low level types.
On the other hand, this also replaces the
Anys with an associated type insideCalendarProtocolitself. This wasn't possible at the beginning because I'm pretty sure we hadn't introduced the trait when we decided to passAnyto all method calls.@nekevss Let me know what you think of the changes!