Allow TimeZoneProvider to be configurable via the ContextBuilder#4447
Conversation
Test262 conformance changes
|
|
NOTE: the tests are not from this PR, but the |
HalidOdat
left a comment
There was a problem hiding this comment.
Nice work! Looks good to me! :)
jedel1043
left a comment
There was a problem hiding this comment.
Looking nice! Though, I have some suggestions.
Also, it seems like we expose this as the timezone_provider, but the trait is called TimeZoneProvider, so it might be better to expose it as the time_zone_provider?
|
Probably blocked on boa-dev/temporal#599 |
|
This is unblocked now |
0952a79 to
a31e5be
Compare
03327d3 to
8904d71
Compare
| Ok(self) | ||
| } | ||
|
|
||
| /// Set the [`timezone_provider::provider::TimeZoneProvider`] that should be used to source |
There was a problem hiding this comment.
s/timezone/time_zone
It is also in other places such as the name of the getter in Context or the debug implementation of ContextBuilder and Context.
There was a problem hiding this comment.
I've gone back and forth on this (partially why the name is different). I think timezone_provider makes sense for consistency with the crate, but there is also a good argument for time_zone_provider as that is what is implied by the pascal case.
There was a problem hiding this comment.
I prefer timezone_provider imo, it looks nicer lol
There was a problem hiding this comment.
That's what I went with too. 🤣 Something about time_zone_provider just does not seem right.
This is related to work on #1804
It changes the following:
DynamicTimeZoneProvider.TimeZoneProvideronContextBuilder.This is related to the general API, so please feel free to let me know if you think this is the wrong approach.
Assuming we're happy with the result of this PR and its merged, I would be fine removing Temporal from the
experimentalfeature flags for the next release. We still need to figure out a good way to make reading the host system time zone accessible, but that's still an odd aspect of the specification itself.