Add time zone normalization #415
Conversation
Manishearth
force-pushed
the
normalized-idents
branch
2 times, most recently
from
38e1c66 to
6ab8a12
Compare
src/builtins/core/zoneddatetime.rs
Outdated
|
|
||
| let timezone = partial.timezone.unwrap_or_default(); | ||
|
|
||
| let timezone = timezone.normalize_with_provider(provider)?; |
There was a problem hiding this comment.
Perhaps this should instead be in temporal_capi's PartialZonedDateTime ctor.
I think things would be cleaner if TimeZone could only ever be constructed with normalized timezones; however this becomes a much more invasive change. I can still attempt it.
There was a problem hiding this comment.
I don't think there's any point where the time zone isn't expected to be normalized by the spec. And lack of normalization impacts things like equality.
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
I think this is an okay middleground for now, but I think the change to TimeZoneProvider should replace the currently existing method.
src/provider.rs
Outdated
| pub trait TimeZoneProvider { | ||
| fn check_identifier(&self, identifier: &str) -> bool; | ||
|
|
||
| fn normalize_identifier(&self, ident: &'_ str) -> TemporalResult<Cow<'_, str>>; |
There was a problem hiding this comment.
If we are adding this normalization, I think I'd prefer to remove check_identifier and always normalize. If I recall correctly, the specification essentially expects the normalized IANA identifier after lookup, so we should do that.
There was a problem hiding this comment.
I'll try. The main problem is that this gets pretty pervasive pretty quickly.
There was a problem hiding this comment.
Is it that pervasive? I was thinking that normalize_identifier should only really be replacing GetAvailableNamedTimeZoneIdentifier.
There was a problem hiding this comment.
Is there a difference between that and every callsite of TimeZone::try_from... not counting offset callsites?
I'll have to experiment with this. Part of the problem is that TimeZones are constructed on the fly in a lot of different places.
|
I think I've mentioned this in the TimezoneProvider issue, but it also applies here. Would it be that bad if we always used compiled data to normalize? Seems like it could simplify this a lot. |
|
Data skew is a problem I think. It's not that big a deal to pass providers down to this code, I don't mind just doing that. |
I thought about doing this initially, but part of the issue is that this is not just normalizing the identifier. It's also checking that the identifier is in the underlying data. They need to be coupled so that they can't desync. Once we know the identifier exists, then the other methods / data fetching is infallible, because it must exist. |
Got it. Though, this IMO is also nicely solved by the |
|
Ideally we can avoid all those allocations and just use an iana id or something. But yes. |
|
I think I've updated this to unconditionally normalize everywhere |
Manishearth
force-pushed
the
normalized-idents
branch
3 times, most recently
from
e2f615c to
da90d6c
Compare
Manishearth
force-pushed
the
normalized-idents
branch
from
da90d6c to
bb14308
Compare
3 participants
Fixes #382
Open to feedback on the API. An alternate way to structure this is to always normalize. That would certainly be cleaner: thoughts?