-
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
How to implement ordering on AnyCalendarInner? #3469
Comments
|
I think approach (3) should be a separate function. I think we should use preferably (2) but (1) is also fine. I think date comparison is a complex situation anyway and we should have good docs telling people what the different options are. In such a scenario But also, the RataDie type fixed the problem wrt bounds. If people want to use these dates as BTreeMap keys then |
Proposals:
Consensus:
LGTM: @robertbastian @Manishearth @sffc @echeran @sotam1069 @atcupps |
There are a few general approaches:
Temporal uses approach (3), but it's a non-starter for the
Ord
impl because it must be a total order.If we can get to (2), that's probably the best for ICU4X's purposes, but it's actually a bit tricky because there is not currently a space into which we can project the full range of dates in all calendars.
fixed_date
is limited toi32
, and the range of years in some calendars covers a different span than the range of years in others. One way to fix that problem is to changefixed_date
to be ani64
, which I think is a change we should make anyway.However, approach (1) works around both of these problems, and we can even use
derive(Ord)
. It's maybe not that bad from a user perspective, too.Another way to get out of this conundrum is to add different types of Ord functions like we did to Locale (#1709).
Discuss with:
Optional:
The text was updated successfully, but these errors were encountered: