Move TimeZoneProvider to timezone_provider crate#526
Move TimeZoneProvider to timezone_provider crate#526Manishearth merged 12 commits intoboa-dev:mainfrom
Conversation
af325bd to
cf448b8
Compare
nekevss
left a comment
There was a problem hiding this comment.
Overall fine with the move. Especially if this is considered more of an intermediate stage.
Left a few general thoughts that I had. Nothing blocking merge. More just notes.
| log = { workspace = true, optional = true } | ||
|
|
||
| # tzdb feature | ||
| tzif = { workspace = true, optional = true } |
| pub(crate) const NS_MIN_INSTANT: i128 = -NS_MAX_INSTANT; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Default)] | ||
| pub struct EpochNanoseconds(pub i128); |
There was a problem hiding this comment.
thought: whenever I've mocked this migration, I always decided against moving this.
Not that the move wouldn't be worthwhile, but it really brings up the question of handling EpochSeconds, which may be something to add in follow-ups. EpochNanoseconds are required by the Temporal specification, but from a generic usability perspective, I'd assume that many operate in seconds.
| #[cfg(feature = "experimental_tzif")] | ||
| pub mod experimental_tzif; | ||
|
|
||
| pub mod epoch_nanoseconds; |
There was a problem hiding this comment.
thought: I've never been sure what to call this module.
EpochNanoseconds is specific to the Unix epoch. This is being reexported from a unix_time module, which also isn't that great of a name ... maybe just a unix module?
There was a problem hiding this comment.
Yeah I generally consider the public API of this crate to still need more work. If you have ideas, I'd love to see them; I don't personally plan to keep cleaning up the structure of the public API.
| fn get_named_tz_epoch_nanoseconds( | ||
| &self, | ||
| identifier: &str, | ||
| local_datetime: IsoDateTime, |
There was a problem hiding this comment.
note: not the biggest fan of IsoDateTime being used here, but I'm not sure that there's a better alternative.
At least it's self contained in timezone_provider. Still might be nice if this could be somehow expressed in seconds and nanoseconds. But I'd also acknowledge that that could cause a lot of confusion.
There was a problem hiding this comment.
The actual use cases all use datetimes; "local seconds" is a confusing concept.
Work towards boa-dev#409 This moves the TimeZoneProvider trait to the timezone_provider crate. There's a lot of stuff that gets moved as a result. Some important things to note: - TimeZoneProvider now operates on its own IsoDateTime type - TimeZoneProvider produces UtcOffsetSeconds, not UtcOffsets - the utils *mostly* got moved to timezone_provider::utils, but it's doc(hidden).
Work towards boa-dev#409 This moves the TimeZoneProvider trait to the timezone_provider crate. There's a lot of stuff that gets moved as a result. Some important things to note: - TimeZoneProvider now operates on its own IsoDateTime type - TimeZoneProvider produces UtcOffsetSeconds, not UtcOffsets - the utils *mostly* got moved to timezone_provider::utils, but it's doc(hidden).
Work towards #409
This moves the TimeZoneProvider trait to the timezone_provider crate. There's a lot of stuff that gets moved as a result.
Some important things to note: