Add Ord and PartialOrd to icu_calendar types #3468
Conversation
| @@ -92,7 +92,7 @@ pub enum AnyCalendar { | |||
| } | |||
|
|
|||
| /// The inner date type for [`AnyCalendar`] | |||
| #[derive(Clone, PartialEq, Eq, Debug)] | |||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug)] | |||
There was a problem hiding this comment.
I guess this does Self::Gregorian(x) < Self::Buddhist(y)? This will make Date<AnyCalendar> ordered non-chronologically.
There was a problem hiding this comment.
Hmm, yeah I guess it will put all Gregorian before all Buddhist. I can leave this out for now. I'll make a follow-up.
There was a problem hiding this comment.
It should be fairly straightforward to implement this manually, as all calendars can convert to ISO.
There was a problem hiding this comment.
I'll include this in my follow-up
There was a problem hiding this comment.
So this is not as straightforward as it initially seems, because the calendars have different ranges. We can't convert them to fixed_date (number of days since 0000-1-1) because fixed_date is an i32, but the calendar objects support dates outside that range; a calendar with a year field of i32::MAX can't necessarily be converted to another one. (@sotam1069 is working on finding these edge cases and making sure they are at least covered in unit tests)
| @@ -7,7 +7,7 @@ use core::convert::TryInto; | |||
| use core::marker::PhantomData; | |||
| use tinystr::tinystr; | |||
|
|
|||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)] | |||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] | |||
There was a problem hiding this comment.
I would ask you not to make this require C: PartialOrd + Ord, but I guess that ship has sailed with Hash, Eq, PartialEq.
Needed for #2703