-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Iterate through canonical timezones and metazones #5675
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but the combination of no issue, no description, no informative commit messages, and no previous discussion means I'm left guessing how this PR fits in with the bigger picture. What I think this does: it figures out the primary zone of each metazone, which is the one for region 001 in the data file, and then it saves a list of those primary zones in the data file, which are then returned as an iterator. Drawing from the content of this PR and previous discussions we've had in this space, I am making an educated guess that you are suggesting this approach in order to demonstrate that time zone IDs can be used instead of metazone IDs in order to uniquely identify a metazone. Is that right? Did you consider when a time zone changes metazones, thereby messing up the mapping?
/// Metazones | ||
pub metazones: ZeroVec<'data, u16>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Please explain/document this field. Previous authors of this data struct (mostly me) have gone above and beyond documenting how the pieces fit together, since it is non-obvious. I don't immediately see how a ZeroVec<u16>
relates to metazones.
pub fn iter_metazones(&self) -> impl Iterator<Item = TimeZoneBcp47Id> + '_ { | ||
self.data | ||
.metazones | ||
.iter() | ||
.filter_map(|i| self.data.bcp47_ids.get(i as usize)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: huh? The function returns an iterator of TimeZoneBcp47Id
but it is documented as returning an iterator of metazones.
@@ -95,6 +95,8 @@ pub struct IanaToBcp47MapV3<'data> { | |||
// Note: this is 9739B as `ZeroVec<TimeZoneBcp47Id>` (`ZeroVec<TinyStr8>`) | |||
// and 9335B as `VarZeroVec<str>` | |||
pub bcp47_ids: ZeroVec<'data, TimeZoneBcp47Id>, | |||
/// Metazones | |||
pub metazones: ZeroVec<'data, u16>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Besides iterating over metazones, what do you plan to use this field for?
(We shouldn't add fields to the core TimeZoneIdMapper data struct unless they have a strong core use case)
So I added this because you were convinced that there are use cases of iterating through all time zones or all metazones for displaying time zone pickers. Do you still think that's a use case? I can bit pack the data struct, we can store this information for free if we want. If not, just close the PR. |
No description provided.