-
Notifications
You must be signed in to change notification settings - Fork 262
Document the limits on the Date type #7544
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
base: main
Are you sure you want to change the base?
Conversation
Manishearth
left a comment
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 agree with these
components/calendar/src/date.rs
Outdated
| /// | ||
| /// 1. Dates can always be converted between calendars | ||
| /// 2. Dates can always be round-tripped through [`RataDie`] | ||
| /// 3. Every possible Date can be constructed from an RFC 9557 string |
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.
"Every date that is possible to acquire through usage of ICU4X APIs can be constructed from ..."
components/calendar/src/date.rs
Outdated
| /// `new_<calendar>_date()` per-calendar methods (and then freely converted between calendars). | ||
| /// # Limits | ||
| /// | ||
| /// Constructors may impose limits on the largest or smallest date that they allow, |
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.
nit: may impose documented limits
Though for the RD range I would prefer to not document the values, instead having the constructor say "an internal range which is at least the range of all possible dates that can be constructed"
OR we list a range and explicitly say it is subject to change.
| /// 1. Generically from fields via [`Self::try_from_fields()`] or [`Self::try_new_from_codes()`] | ||
| /// 2. With calendar-specific constructors, typically named `try_new_xyz()` | ||
| /// 3. From a RFC 9557 string via [`Self::try_from_str()`] | ||
| /// 4. From a [`RataDie`] via [`Self::from_rata_die()`] |
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.
also list arithmetic here? Maybe not because it's currently unstable.
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.
arithmetic needs you to have a Date already, so it's not really creating one from scratch. Same with to_calendar()
components/calendar/src/date.rs
Outdated
| /// | ||
| /// 1. Dates can always be converted between calendars | ||
| /// 2. Dates can always be round-tripped through [`RataDie`] | ||
| /// 3. Every possible Date can be constructed from an RFC 9557 string |
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.
this is where I disagree. this phrasing means the the RD range needs to be smaller than the IXDTF range (-999999-01-01..=999999-12-31), making the Date -> RFC9557 conversion infallible (an operation which we don't have). I want the RD range to be larger than the IXDTF range, making the RFC-9557 -> Date conversion infallible (well, less fallible). I think from a user perspective this also makes more sense:
| /// 3. Every possible Date can be constructed from an RFC 9557 string | |
| /// 3. Every valid RFC 9557 string can be parsed into a [`Date`] |
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.
The "IXDTF range" is not quite a thing, nor is it meaningful to talk about "valid RFC 9557 string"s when applying a date range.
As I mentioned before, the ISO specification allows for an "expanded date" but does not specify a number of digits.
The Temporal specification chooses a 6-digit expanded date format, which means it is able to parse up to 999999. However, it also caps dates at ~270k, so 999999-12-31 is not an actual valid value. There is no Temporal code where dates between ~270k and 999999 are meaningful.
There is no specification or software out there that will accept 999999-12-31 but not 1000000-01-01, except for ICU4X's ixdtf parser. Most things out there reject extended dates completely. If something supports extended dates, it picks its own number of digits. Temporal has picked 6 and imposed further constraints on it.
So I don't see a reason to treat 999999-12-31 as a bound handed to us from above. The only place that it comes from is from a partial reading of a particular standard.
|
AFAICT, the 6-digit years are a Temporal grammar limitation, not an IXDTF or ISO-8601 limitation. There are multiple variables, some of which we can tweak, some of which we cannot:
Given that we care about interchange with Temporal, as an "interchange party" we should probably also pick 6-digit years in our syntax. |
|
And note that specifically the "temporal syntax range" comes from lifting a part of the standard and implementing it as a separate crate. It's an artifact of code organization. |
|
My two cents (alluded in another thread): I see @robertbastian's point about But, I want for all ICU4X Dates to be represented as a string. Although we don't currently have serialization, it is something I see in scope. This is why I prefer setting the RD range to be equal to the Temporal syntax range. Then, the constructor ranges can be whatever they like for ease of documentation. +/- 500k years seems fine, or +/- 900k. We can't do +/- 1M in my model, though, because it would exceed the Temporal syntax range. |
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.
Currently:
- ✅ Every RFC 9557 string that the
ixdtfcrate parses is a validDate- If the
ixdtfcrate were to be extended to parse arbitrary-length years, we would need to apply either the RD range, or the Gregorian year range (the latter would give better errors)
- If the
- ✅ We could produce an RFC 9557 string for every
Date- This is not an operation that we currently have
- ISO 8601 defines either a 4-digit year, or an arbitrary digit year. So, unless we want to restrict ourselves to 4-digit years we can produce an RFC 9557 string for any date.
- ❌ "Every possible Date can be constructed from an RFC 9557 string"
- This requirement is being added in this PR, but I don't understand why.
- This conceptually works, but practically doesn't because the
ixdtfcrate only supports 6-digit years - Is it intended to be a restatement of "We could produce an RFC 9557 string for every
Date"? Because in that direction we're not limited by theixdtfcrate
Now for serialization: we don't have it. Temporal has it, and it's not an issue for them, because their syntax range is larger than their value range. They don't need any invariants from us for this.
| /// Options to create one of these: | ||
| /// | ||
| /// 1. Generically from fields via [`Self::try_from_fields()`] or [`Self::try_new_from_codes()`] | ||
| /// 2. With calendar-specific constructors, typically named `try_new_xyz()` |
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.
| /// 2. With calendar-specific constructors, typically named `try_new_xyz()` | |
| /// 2. With calendar-specific constructors, e.g. [`Self::try_new_chinese_traditional()`] |
| /// 1. Generically from fields via [`Self::try_from_fields()`] or [`Self::try_new_from_codes()`] | ||
| /// 2. With calendar-specific constructors, typically named `try_new_xyz()` | ||
| /// 3. From a RFC 9557 string via [`Self::try_from_str()`] | ||
| /// 4. From a [`RataDie`] via [`Self::from_rata_die()`] |
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.
arithmetic needs you to have a Date already, so it's not really creating one from scratch. Same with to_calendar()
| /// This can work with wrappers around [`Calendar`] types, | ||
| /// e.g. `Rc<C>`, via the [`AsCalendar`] trait. |
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.
nit: might want to remove this now. there's no point in wrapping any of our calendars at least, so this might be confusing
| /// Constructors may impose documented limits on the largest or smallest date that they allow, | ||
| /// subject to the following invariants: | ||
| /// | ||
| /// 1. Dates can always be converted between calendars |
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.
| /// 1. Dates can always be converted between calendars | |
| /// 1. Dates can always be converted between calendars ([`Self::to_calendar`]) |
| /// subject to the following invariants: | ||
| /// | ||
| /// 1. Dates can always be converted between calendars | ||
| /// 2. Dates can always be round-tripped through [`RataDie`] |
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.
| /// 2. Dates can always be round-tripped through [`RataDie`] | |
| /// 2. Dates can always be round-tripped through [`RataDie`] ([`Self::to_rata_die`], [`Self::from_rata_die()`]) |
| /// | ||
| /// 1. Dates can always be converted between calendars | ||
| /// 2. Dates can always be round-tripped through [`RataDie`] | ||
| /// 3. Every date that is possible to acquire through usage of ICU4X APIs, including calendar |
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.
don't say ICU4X
| /// 3. Every date that is possible to acquire through usage of ICU4X APIs, including calendar | ||
| /// conversion and arithmetic, can be constructed from an RFC 9557 string |
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 don't understand why you changed this to be so complicated. Dates can obviously only be obtained from APIs in this crate, because its fields are private
| /// 3. Every date that is possible to acquire through usage of ICU4X APIs, including calendar | |
| /// conversion and arithmetic, can be constructed from an RFC 9557 string | |
| /// 3. Every [`Date`] can be constructed from an RFC 9557 string |
This is a technical point, I still don't agree with this invariant
We've wanted it. I think @nekevss has been planning on contributing it upstream to
Right, but we can fix that by changing the limits. |
We keep talking past each other when it comes to date ranges, so I wrote down some invariants of what I think we should be trying to implement.