Skip to content
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

"zone" vs "time zone" #5999

Open
robertbastian opened this issue Jan 15, 2025 · 48 comments
Open

"zone" vs "time zone" #5999

robertbastian opened this issue Jan 15, 2025 · 48 comments
Labels
C-time-zone Component: Time Zones

Comments

@robertbastian
Copy link
Member

We're not very consistent with when we use "zone" and when we spell out "time zone".

For types, we have TimeZoneBcp47Id, TimeZoneInfo, TimeZoneModel, but ZoneVariant, ZoneOffsets, and ZoneOffsetCalculator.

For methods, datetime uses .with_zone_{location,offset,...}, but also load_time_zone_{essentials, locations,...}.

ZonedDateTime has a field zone: TimeZoneInfo, but the we use time_zone: TimeZoneInfo on FFI.

@robertbastian robertbastian added discuss-priority Discuss at the next ICU4X meeting C-time-zone Component: Time Zones labels Jan 15, 2025
@robertbastian robertbastian added this to the ICU4X 2.0 ⟨P1⟩ milestone Jan 15, 2025
@robertbastian
Copy link
Member Author

robertbastian commented Jan 20, 2025

Time type and subtypes at icu::time root. Remove Iso prefix from HMS because NanoSecond doesn't have it either, and the ISO model is not very special.

Current Name Proposed Name
icu::timezone::Time icu::time::Time
icu::timezone::types::IsoHour icu::time::Hour
icu::timezone::types::IsoMinute icu::time::Minute
icu::timezone::types::IsoSecond icu::time::Second
icu::timezone::types::NanoSecond icu::time::Nanosecond

TimeZoneInfo, composite types and parser at icu::time root.

Current Name Proposed Name
icu::timezone::DateTime<A> icu::time::DateTime<A>
icu::timezone::TimeZoneInfo<M> icu::time::TimeZoneInfo<M>
icu::timezone::ZonedDateTime<A> icu::time::ZonedDateTime<A>
icu::timezone::IxdtfParser icu::time::ZonedDateTimeParser
icu::timezone::InvalidOffsetError icu::time::InvalidOffsetError
icu::timezone::ParseError icu::time::ParseError

Time zones things in a icu::time::zone module, using TimeZone and not just Zone:

Current Name Proposed Name
icu::timezone::TimeZoneBcp47Id icu::time::zone::TimeZoneId
icu::timezone::ZoneVariant icu::time::zone::TimeZoneVariant
icu::timezone::UtcOffset icu::time::zone::UtcOffset
icu::timezone::ZoneOffsets icu::time::zone::UtcOffsets
icu::timezone::ZoneOffsetCalculator icu::time::zone::UtcOffsetCalculator

Time zone info models also in icu::time::zone because there isn't really a better place:

Current Name Proposed Name
icu::timezone::TimeZoneModel icu::time::zone::TimeZoneInfoModel
icu::timezone::models::AtTime icu::time::zone::models::AtTime
icu::timezone::models::Base icu::time::zone::models::Base
icu::timezone::models::Full icu::time::zone::models::Full

IANA conversions in icu::time::zone::iana:

Current Name Proposed Name
icu::timezone::TimeZoneIdMapper[Borrowed,...] icu::time::zone::iana::IanaMapper[Borrowed,...]
icu::timezone::TimeZoneBcp47Iter icu::time::zone::iana::TimeZoneIdIter
icu::timezone::TimeZoneCanonicalIanaIter icu::time::zone::iana::CanonicalIanaIter

Windows conversions in icu::time::zone::windows:

Current Name Proposed Name
icu::timezone::WindowsTimeZoneMapper[Borrowed] icu::time::zone::windows::WindowsMapper[Borrowed]

@robertbastian
Copy link
Member Author

Also the field ZonedDateTime::zone and the FFI argument time_zone: TimeZoneInfo should probably be called zone_info to drive home that this is an enriched time zone.

@sffc
Copy link
Member

sffc commented Jan 20, 2025

Now that we have thin DateTime, we should also change TimeZoneModel::LocalTime to be DateTime<Iso>.

@robertbastian robertbastian added the needs-approval One or more stakeholders need to approve proposal label Jan 20, 2025
@sffc
Copy link
Member

sffc commented Jan 21, 2025

I think NanoSecond should be Nanosecond

Not sure about icu_time crate name. icu_clock has a nice ring and is the closest parallel to calendar (you read dates from a calendar and times from a clock). icu_chrono is fine too.

If we put the time zone fields like TimeZoneId in a sub-module, then maybe Hour and friends should go in a sub-module. icu::clock::time::Hour?

All the rest of the names in the table are LGTM.

@robertbastian
Copy link
Member Author

icu_clock is bad, because it's the physical object that tells you the time, not the system of time calculations (like a calendar is). I much prefer icu_time over icu_chrono, nobody knows what chrono means, it's just a weird synonym for time that C++ started using because the time name was already taken.

@robertbastian
Copy link
Member Author

icu_temporal would actually be more aligned with icu_calendar; it's a crate about things related to time, temporal things.

@sffc
Copy link
Member

sffc commented Jan 28, 2025

@Manishearth @zbraniecki Feedback?

@Manishearth
Copy link
Member

I don't like icu_clock, it feels like an API for system time (in a way that "calendar" is not). "calendar" can mean calendar systems, "clock" doesn't have an easy parallel.

icu_time sounds great to me, I don't see a reason to work around it.

As far as Zone vs TimeZone, I don't feel like it's a problem we must solve, but it's a problem we could solve

Time type and subtypes at icu::time root. Remove Iso prefix from HMS because NanoSecond doesn't have it either, and the ISO model is not very special.

Strongly in favor, I think the reason behind these names (consistency with IsoYear / etc) is not relevant anymore)

TimeZoneInfo, composite types and parser at icu::time root.

In favor of the changes here, but unsure on renaming IxdtfParser. It might be nice to be specific about what format it parses. OTOH people mostly haven't heard of ixdtf.

Time zones things in a icu::time::zone module, using TimeZone and not just Zone:

I like this, and I like Zone -> Utc where it matters.

IANA conversions in icu::time::zone::iana:

Windows conversions in icu::time::zone::windows:

I don't love four levels of modules but it's not that big a deal for these less common types. Otherwise like the naming here

@nekevss
Copy link
Contributor

nekevss commented Jan 28, 2025

FWIW, I'd agree that icu_clock sounds like a system time crate.

Both icu_time and icu_temporal sounds fine with a slight preference towards icu_temporal because having a DateTime<A> and ZonedDateTime<A> in icu_time seems slightly surprising to me.

@sffc
Copy link
Member

sffc commented Jan 31, 2025

Sounds like people are okay with icu_temporal. I am, too, though I should flag @justingrant who may worry that this would further the confusion about icu_temporal::DateTime looking like it is some official DateTime for the Rust ecosystem when it is only a thin interface for formatting.

@Manishearth
Copy link
Member

Manishearth commented Jan 31, 2025

I'm not OK with icu_temporal; I'd rather not have confusion between ECMA Temporal and ICU Temporal. I think icu_time is fine. Time zones are a part of dealing with time.

@sffc
Copy link
Member

sffc commented Jan 31, 2025

I'm somewhat opposed to icu_time. It sounds like it is "ICU4X's time formatting crate", which is not true. I think it is confusing with icu_datetime, which is actually a formatting utility. This is a crate that is barely a component crate and is only categorized as such because it depends on another component crate (icu_calendar, which is a good name). I'm also somewhat opposed to icu_timezone since it now does more than time zones, although time zones are still the primary ICU4X data in the crate, so for me icu_timezone probably is slightly above icu_time.

My favorite is still icu_clock. I think the parallel between a Calendar for dates and a Clock for times is compelling. I am not distracted by the analogy to the "system clock", but I acknowledge that it is a possible source of confusion. The other names are all fine.

So, sounds like we're headed toward a bikeshed ballot. Any more to add to the list?

  1. icu_timezone
  2. icu_time
  3. icu_clock
  4. icu_chrono
  5. icu_temporal

Here are some from Gemini. Any we want to promote?

  • icu_chronos (Greek for "time")
  • icu_chronometry
  • icu_tempo
  • icu_tempus (Latin for "time")
  • icu_timekit and similar things like timewarp and timecraft
  • icu_meridian
  • icu_nexus
  • icu_tick
  • icu_span
  • icu_chron
  • icu_phase
  • icu_sundial
  • icu_hourglass

I kind-of like icu_chronos; doesn't have the name conflict with "chrono" but it is still clearly about chronology. icu_tempo is also curious.

@Manishearth
Copy link
Member

Manishearth commented Jan 31, 2025

I am also fine with icu_timezone. I don't like icu_temporal or icu_clock.

I don't think any of those other names are good: I think clever names work well for utils crates but in the icu_* universe they stand out and feel like they're interoperating with some other fancily named thing: e.g. icu_chrono and icu_temporal sound like they interoperate with those things. Even if such a project does not exist, the fancy name makes me think such a project does. (The utils crates don't have this problem, they are the fancily named other project. But icu_foo means "icu crate for dealing with foo" and if foo is a fancy word then it sounds like some 3p integration)

Tbh I'm not sure we need a vote, I don't see how this issue became an issue for discussing the crate name. It's unclear to me if anyone has provided a reason for disliking icu_timezone.

But my vote is 1 ~> 2 >> 5 ~> 3 > 4, I'm rather strongly against any of the more clever names.

@sffc
Copy link
Member

sffc commented Jan 31, 2025

I don't see how this issue became an issue for discussing the crate name.

The first line of @robertbastian's proposal in comment#2 is the new crate name. I think it's the most important and probably most controversial proposal in that post.

But icu_foo means "icu crate for dealing with foo"

This is the crux. Is it the crate for "dealing with" the name? Maybe so. I think "processing" might be a better term. icu_calendar processes calendars, producing dates. icu_datetime processes dates, producing strings. icu_clock would process clocks, producing times.

I don't think any of those other names are good: I think clever names work well for utils crates

I don't see these as "clever" names. Some of them, sure. But there is nothing clever about "temporal" or "clock" or "chronos" referring to a crate dealing with times.

It's unclear to me if anyone has provided a reason for disliking icu_timezone.

Because the crate does more than time zones now?

It's weird to import icu::timezone::DateTime, a requirement to format a datetime without a time zone.

@robertbastian
Copy link
Member Author

I think "chrono" and "chronos" are fancy names, the average developer will not know that it means timey-things.

@Manishearth
Copy link
Member

This is the crux. Is it the crate for "dealing with" the name? Maybe so. I think "processing" might be a better term. icu_calendar processes calendars, producing dates. icu_datetime processes dates, producing strings. icu_clock would process clocks, producing times.

My choice of language there was vague. Regardless of whether it is for dealing with or processing or whatever, putting a fancy name there makes it sound like "the ICU crate for verbing with FancyName", which makes it sound like an external project.But there is nothing clever about "temporal" or "clock" or "chronos" referring to a crate dealing with times.

I think "processing" might be a better term. icu_calendar processes calendars, producing dates. icu_datetime processes dates, producing strings. icu_clock would process clocks, producing times.

I'm not convinced by this. Firstly, the usage of clock doesn't match what I think of as a clock in this context. A timestamp is not a clock, a clock is something that changes, and this crate does not interoperate with system time APIs.

We do not have a Clock type. This crate does not process clocks.

Secondly, this falls down for crates like segmenter, casemapper, etc. We have a variety of naming conventions here from different constraints, I don't think we are beholden to backsolving a justification for the datetime constraint and applying it here.

But there is nothing clever about "temporal" or "clock" or "chronos" referring to a crate dealing with times.

Okay, whatever, some of them are not "clever", but they still sound like library names to me: they're using a word that is a bit removed from the usual vocabulary and not shared with any of the types it contains.

The only one that doesn't have this property, to me, is "clock", which has a different problem.

General rule of thumb: if the component crate is called icu_foo, it should have a type called Foo, FooBar (e.g. PluralRules), or some grammatical inflection thereof. If not, we risk misleading people about the purpose of the crate, either by telling them that the crate processes a type of thing that it does not contain, or that the crate interoperates with some other library named Foo.

@Manishearth
Copy link
Member

Because the crate does more than time zones now?

It's weird to import icu::timezone::DateTime, a requirement to format a datetime without a time zone.

Okay, this is the first time this argument has been put forth here. I'm not super swayed by it, the type can be reexported if we want. I think we've ended up with a crate split that's got some issues but is also the best we can do, and it's fine if the name doesn't cover everything the crate does. DateTime is a simple wrapper type, it isn't the focus of the crate, I'm okay with it living wherever and having appropriate signposting/reexports.

This crate is still very much a timezones crate.

Also, never actually responded to this:

I'm somewhat opposed to icu_time. It sounds like it is "ICU4X's time formatting crate", which is not true. I think it is confusing with icu_datetime, which is actually a formatting utility.

I still think icu_time is fine. icu_calendar doesn't format calendars, and it performs a similar task: processing related to calendars. I think if people don't like icu_time then we should revisit icu_calendar.

@sffc
Copy link
Member

sffc commented Jan 31, 2025

icu_calendar doesn't format calendars

I'll push back on this. It "verbs" calendars in many other ways. And when (not if) we add calendar display names, they should probably go in icu_calendar.

My ranking is roughly: 3 > 5 ~> 4 ~>> 1 ~>> 2

Another idea: icu_datetime_timezone and re-export all the stuff from icu_datetime which is close to the only use case for these things.

@Manishearth
Copy link
Member

Manishearth commented Feb 1, 2025

I'll push back on this. It "verbs" calendars in many other ways

That's not pushback, that's in concordance with my opinion: this crate primarily "verbs" times/timezones, even though it doesn't format them. I mentioned formatting as a response to the idea that icu_time or icu_timezone makes it sound like it formats times/timezones, with an example of a similar crate that doesn't format.

icu_datetime_timezone is also fine

@Manishearth
Copy link
Member

observation: our rankings are incompatible with the vetos

@justingrant
Copy link

I have no opinion about most of these renames because I don't have enough context, but here's feedback on a few of them:

icu::timezone::TimeZoneBcp47Id icu::time::zone::TimeZoneId

Although we may wish it were otherwise, the vast majority of computing platforms consider time zone IDs to be IANA names. So most developers will at first expect an API called TimeZoneId to return an IANA ID.

If an API exposes a non-IANA ID format then I think it'd be helpful to clarify this fact in the API name. So I'd suggest leaving this API name as-is for clarity, or using some other qualifier like TimeZoneUnicodeId, TimeZoneUts35Id, TimeZoneCldrId, etc. to clarify that this is NOT an IANA name. I don't think it matters very much what qualifier is used, as long as it's clear that this isn't an IANA ID.

In favor of the changes here, but unsure on renaming IxdtfParser. It might be nice to be specific about what format it parses. OTOH people mostly haven't heard of ixdtf.

If a format-specific name is retained, then Rfc9557Parser is probably the right name. Now that the RFC has been approved, it seems unlikely that IXDTF will ever be popularized as an acronym. Developers will just call them "RFC 9557 strings" like they do for ISO 8601 strings.

I also don't mind ZonedDateTimeParser because it makes it clear what's gonna come out the other end. And also what makes the parser different than, say, a parser for an ISO 8601 timestamp without annotations.

I am also fine with icu_timezone. I don't like icu_temporal or icu_clock.

Is this library only focused on timezones? If not then I'd avoid icu_timezone as false advertising.

Unless this is the Rust port of the JS Temporal API, I'd also vote against icu_temporal as needlessly confusing with another widely-used API.

Also, honestly, most people who've given feedback about the name "Temporal" in JS don't like the name. It was chosen to avoid confusion with the existing Date API. Had there been no legacy it would probably have been called Time or Date or DateTime. It's hard to imagine someone choosing it except as a last resort if all other better names were excluded.

re: icu_clock: after interacting with programmers during the design of Temporal, I don't think most developers understand the subtle differences between "clock time" vs. just "time". That isn't a reason to avoid icu_clock, but IMO "clock" doesn't add much (if any) additional meaning for most users over other synonyms like "time".

I think icu_chrono is OK but kinda obscure.

TL;DR - If icu_time is available that'd be my recommendation, followed by icu_clock and icu_chrono. I would avoid the other two. Sorry, I wasn't sure how to translate this into voting syntax!

@Manishearth
Copy link
Member

Is this library only focused on timezones? If not then I'd avoid icu_timezone as false advertising.

That's part of the disagreement here, potentially.

Here are the current docs: https://unicode-org.github.io/icu4x/rustdoc/icu_timezone/index.html

All of the complex types are timezone related. However it also contains a simple Time, DateTime, and ZonedDateTime type.

(My opinion is that it's fine for icu_timezone to contain those types and they can be reexported from icu_datetime if the location feels confusing to people)

@BurntSushi
Copy link

I got linked here by @Manishearth and invited to offer an opinion. While I've perused the icu APIs a bit, I definitely don't have a ton of context.

With respect to the crate name, I think if I saw icu_chrono or icu_temporal, I would, absent other information, assume they are somehow related to the chrono Rust crate or the Temporal JS project, respectively. I think this goes toward's Manish's "fancy" name argument. They're just obscure enough that I would assume some kind of connection. Conversely, I would not assume that icu_time is related to the time crate.

Otherwise, I don't think I have enough context to levy an opinion. Mostly just going by the docs linked, the first line says that it's about time zones. But that makes me confused when I see types like DateTime defined in the crate, as I would expect that type to be in the icu_datetime crate. Which I think just leads me to believe that I don't really understand the API design philosophy, and probably makes me under-equipped to have an opinion here. But I think it's worth sharing that I am confused by the APIs here (even though I simultaneously believe that y'all have considered all of this and there's probably a very good reason for why things are the way they are).

I would also agree with @justingrant that if I see "time zone ID," then I'm probably going to assume that's an IANA time zone id. I don't think any other identifier has a lot of mindshare.

One other thing I'd point out is a potential incongruity with how Nanosecond is named. It matches the names of other units like Hour, Minute and Second, but its actual value is more like, "fractional seconds, to nanosecond precision." This becomes potentially confusable in contexts where you want to represent or expose milliseconds, microseconds and nanoseconds as distinct units. For example, std::time::Duration uses the subsec_ prefix to disambiguate between these concepts on the std::time::Duration type. I do the same in Jiff for its civil::Time type.

@Manishearth
Copy link
Member

Otherwise, I don't think I have enough context to levy an opinion. Mostly just going by the docs linked, the first line says that it's about time zones. But that makes me confused when I see types like DateTime defined in the crate, as I would expect that type to be in the icu_datetime crate. Which I think just leads me to believe that I don't really understand the API design philosophy, and probably makes me under-equipped to have an opinion here.

Some background here: icu_datetime is date time formatting, and most of our formatter crates don't say "formatter" in them since it's a mouthful. The DateTime type is a thin wrapper around Date and Time which is occasionally needed for timezone stuff and of course one of the types you can feed to the datetime formatter. As such it needs to exist in icu_timezone or a dependency of it (icu_datetime depends on icu_timezone, since it can format zoned stuff). It feels weird to have a whole crate for a very small type, and then you need to figure out what to do with ZonedDateTime, so it ended up in this crate.

I generally feel that the decision to do so was done more out of expediency rather than some vision of what the icu_timezone crate should be about, so having it be named after times or timezones whilst containing one out-of-place type seems fine by me.

@justingrant
Copy link

Although we may wish it were otherwise, the vast majority of computing platforms consider time zone IDs to be IANA names. So most developers will at first expect an API called TimeZoneId to return an IANA ID.

TimeZoneId represents an IANA time zone. It may use a different internal string representation, and different canonicalization equivalence classes, but it is inherently an IANA time zone (unlike, for example, Windows time zones) that can be parsed and serialized into IANA strings.

If I call as_str() on a TimeZoneId, is that output an IANA string? If not, then I'd suggest giving the type a name that hints that the default string representation won't be IANA. Otherwise most developers will reasonably assume that the result is going to be an IANA name.

If it's named TimeZone then it seems less of a big deal because that sounds like an object that does stuff as opposed to a string ID. I'd still love to see properties or methods exposed like Bcp47Id and IanaId to make it clear that there are different representations available.

Also, if a time zone instance uses inccu internally, what will it output when converting to IANA: "Asia/Calcutta" or "Asia/Kolkata"?

@robertbastian
Copy link
Member Author

There doesn't necessarily need to be an as_str. It could be TimeZoneId::as_bcp47 which could return an icu::locale::subtags::Subtag (which in turn has an as_str).

Also, if a time zone instance uses inccu internally, what will it output when converting to IANA: "Asia/Calcutta" or "Asia/Kolkata"?

#5610 (comment)

@justingrant
Copy link

There doesn't necessarily need to be an as_str. It could be TimeZoneId::as_bcp47 which could return an icu::locale::subtags::Subtag (which in turn has an as_str).

This sounds like a good idea. More discoverable.

@sffc
Copy link
Member

sffc commented Feb 4, 2025

In #5610, we have a few definitions floating around for "time zone." ICU4X uses the definition based on CLDR, which defines BCP-47, which is derived from TZDB zone.tab, which is consistent with ECMA-262. (Let's not get into that here: see #5610.) But, this may not be the definition all users expect.

So, maybe instead of TimeZone, the original name Bcp47TimeZoneId was actually more accurate. We could also consider CldrTimeZone or similar, to emphasize that this is from that specific reckoning of time zones.

@Manishearth
Copy link
Member

My expectation of a type named TimeZone is that it has machinery for doing things with the time zone, such as Temporal.TimeZone had, like getting the next and previous transition.

I don't think this is a useful litmus test: ICU4X's data-driven design means that most types with data from the user will have limited functionality, with most interesting functionality being on types wrapping ICU4X data. This is a choice driven by the language of implementation and API design.

But I think calling it ID is fine as well.

@Manishearth
Copy link
Member

Manishearth commented Feb 7, 2025

Decided to write down some facts to base the discussion off of:

  • icu_timezone currently contains the following classes of types:
    • tools for mapping between different formats of time zone ids, like TimeZoneIdMapper
    • ZonedDateTimeParser for performing timezone-aware IXDTF parsing.
    • ZoneOffsetCalculator
    • Types for representing different time zone models, used by the other APIs
    • Different time zone id types, like TimeZoneBcp47Id
    • A bunch of types. The types themselves are overall rather simple:
      • ZoneVariant and UtcOffset
      • a simple Time type, that wraps IsoHour, IsoMinute, IsoSecond etc. I believe there is some desire to remove the typed time unit types and use integers instead
      • DateTime, a thin wrapper around an icu_calendar::Date that interoperates with icu_datetime::DatetimeFormatter. The main additional functionality it provides is parsing from an ixdtf string, otherwise it's similar to a (Date, Time)
      • ZonedDateTime, like DateTime but with a zone. Can be parsed with a ZonedDateTimeParser
  • icu_calendar contains various Calendars, and a Date type, alongside a bunch of helper types like YearInfo etc
  • Our formatter crates tend to be named after the thing they format
    • icu_datetime contains DatetimeFormatter, which is able to format Dates (from icu_calendar), Times, DateTimes, and ZonedDateTimes (from icu_timezone`).
    • icu_decimal contains DecimalFormatter, which formats fixed_decimal::Decimals
  • We have non-formatter crates that are named after the thing they handle: icu_plurals classifies plurals (but doesn't format them), icu_casemap does casemapping, etc.
  • Time and DateTime were moved into icu_timezone from icu_calendar since we wanted icu_calendar to have a very narrow purpose: handling dates. This also meant that we could remove the fancy date-wrapping APIs on DateTime.
  • Time pretty clearly needs to be in icu_timezone or a dependency thereof, it cannot live in icu_datetime.
  • Less clearly: DateTime also needs to live in icu_timezone or a dependency thereof, as icu_timezone lets you compute what a timezone resolves to at a particular date (TimeZoneInfo::at_time()). This means that DateTime cannot live in icu_datetime, since icu_datetime needs to depend on icu_timezone and not the other way around.

@Manishearth
Copy link
Member

Some discussion between @sffc @robertbastian and @Manishearth

  • @Manishearth I want to leave a comment about splitting up vs. "zone". I have a position-neutral comment in the issue about where things are located currently. We need a time type, but not necessarily a datetime type.
  • @robertbastian The reason we have a time parser in the time zone crate is because we don't want users to ______.
  • @sffc The discussion we had previously is exacxtly what @robertbastian said, and I agree. I think the date time type is a tuple when we need date and time, and we should only use it when we need it.
  • @robertbastian We can't come up with a good name for this crate because it's not clearly demarcated.
  • @Manishearth Everyone was on board with datetime being in the crate because "it's time zone, so it's okay", but I don't think time zone should drive the name of the crate. It has a time zone type and a date time type. The date time type handles parsing. That's just a consequence of how we've organized code so far.
  • @robertbastian For me, it seems the main purpose of this crate is zoned date time. You don't load this crate for BCP-47 ids. You load it for formatting datetimes. I think the main type here is ZonedDateTime.
  • @Manishearth I don't really think that it's for ZonedDateTime.
  • @sffc ZonedDateTime encompasses everything else. So ZonedDateTime is the main entry point. But it also deals with time zones, so it's important to deal with.
  • @robertbastian critical user journey wise: you want to format a zoned datetime. you should just need the datetime crate. could be icu_zone, or icu_date, or icu_time
  • @sffc I think Critical User Journey (CUJ) is not a useful way to frame the argument for this crate.
  • @Manishearth For the CUJ of doing time zones, you should use the timezone crate.
  • @robertbastian This crate doesn't really do a lot about time zones. It can parse IDs and calculate variants, but it doesn't do much with offsets. We decided last week that we wouldn't add offset things. So should we call it icu_rfc9557? There are Rust crates that are called iso8601_timestamp or the like.
  • @sffc We have ixdtf which is not ICU-specific. So should we have icu_ixdtf?
  • @robertbastian I think Justin said that that is not the right term?
  • @sffc Functionally, this is the crate that does all of the ECMA-402 Temporal functionality that is in scope for ICU4X. So icu_temporal?
  • @Manishearth It's the ICU part of Temporal time zones.
  • @sffc It exposes IXDTF parsing as well.
  • @Manishearth I thought it was Justin or someone said it's bad because it's Temporal that is not Temporal.
  • @sffc I acknowledge that.
  • @Manishearth The people who know that name are JS people. It's not a set of functoinality that has that name outside of the JS world.
  • @robertbastian There are solid arguments against icu_clock, _chrono, and _temporal. The only realistic options we have are icu_time and icu_timezone. icu_time is the more general one, as time encompasses timezones, so we should use that.
  • @sffc We have other names that we brought up today that we didn't work. If it's true that ZonedDateTime and IXDTF is the main functionality of the crate, which it kind of is, then we can consider icu_zoned or just icu_zone.
  • @robertbastian But it also has the DateTime type.
  • @sffc We just said that we would ignore date time for the purpose of crate naming.
  • @Manishearth We were going to ignore the point that it should live in icu_datetime. I'm happy to do that for the purpose of the naming of this crate.
  • @robertbastian We're not going to doc(hide) in this crate, we might or might not reexport in icu_datetime, but that's irrelevant for this dicussions. It would be icu_zoned::DateTime, which is not a zoned datetime.
  • @sffc I'm convinced by icu::zoned::DateTime being confusing.
  • @sffc How about icu_zdt (abbreviate something)
  • @Manishearth I wouldn't be opposed to icu_date.
  • @sffc I think icu_calendar and icu_datetime are well named.
  • @Manishearth If we're going to go off of icu_calendar as a model for naming this crate, icu_date would have been a very good name for that crate as well, so icu_time in comparison would also be a good name.
  • @sffc I already put forward a justifcation for what we did based on what we did before. I see the icu_calendar crate as processing calendar. We're "verbing calendars". In the icu_datetime crate, we're "verbing datetimes". In this crate, we're "verbing ixdtf and time zones", but we're not verbing times.
  • @robertbastian: icu_calendars has Dates, and it processes those. icu_date would have been a very good name for the crate, icu_calendar is great for pointing out that we support multiple calendars, unlike most rust date crates, but icu_date is also fine.
  • @Manishearth By our naming scheme, other crates work differently. We're not consistent across crates. fixed_decimal isn't in an ICU named crate because it doesn't load data. In icu_calendar, we load calendar data, not date data.
  • @robertbastian From the naming perspective, I think icu_timezone is a better name, but from the functionality perspective, it does way more than time zones.
  • @sffc My issue here is the icu:: namespace is where we "verb things"
  • @robertbastian We don't "verb" lists, in icu_list list is clearly a noun
  • @Manishearth We do verb lists in icu_lists, we format them. I don't think we have consistency across "icu_calendar" and "fixed_decimal".
  • @sffc I put ICU crates into 2 buckets: collator, normalization, segmentation, etc. And then, everything else, which "verbs things". Maybe icu_collator should be called icu::text::Collator because it "verbs text", and collation is the type of operation. We have icu::locale::core, and we have consensus for icu::locale::fallback if we need to in the future. Should we just say that its main purpose is feeding icu::datetime, so we make it a subcrate?
  • @robertbastian I think you're trying to create an ontology for the sake of having an ontology. I think our current icu_timezone is a self-contained component. Putting it in the icu::datetime namespace just to satisfy a structure itch is not worth it, as the names will become super verbose (icu_datetime_timezone)
  • @Manishearth I see the buckets you're talking about. What does it do with plurals? We have transliterators, segmenters, plurals, etc., and we're not consistent. Maybe icu::time would make our users confused because it's inconsistent, but our names are all over the place.
  • @sffc I think what @Manishearth just said is a valid response to what I just said. To respond to what @robertbastian said about icu::timezone standing on its own: One of the facts of the situation is that the current crate doesn't do a lot. Just time zone calculation and IXDTF parsing. So maybe it doesn't stand on its own, and therefore maybe it goes in icu::datetime.
  • @robertbastian I think there is enough functionality that it justifies its own crate. Just to go from a string to a structured type means that it stands on its own.
  • @sffc icu_datetime_parse then? (not to be confused with human-readable parsing)
    • @robertbastian it should be icu_datetime and icu_datetime_format, but that's not how we do names
    • @sffc From that line of reasoning, we'd have icu_datetime that re-exports icu_datetime_formatter and icu_datetime_parsing
  • @Manishearth It is an underpowered timezone crate, but not so much that it should be subsumed by icu_datetime.
  • @robertbastian - I don't think the type DateTime should live in a crate that suggests it contains a timezone
  • @sffc I think I agree with that statement. I'm not sure I know or would agree where that argument is going.
  • @robertbastian I think this argument extends to timezone. We agreed that icu::zoned::DateTime is bad. I think icu::timezone::DateTime is almost as bad. In many APIs, DateTime is a zone-aware type. Many APIs don't distinguish between the two.
  • @Manishearth A point brought up earlier by @sffc that we didn't fully discuss, about namespacing like icu_text... I don't want to suggest that we apply that namespacing retroactively, but doing that namespacing here might be okay.
  • @robertbastian What if the metacrate gains a stronger ontology, like icu::text::collator but the crate is called icu_collator. Maybe we decide that icu_segmenter was not a good name; fine, we move it in the metacrate, even if we don't rename the component crate.
  • @Manishearth Mixed feelings
  • @echeran The callout about what other APIs do made me think of java.time... it has ZonedDateTime and LocalDateTime to make the distinction clearer. From that, they're not letting the length of things influence what is default or not default. It is an unopinionated approach in that naming convention.
  • @Manishearth Should we have icu_zoned::LocalDateTime.
  • @robertbastian Java has all this stuff in a time module.
  • @sffc I kind-of like @Manishearth's proposal. Maybe name it icu_zoned::PlainDateTime because that is the JS Temporal language, which was decided after extensive user research. (or civil, etc.)
  • @Manishearth When you format datetimes, you need to format a LocalDateTime or ZonedDateTime... is that good? Maybe so. I think Local is better for Plain for contrasting it with Zoned.
  • @sffc Plain is the prefix that was least confusing from a programmer clarity perspective.
  • @robertbastian Still don't think we need the prefix though.
  • @Manishearth I am still fine with time::DateTime. I am now convinced that we shouldn't do zoned::DateTime or timezone::DateTime, and if we want to do that we should prefix with Local.
  • @sffc can't make an argument for Plain (1) it's what the newest time stdlib and (2) it's the result of a user study
  • @robertbastian Java uses Local, and it's a relatively recent library that they have put tons of effort into it
  • @sffc I think ECMA is a stronger reference point than Java because we are already heavily inspired by ECMA.
  • @sffc Could we say that the crate is named icu_zoned and it exports a type named FooDateTime where Foo is to be bikeshed separately, probably landing on either Local or Plain?
  • @Manishearth fine with this, but would prefer icu_time::DateTIme
  • @robertbastian: not fine, i think there are cases for using DateTimes without needing timezone stuff. Timezones are advanced i18n. icu_calendar is not called LocalDate, because there are use cases for just using calendar/civil dates. Same use cases apply to datetimes, that are often enough for all use cases. ZDT is an extension to DT. It's not two parallel things. If you don't want to get into timezones, your project is completely in one tz, you shouldn't have to look into the timezone crate for stuff.
  • @sffc Following your logic it's an argument to move DateTime out.
  • @robertbastian no; your ontology is that you have a timezone universe with civildatetime and zoneddatetime. my ontology is that there's a time universe with datetime, time, and a zone subuniverse if you need to deal with time zones
  • @Manishearth i don't think we need to be precise about where these live. we could even do more crates/etc but we're not and i don't think we need to. it's a bit messy, it's fine.
  • @Manishearth overall i'm not convinced by either ontology: to me the important thing is if users get confused, i am now convinced zoned::DateTime and friends would confuse users, but I don't think "this is confusing within this ontology" with an ontology that needs some work to be explained in this meeting is something that will ever confuse users because there are many ontologies to get confused by.
    (missed)
  • @Manishearth I think fundamentally all ontologies around this crate will be inconsistent, BECAUSE this crate is inconsistent. All of our crates have a very central "thing" (and a couple things hanging off of it): calendar has Calendar, datetime has DateTimeFormatter. This one doesn't: it's a grab bag of random stuff related to times: Time, DateTime, ZonedDateTime (all times) and a bunch of time zone id stuff.
  • @sffc icu_timeutil? Also I haven't heard a compelling argument against icu_datetime_foo.
  • @robertbastian I think it is compelling as a standalone component. IXDTF parsing is useful.
  • @sffc Most people should probably be using jiff for that now, even though it didn't exist when we implemented the ixdtf crate last year.
  • @robertbastian why are we launching it then? we shouldn't launch functionality while saying "you should use this other crate"
  • @Manishearth it feels like a core issue here is "is ixdtf parsing a core bit of functionality or 'random crap'"
  • @sffc I still have trouble pinpointing the users of this crate. Most users who just want to parse IXDTF should use Jiff. And the only reason to use the timezone calculation is in the process of formatting.
  • @robertbastian Our IXDTF parsing is more fully-featured. We support parsing different subsets of dates, times, and zones, and we support the calendar extension.
  • @sffc Okay, thanks. I think that could be a compelling argument that it is its own component. Not fully convinced. If this were its own component, icu_timeutil seems best. It avoids the icu_time confusion.

Crate Bikeshed:

  1. icu_timezone::{DateTime, ZonedDateTime, ...}
  2. icu_time::{Time, DateTime, ZonedDateTime, zone::{...}, ...}
  3. icu_datetime_foo::{DateTime, ZonedDateTime, ...}
  4. icu_zoned::{BikeshedDateTime, ZonedDateTime, ...}
  5. icu_timeutil

Votes:

  • Manish: 2 > 3₂ > 5 ~>> 1 ~> 4 > 3₁,3₃
  • Shane: 5 ~> 3 > 2 > 4 > 1
  • Rob: 2 ~>> 1 > 4 ~>> 5 >> 3

For whether we should rename DateTime to BikeshedDateTime (necessary for some proposals)

  • Shane: Yes ~>> No
  • Manish: Yes ~> No
  • Rob: No ~>> Yes

For BikeshedDateTime:

  1. LocalDateTime
  2. PlainDateTime

Votes:

  • Shane: 2 ~>> 1
  • Manish: 1 ~>> 2

For foo:

  1. parsing
  2. types
  3. zone
  • Manish: 2 ~>> 3 ~> 1
  • Shane: 2 ~> 3 ~>> 1

(Further discussion paring down variants, boiling down ultimately to icu_time vs icu_timezone)

Summary: Currently: Manish prefers icu_time, also likes icu_timeutil. Robert prefers icu_time and dislikes icu_timeutil. Shane prefers icu_timeutil, but will not veto icu_time.

The core disagreements are:

  • What is the most important part of this crate? Is it just all equal, or is the ixdtf parsing its most prominent part, or something else?
  • Will icu_time mislead users into thinking that this is a time formatting crate?
  • Will icu_timeutil deemphasize what is important in this crate? Will it mislead users into thinking that it doesn't contain anything?

General direction seems to be icu_time unless we have further discussion.

@robertbastian
Copy link
Member Author

robertbastian commented Feb 10, 2025

Updated proposal:

Time type and subtypes at icu::time root. Remove Iso prefix from HMS because NanoSecond doesn't have it either, and the ISO model is not very special.

Table 1

Current Name Proposed Name
icu::timezone::Time icu::time::Time
icu::timezone::types::IsoHour icu::time::Hour
icu::timezone::types::IsoMinute icu::time::Minute
icu::timezone::types::IsoSecond icu::time::Second
icu::timezone::types::NanoSecond icu::time::Nanosecond

Table 2

TimeZoneInfo, composite types and parser at icu::time root.

Current Name Proposed Name
icu::timezone::DateTime<A> icu::time::DateTime<A>
icu::timezone::TimeZoneInfo<M> icu::time::TimeZoneInfo<M>
icu::timezone::ZonedDateTime<A> icu::time::ZonedDateTime<A>
icu::timezone::IxdtfParser icu::time::ZonedDateTimeParser
icu::timezone::InvalidOffsetError icu::time::InvalidOffsetError
icu::timezone::ParseError icu::time::ParseError

Table 3

Time zones things in a icu::time::zone module, using TimeZone and not just Zone:

Current Name Proposed Name
icu::timezone::TimeZoneBcp47Id icu::time::zone::TimeZone
icu::timezone::ZoneVariant icu::time::zone::TimeZoneVariant
icu::timezone::UtcOffset icu::time::zone::UtcOffset
icu::timezone::ZoneOffsets icu::time::zone::UtcOffsets
icu::timezone::ZoneOffsetCalculator icu::time::zone::UtcOffsetCalculator

Table 4

Time zone info models also in icu::time::zone because there isn't really a better place:

Current Name Proposed Name
icu::timezone::TimeZoneModel icu::time::zone::TimeZoneInfoModel
icu::timezone::models::AtTime icu::time::zone::models::AtTime
icu::timezone::models::Base icu::time::zone::models::Base
icu::timezone::models::Full icu::time::zone::models::Full

Table 5

IANA conversions in icu::time::zone::iana:

Current Name Proposed Name
icu::timezone::TimeZoneIdMapper[Borrowed] icu::time::zone::iana::IanaParser[Borrowed,...]
icu::timezone::TimeZoneIdMapperWithFastCanonicalization[Borrowed] icu::time::zone::iana::IanaParserExtended[Borrowed]

Table 6

Windows conversions in icu::time::zone::windows:

Current Name Proposed Name
icu::timezone::WindowsTimeZoneMapper[Borrowed] icu::time::zone::windows::WindowsMapper[Borrowed]

@BurntSushi
Copy link

Our IXDTF parsing is more fully-featured [than Jiff's]. We support parsing different subsets of dates, times, and zones, and we support the calendar extension.

I'm not sure precisely what is meant by different subsets of dates, times and zones, but a somewhat more recent addition to Jiff is jiff::fmt::temporal::Pieces. Which gives more direct access to the specific parsed components. I'm not sure if that's what you meant thought.

With respect to calendar extensions, I would be open to adding lower level parsing support for that. That shouldn't be much work and it seems like it'd be worth supporting if it's possible to do. And also potentially find a way to integrate that in a soon-to-be-published jiff-icu crate.

With respect to renaming DateTime, I do like PlainDateTime for y'all as a way of contrasting it with ZonedDateTime. But I get why y'all settled on unprefixed (I think).

@Manishearth
Copy link
Member

With respect to renaming DateTime, I do like PlainDateTime for y'all as a way of contrasting it with ZonedDateTime. But I get why y'all settled on unprefixed (I think).

We didn't necessarily settle on unprefixed, it was a different discussion that didn't really get chased down to conclusion, but with the current naming proposal unprefixed works fine, we might prefix it.

Thanks for the point in favor of PlainDateTime, a Rust ecosystem person finding it clear makes me like it more (I prefer LocalDateTime, but now less so)

@BurntSushi
Copy link

Ah I misunderstood the votes/summary thing. Hah.

I like Civil the best personally (hence why Jiff uses it), but I like Plain over Local. For the most part, the opinion is more of one against Local than anything else. I think it is slightly overloaded in its meaning and can lead to confusing interpretations. With that said, Local is very widely used, so it's hard to really "go wrong" there. Y'all would be in good company!

@Manishearth
Copy link
Member

Ah I misunderstood the votes/summary thing. Hah.

Yeah in this case that was more of a way for us to communicate preferences to each other, and then we had more discussion after that

That's a compelling (to me) reason to dislike Local!

@BurntSushi
Copy link

QFE:

One other thing I'd point out is a potential incongruity with how Nanosecond is named. It matches the names of other units like Hour, Minute and Second, but its actual value is more like, "fractional seconds, to nanosecond precision." This becomes potentially confusable in contexts where you want to represent or expose milliseconds, microseconds and nanoseconds as distinct units. For example, std::time::Duration uses the subsec_ prefix to disambiguate between these concepts on the std::time::Duration type. I do the same in Jiff for its civil::Time type.

Here is an example where someone got confused by this: BurntSushi/jiff#261

Chrono has the same naming convention as y'all, and that got carried over to Jiff and resulted in a mismatched expectation. Jiff's naming convention is closer to std, and has both "nanoseconds as a component on its own, in the range [0, 999]" and "sub-second nanoseconds as fractional seconds represented with nanosecond precision, in the range [0, 999999999]."

@robertbastian
Copy link
Member Author

I think issues like that are less of a problem for the type name, and more for a getter.
Time::try_from(Hour, Minute, Second, Nanosecond) is pretty clear, there's no microsecond field so Nanosecond probably covers it.

We might want to call the field/getter subsecond. The type should still be Nanosecond, because Subsecond doesn't tell you what the number is actually counting.

@Manishearth
Copy link
Member

Here is an example where someone got confused by this: BurntSushi/jiff#261

Heh, I got hit by this same confusion when hacking around V8 and trying to figure out how to pass their time (split into milli/micro/nanos) to our DateTime (not split).

@robertbastian
Copy link
Member Author

robertbastian commented Feb 12, 2025

We could have a constructor Nanosecond::from_millis_micros_nanos(u32, u32, u32)

@Manishearth
Copy link
Member

Oh absolutely,but I was using FFI so things were less clear overall anyawy. It's not a big deal, jsut wanted to note that I've been confused by this type of thing before,

@sffc
Copy link
Member

sffc commented Feb 12, 2025

Maybe just name it FractionalSecond and document that the field is in terms of nanoseconds?

@sffc
Copy link
Member

sffc commented Feb 14, 2025

Additional dicussion on XDateTime:

  • @sffc I think we should rename DateTime to XDateTime because:
    1. We know that DateTime is confusing, because there is evidence that programmers may assume that it is associated with a time zone
    2. Many pre-existing datetime libraries use this naming convention
    3. Eliminates the confusion I've noted earlier that DateTimeFormatter formats things other than DateTime
  • @sffc I would like the decision on "what is X" to be decoupled from the decision on whether to rename it to XDateTime.
  • @Manishearth I find (3) to be compelling.
  • @robertbastian I find only (3) to be compelling.
  • @Manishearth I don't think we can decouple it entirely, but I'm generally in agreement that we should look for a better name. But I don't want us to come up with a name that is worse than DateTime.
  • @robertbastian I consider these tuple types: DateTime is date and time; ZonedDateTime is zone and date and time. We have a field set called TimeZone which is time and zone.
  • @sffc Should we rename ZonedDateTime to DateTimeZone in order to be more consistent?
  • @robertbastian Maybe. But then it looks like it could be a Date with a Timezone.
  • @Manishearth I like the symmetry of XDateTime and ZonedDateTime.
  • @robertbastian It's not symmetric: ZonedDateTime builds on XDateTime. There is a superset relationship.
  • @Manishearth Figuring out what is being talked about is useful at times. Documenting DateTime is fine, but XDateTime is probably better. Even if we don't care about the distinction, our users probably do.
  • @robertbastian DateTime is a thin type. Just a tuple.
  • @sffc Regarding consistency with other libraries, it should be noted that Jiff and Temporal, and maybe others, use the naming convention not only for DateTime but also for Date and Time. For example, Temporal.PlainDate, PlainTime, and PlainDateTime. Jiff uses a civil module.
  • @Manishearth It's only for DateTime where this is a problem.
  • @sffc In ECMA, they wanted to apply the convention to both Date and DateTime because there is a JS type named Date. I think Time followed from that. And Jiff followed from them.
  • @Manishearth Yeah, that's useful; we should stick with only XDateTime if we do the change.
  • @robertbastian ICU4X doesn't do Instant calculations. The name is about what functionality a user expects from an object.
  • @sffc Do we reject IXDTF strings containing time zones in DateTime::try_from_str? I think it should; you should parse to the type that corresponds to your string format. This would be different than what Temporal does, but I think it would make sense for ICU4X input types.
  • @sffc The reason I bring this up is because there are only 2 ways to construct a DateTime: DateTime { date, time } and DateTime::try_from_str("YYYY-MM-DDThh:mm"). I think in terms of code readability, it's fairly clear that there are no time zones involved here.
  • @robertbastian We can add try_from_str_loose later.
  • @Manishearth Or extract_from_str.
  • @sffc Or people can use the ixdtf crate directly, if they need this type of control over exactly how the strings get parsed.

Conclusion:

  1. Keep DateTime the way it is
  2. Make sure that the try_from_str impls reject strings containing superfluous fields

Additional discussion:

  • @sffc I think @BurntSushi's concern about Nanosecond is valid
  • @Manishearth I think it makes sense that Nanosecond is in a field called subsecond
  • @sffc We use the language "fractional second" everywhere, not "subsecond". Should we change it?
  • @robertbastian - I think "fractional second" is actually quite confusing, we have an ixdtf PR that describes an error as "invalid fraction", that's really not it

Approval for the tables in #5999 (comment):


Approval for Table 1:

  • Caveat: name the field "subsecond"

@robertbastian @Manishearth @sffc


Approval for Table 2:

  • Caveat: Yes, but ZonedDateTimeParser is being removed and turned into methods as discussed elsewhere
  • Small discussion around TimeZoneInfo, sticking with the same name with recognition that users should not be directly naming this type most of the time

@robertbastian @Manishearth @sffc


Approval for Table 3:

@sffc @Manishearth @robertbastian


Approval for Table 4:

  • Change: rename models to model
  • Change:TimeZoneInfoModel also lives in the model module

@sffc @Manishearth @robertbastian


Approval for Table 5:

  • Caveat: keep all 6 types in zone::iana and re-export just IanaParser from zone

@sffc @robertbastian @Manishearth


Approval for Table 6:

  • Change: WindowsMapper should be WindowsParser
  • Caveat: re-export WindowsParser from zone; it will nicely mirror IanaParser

@sffc @robertbastian @Manishearth

@sffc sffc removed discuss-priority Discuss at the next ICU4X meeting needs-approval One or more stakeholders need to approve proposal labels Feb 14, 2025
robertbastian added a commit that referenced this issue Feb 14, 2025
I've not prettified docs, because they'll be touched again by #6069 and
#5999.

Part of #6119
@sffc
Copy link
Member

sffc commented Feb 18, 2025

Change discussed with @robertbastian, @sffc: move icu::time::InvalidOffsetError to icu::time::zone::InvalidOffsetError

@Manishearth
Copy link
Member

LGTM too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-time-zone Component: Time Zones
Projects
None yet
Development

No branches or pull requests

6 participants