Add ZonedDateTime FFI#329
Conversation
Manishearth
force-pushed
the
zdt
branch
3 times, most recently
from
0499ab6 to
783037b
Compare
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Have a couple questions after looking at this
| } | ||
|
|
||
| /// <https://tc39.es/proposal-temporal/#sec-getavailablenamedtimezoneidentifier> but just a getter | ||
| pub fn is_valid_with_provider(&self, provider: &impl TimeZoneProvider) -> bool { |
There was a problem hiding this comment.
question: This may change in the future depending on changes to TimeZoneProvider and the trait implementations. Is this volatility going to be fine?
There was a problem hiding this comment.
Personally I don't care about volatility in temporal_rs; the primary client is boa and as long as that volatility is fine with you it's fine with me.
temporal_capi is where I worry.
There was a problem hiding this comment.
I'm fine with volatility for now due to it being flagged experimental in Boa. I think this method could work for now, but I really don't expect it to last for long once the providers start to take shape and the TimeZoneProvider is updated to handle GetAvailableNamedTimeZones better than it currently can.
(Also, as far as temporal_capi, see my other question about compiled_data)
| @@ -0,0 +1,391 @@ | |||
| #[cfg(feature = "compiled_data")] | |||
There was a problem hiding this comment.
question: I had always thought that these APIs would be more aligned to the with_provider versions. Is using the compiled data version more of a short term approach?
There was a problem hiding this comment.
I think the compiled data APIs are useful in the short and long term. In the short term they enable me to write a working implementation without worrying about the provider stuff not being done. In the long term they allow other clients of this code to not worry about a zoneinfo source (you already have a temporal_capi client that is not v8: rust-diplomat/diplomat#880 (comment)).
My own plan for V8 is:
- Use compiled_data APIs for now while filling in the implementation
- Wait for there to be a "final" provider API in temporal_rs (or help make it happen)
- Wait for there to be a provider impl that accepts zoneinfo files
- At some point, design a Provider API for FFI
- Figure out how to tie it all in together
So for now I'm using non-provider stuff. I'm reluctant to add provider over FFI yet since I know it's going to change a bunch.
There was a problem hiding this comment.
Perfect, just wanted to confirm. Thanks!
There was a problem hiding this comment.
So this means compiled_data is a longterm thing that is helpful for other users too. As far as volatility is concerned, while V8 is using these APIs I'd like prior warning when they change (and ideally, they do not change), but I don't really expect them to change much.
There was a problem hiding this comment.
while V8 is using these APIs I'd like prior warning when they change
Can do. I don't expect the APIs to change. But the underlying provider might (we are at least a few weeks off from that though probably). I'll make sure to keep you in the loop.
github-project-automation
Bot
moved this from In review
to Done
in Temporal Implementation
3 participants
Based on #326Based on #327Fills in FFI for ZDT. Where there are missing provider methods I added some.
I was hoping to tackle ZDT after the provider stuff is ready, but I decided instead to have the full API surface so I can fill everything in on V8, and we can revisit things later. I have deliverately attempted to design the FFI API such that it is overall immune to provider changes.
One major difference from the temporal_rs ZDT API is that most of the getters are infallible and have GIGO behavior on errors. See #328