Improve docs of TimeZoneVariant #6704
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
|
I feel that the docs are better than the status quo. The new docs are precise and accurate. The old docs are misleading at best. Remember that our audience is prone to making incorrect assumptions, so we want to make sure that the docs are accurate and don't feed into those assumptions about how time zones work. |
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
components/time/src/zone/mod.rs
Outdated
| @@ -48,7 +48,7 @@ | |||
| //! Many zones use different names and offsets in the summer than in the winter. In ICU4X, | |||
| //! this is called the _zone variant_. | |||
| //! | |||
| //! CLDR has two zone variants, named `"standard"` and `"daylight"`. However, the mapping of these | |||
| //! CLDR has two zone variants. Although they are named `"standard"` and `"daylight"`, the mapping of these | |||
There was a problem hiding this comment.
I think the mapping is pretty consistent if we say standard is the standard offset and daylight is higher. I also don't see the point of introducing winter and summer concepts here. They consistently represent standard and daylight time, which is usually observed during summer.
components/time/src/zone/mod.rs
Outdated
| @@ -203,19 +219,25 @@ pub(crate) mod ule { | |||
| #[cfg_attr(feature = "serde", derive(serde::Deserialize))] | |||
| #[non_exhaustive] | |||
| pub enum TimeZoneVariant { | |||
| /// The variant corresponding to `"standard"` in CLDR. | |||
| /// Indicates the period in a time zone with a lower UTC offset. | |||
There was a problem hiding this comment.
I would also want to remove the Standard = lower definition here. Please have a look at my draft: https://github.com/unicode-org/icu4x/pull/6713/files#diff-dd9bce37cba94e980431a686c194069fb7d4ed0275434b9301a369ab4e44558b
There was a problem hiding this comment.
I reworded this to state "lower than Daylight" and that all time zones use this variant.
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
components/time/src/zone/mod.rs
Outdated
| /// Although CLDR calls this variant `"daylight"`, the semantics vary | ||
| /// from time zone to time zone. The time zone display name of this variant |
There was a problem hiding this comment.
Again, I don't actually think the "semantics" vary, it's always the higher-offset zone. The display names vary.
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
| /// offset from UTC than the `Standard` variant. Although these definitions | ||
| /// correspond to daylight saving time as observed in some parts of the world, | ||
| /// the actual behavior may or may not be intuitive; for example: |
There was a problem hiding this comment.
Let's not get carried away here, 3 out of 490 zones have exceptions
| /// offset from UTC than the `Standard` variant. Although these definitions | |
| /// correspond to daylight saving time as observed in some parts of the world, | |
| /// the actual behavior may or may not be intuitive; for example: | |
| /// offset from UTC than the `Standard` variant. Although these definitions | |
| /// correspond to daylight saving time as observed in most of the world, | |
| /// some zones exhibit unintuitive behavior: |
| @@ -203,19 +225,29 @@ pub(crate) mod ule { | |||
| #[cfg_attr(feature = "serde", derive(serde::Deserialize))] | |||
| #[non_exhaustive] | |||
| pub enum TimeZoneVariant { | |||
| /// The variant corresponding to `"standard"` in CLDR. | |||
| /// Indicates the period in a time zone with a lower UTC offset than `Daylight`. | |||
There was a problem hiding this comment.
I don't like defining standard in terms of daylight, because most zones don't have a daylight variant...
| /// Although CLDR calls this variant `"standard"`, the display names vary | ||
| /// from time zone to time zone. The time zone display name of this variant | ||
| /// may or may not be called "Standard Time". |
There was a problem hiding this comment.
| /// Although CLDR calls this variant `"standard"`, the display names vary | |
| /// from time zone to time zone. The time zone display name of this variant | |
| /// may or may not be called "Standard Time". | |
| /// Although CLDR calls this variant `"standard"`, the display names vary | |
| /// between time zones and locales and may or may not include the term | |
| /// "Standard Time" or a translation thereof. |
| /// Although CLDR calls this variant `"daylight"`, the display names vary | ||
| /// from time zone to time zone. The time zone display name of this variant | ||
| /// may or may not be called "Daylight Time". |
There was a problem hiding this comment.
| /// Although CLDR calls this variant `"daylight"`, the display names vary | |
| /// from time zone to time zone. The time zone display name of this variant | |
| /// may or may not be called "Daylight Time". | |
| /// Although CLDR calls this variant `"daylight"`, the display names vary | |
| /// between time zones and locales and may or may not include the term | |
| /// "Daylight Time" or a translation thereof. |
#6701
CC @Manishearth