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}
components/time/src/zone/mod.rs
Outdated
| /// This should not generally be constructed by client code. Instead, use | ||
| /// Although the variants are named `Standard` and `Daylight`, what they actually indicate | ||
| /// is whether the period has a lower offset or a higher offset from UTC. They do NOT | ||
| /// unambiguously indicate "standard time" and "daylight time" in all time zones. Some |
There was a problem hiding this comment.
I don't agree that this change is necessary. Daylight savings time is by definition a period with a higher offset, even if Ireland uses weird display names in non location formats (in specific location format, you will get things like "Irland (Normalzeit)" and "Irland (Sommerzeit)" as assembled from the patterns).
TZDB and CLDR both use the standard and daylight terminology. They are our data sources. I feel like we had this discussion before, I don't understand what changed since then?
There was a problem hiding this comment.
I disagree on two points:
- TZDB and CLDR are both internal-facing. The concept of standard/daylight is not normally exposed via datetime APIs. They are a very, very weak precedent to use for naming these. Also, TZDB calls it
isdst, not "daylight". - The standard/daylight naming is very 1980s US-centric and might not even work moving forward with states thinking about doing strange things with their time zones.
Given a lack of naming precedent, and since these variants are prone to causing confusion, we should call these variants based on what they are.
There was a problem hiding this comment.
TZDB and CLDR are both internal-facing. The concept of standard/daylight is not normally exposed via datetime APIs.
Ok CLDR is internal, but the external project ICU also exposes it: https://javadoc.io/static/com.ibm.icu/icu4j/77.1/com/ibm/icu/util/TimeZone.html#getDisplayName-boolean-int-.
And because it's exposed in TZDB (what do you think isdst stands for?) it's exposed in most libraries that use TZDB data, e.g. https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/zone/ZoneRules.html#getDaylightSavings(java.time.Instant)
The standard/daylight naming is very 1980s US-centric
You know I'm all for avoiding US-centrism, but this is not. Daylight saving time as a concept is a universal thing, look at this map on Wikipedia that colors more than the American Time zones that use "Daylight" in their English names. Please don't get hung up on display names.
might not even work moving forward with states thinking about doing strange things with their time zones.
Yes it will. There are already many locations that abolished DST or switched to "permanent DST" (Yukon a few years ago, Coyhaique most recently), and it just works.
Given a lack of naming precedent
demonstrably false
we should call these variants based on what they are.
We should! They are Standard and Daylight
There was a problem hiding this comment.
Okay, I softened the language a bit.
There was a problem hiding this comment.
Also please note that I'm not changing the name of anything in this PR. I am only using the docs to better define what "standard" and "daylight" mean. This is what docs are for and therefore should not be controversial!
I have a separate issue to discuss whether we should change these names to something else in 3.0, #6701, but that isn't what I'm doing in this PR.
| /// | ||
| /// - Irish Standard Time, UTC+1, is the `Daylight` variant of "Europe/Dublin", | ||
| /// which observes Greenwich Mean Time, UTC+0, during the winter. | ||
| /// - Morocco observes UTC+1 for most of the year, but falls back to UTC+0 during |
There was a problem hiding this comment.
I think this is the only case where there is actually a meaningful mismatch between the TZDB/CLDR concept and the facts on the ground, but given that neither TZDB nor CLDR nor ICU can model Ramadan Time, we should not confuse users by pointing this out. Modelling it like this works and it's how all other systems do it.
There was a problem hiding this comment.
They do model it: by using Standard == lower offset and Daylight == higher offset. How to represent Ramadan Time flows naturally and easily from that mental model.
If we end up with a zone that has more than two offsets in a year, then we can add a third variant above Daylight such as SuperDaylight or perhaps one below Standard called Darkness or something.
There was a problem hiding this comment.
But it doesn't. In Morocco the time that is used for 11 months of the year is not called standard time under this model, it's the daylight time, and Ramadan is the standard time. That doesn't make sense.
|
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. |
| /// | ||
| /// - Irish Standard Time, UTC+1, is the `Daylight` variant of "Europe/Dublin", | ||
| /// which observes Greenwich Mean Time, UTC+0, during the winter. | ||
| /// - Morocco observes UTC+1 for most of the year, but falls back to UTC+0 during |
There was a problem hiding this comment.
But it doesn't. In Morocco the time that is used for 11 months of the year is not called standard time under this model, it's the daylight time, and Ramadan is the standard time. That doesn't make sense.
#6701
CC @Manishearth