TimeZone + ResolvedTimeZone
#5549
Conversation
robertbastian
requested review from
nordzilla,
zbraniecki and
a team
as code owners
robertbastian
requested review from
sffc
and removed request for
zbraniecki and
nordzilla
| /// ``` | ||
| /// | ||
| /// [`CustomZonedDateTime::try_iso_from_str`]: crate::CustomZonedDateTime::try_iso_from_str | ||
| /// [`ZonedDateTime::try_iso_from_str`]: crate::ZonedDateTime::try_iso_from_str | ||
| #[cfg(feature = "compiled_data")] | ||
| #[inline] | ||
| pub fn try_from_str(s: &str) -> Result<Self, UnknownTimeZoneError> { |
There was a problem hiding this comment.
Thought: We should support parsing -0500[America/Chicago]
There was a problem hiding this comment.
We should, we already have the code through ixdtf.
| metazone_calculator: &MetazoneCalculator, | ||
| zone_offset_calculator: &ZoneOffsetCalculator, |
There was a problem hiding this comment.
This feels obvious, but we should probably try to merge these down to one type. I can't think of any use case where someone would want to load a MetazoneCalculator and a ZoneOffsetCalculator from different data sources.
There was a problem hiding this comment.
I think for now just keep MetazoneCalculator and add the zone offset data marker to its signature. It will load two data payloads. Eventually we should merge the data payloads into one.
| /// The CLDR metazone identifier | ||
| pub fn metazone_id(&self) -> Option<MetazoneId> { | ||
| self.metazone_id | ||
| } |
There was a problem hiding this comment.
Issue: I do still feel it is useful/necessary to give a way to make one of these from IDs without data.
If we keep CustomTimeZone, it gives that low-level power for anyone who needs it.
There was a problem hiding this comment.
There is already a way to make one of these IDs without data: MetazoneId(tinystr!(_, "usce")). If we teach the datetime formatter to format this, our job is done. I do not want anyone to be able to construct the combination of chzrh and usce.
There was a problem hiding this comment.
If we teach the datetime formatter to format this
Ok.
robertbastian
force-pushed
the
tztype
branch
from
e1f355b
to
151f0b3
Compare
|
The build failures are FFI, which I can get to tomorrow. |
robertbastian
changed the title
TimeZone + FormattableTimeZoneTimeZone + FormattableTimeZone
robertbastian
changed the title
TimeZone + FormattableTimeZoneTimeZone + ResolvedTimeZone
| time_zone: &TimeZone, | ||
| timezone_calculator: &TimeZoneCalculator, |
There was a problem hiding this comment.
Nit: Do the same abstraction in FFI as you have in Rust, instead of passing the timezone calculator in here
There was a problem hiding this comment.
Let's do that when we have finalised all Rust APIs (timezone and datetime). It's a lot of work.
There was a problem hiding this comment.
OK, I thought we were mostly finalized, but from #5533 now I understand that we're not
| /// | ||
| /// [data provider]: icu_provider | ||
| #[derive(Debug)] | ||
| pub struct MetazoneCalculator { | ||
| pub struct TimeZoneCalculator { |
There was a problem hiding this comment.
Discussion: Not sure about the name TimeZoneCalculator. I would expect such a name to be reserved for a TZDB calculator.
#5533