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.

Sign up for GitHub

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

Add Ord and PartialOrd to icu_calendar types #3468

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Add Ord and PartialOrd to icu_calendar types #3468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/calendar/src/any_calendar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this does Self::Gregorian(x) < Self::Buddhist(y)? This will make Date<AnyCalendar> ordered non-chronologically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fairly straightforward to implement this manually, as all calendars can convert to ISO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll include this in my follow-up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Ord?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3469

#[non_exhaustive]
pub enum AnyDateInner {
/// A date for a [`Gregorian`] calendar
Expand Down
37 changes: 36 additions & 1 deletion components/calendar/src/calendar_arithmetic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would ask you not to make this require C: PartialOrd + Ord, but I guess that ship has sailed with Hash, Eq, PartialEq.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on both counts.

#[allow(clippy::exhaustive_structs)] // this type is stable
pub struct ArithmeticDate<C: CalendarArithmetic> {
pub year: i32,
Expand Down Expand Up @@ -250,3 +250,38 @@ pub fn ordinal_solar_month_from_code(code: types::MonthCode) -> Option<u8> {
}
None
}

#[cfg(test)]
mod tests {
use super::*;
use crate::Iso;

#[test]
fn test_ord() {
let dates_in_order = [
ArithmeticDate::<Iso>::new(-10, 1, 1),
ArithmeticDate::<Iso>::new(-10, 1, 2),
ArithmeticDate::<Iso>::new(-10, 2, 1),
ArithmeticDate::<Iso>::new(-1, 1, 1),
ArithmeticDate::<Iso>::new(-1, 1, 2),
ArithmeticDate::<Iso>::new(-1, 2, 1),
ArithmeticDate::<Iso>::new(0, 1, 1),
ArithmeticDate::<Iso>::new(0, 1, 2),
ArithmeticDate::<Iso>::new(0, 2, 1),
ArithmeticDate::<Iso>::new(1, 1, 1),
ArithmeticDate::<Iso>::new(1, 1, 2),
ArithmeticDate::<Iso>::new(1, 2, 1),
ArithmeticDate::<Iso>::new(10, 1, 1),
ArithmeticDate::<Iso>::new(10, 1, 2),
ArithmeticDate::<Iso>::new(10, 2, 1),
];
for (i, i_date) in dates_in_order.iter().enumerate() {
for (j, j_date) in dates_in_order.iter().enumerate() {
let result1 = i_date.cmp(j_date);
let result2 = j_date.cmp(i_date);
assert_eq!(result1.reverse(), result2);
assert_eq!(i.cmp(&j), i_date.cmp(j_date));
}
}
}
}
4 changes: 2 additions & 2 deletions components/calendar/src/coptic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ use tinystr::tinystr;
///
/// This calendar supports two era codes: `"bd"`, and `"ad"`, corresponding to the Before Diocletian and After Diocletian/Anno Martyrum
/// eras. 1 A.M. is equivalent to 284 C.E.
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq, PartialOrd, Ord)]
#[allow(clippy::exhaustive_structs)] // this type is stable
pub struct Coptic;

/// The inner date type used for representing [`Date`]s of [`Coptic`]. See [`Date`] and [`Coptic`] for more details.
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
pub struct CopticDateInner(pub(crate) ArithmeticDate<Coptic>);

impl CalendarArithmetic for Coptic {
Expand Down
57 changes: 57 additions & 0 deletions components/calendar/src/date.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,29 @@ where

impl<A: AsCalendar> Eq for Date<A> {}

impl<C, A, B> PartialOrd<Date<B>> for Date<A>
where
C: Calendar,
C::DateInner: PartialOrd,
A: AsCalendar<Calendar = C>,
B: AsCalendar<Calendar = C>,
{
fn partial_cmp(&self, other: &Date<B>) -> Option<core::cmp::Ordering> {
self.inner.partial_cmp(&other.inner)
}
}

impl<C, A> Ord for Date<A>
where
C: Calendar,
C::DateInner: Ord,
A: AsCalendar<Calendar = C>,
{
fn cmp(&self, other: &Self) -> core::cmp::Ordering {
self.inner.cmp(&other.inner)
}
}

impl<A: AsCalendar> fmt::Debug for Date<A> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
write!(
Expand All @@ -381,3 +404,37 @@ where
<<A as AsCalendar>::Calendar as Calendar>::DateInner: Copy,
{
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_ord() {
let dates_in_order = [
Date::try_new_iso_date(-10, 1, 1).unwrap(),
Date::try_new_iso_date(-10, 1, 2).unwrap(),
Date::try_new_iso_date(-10, 2, 1).unwrap(),
Date::try_new_iso_date(-1, 1, 1).unwrap(),
Date::try_new_iso_date(-1, 1, 2).unwrap(),
Date::try_new_iso_date(-1, 2, 1).unwrap(),
Date::try_new_iso_date(0, 1, 1).unwrap(),
Date::try_new_iso_date(0, 1, 2).unwrap(),
Date::try_new_iso_date(0, 2, 1).unwrap(),
Date::try_new_iso_date(1, 1, 1).unwrap(),
Date::try_new_iso_date(1, 1, 2).unwrap(),
Date::try_new_iso_date(1, 2, 1).unwrap(),
Date::try_new_iso_date(10, 1, 1).unwrap(),
Date::try_new_iso_date(10, 1, 2).unwrap(),
Date::try_new_iso_date(10, 2, 1).unwrap(),
];
for (i, i_date) in dates_in_order.iter().enumerate() {
for (j, j_date) in dates_in_order.iter().enumerate() {
let result1 = i_date.cmp(j_date);
let result2 = j_date.cmp(i_date);
assert_eq!(result1.reverse(), result2);
assert_eq!(i.cmp(&j), i_date.cmp(j_date));
}
}
}
}
52 changes: 52 additions & 0 deletions components/calendar/src/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,32 @@ where
// We can do this since DateInner is required to be Eq by the Calendar trait
impl<A: AsCalendar> Eq for DateTime<A> {}

impl<C, A, B> PartialOrd<DateTime<B>> for DateTime<A>
where
C: Calendar,
C::DateInner: PartialOrd,
A: AsCalendar<Calendar = C>,
B: AsCalendar<Calendar = C>,
{
fn partial_cmp(&self, other: &DateTime<B>) -> Option<core::cmp::Ordering> {
match self.date.partial_cmp(&other.date) {
Some(core::cmp::Ordering::Equal) => self.time.partial_cmp(&other.time),
other => other,
}
}
}

impl<C, A> Ord for DateTime<A>
where
C: Calendar,
C::DateInner: Ord,
A: AsCalendar<Calendar = C>,
{
fn cmp(&self, other: &Self) -> core::cmp::Ordering {
(&self.date, &self.time).cmp(&(&other.date, &other.time))
}
}

impl<A: AsCalendar + Clone> Clone for DateTime<A> {
fn clone(&self) -> Self {
Self {
Expand All @@ -152,3 +178,29 @@ where
<<A as AsCalendar>::Calendar as Calendar>::DateInner: Copy,
{
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_ord() {
let dates_in_order = [
DateTime::try_new_iso_datetime(0, 1, 1, 0, 0, 0).unwrap(),
DateTime::try_new_iso_datetime(0, 1, 1, 0, 0, 1).unwrap(),
DateTime::try_new_iso_datetime(0, 1, 1, 0, 1, 0).unwrap(),
DateTime::try_new_iso_datetime(0, 1, 1, 1, 0, 0).unwrap(),
DateTime::try_new_iso_datetime(0, 1, 2, 0, 0, 0).unwrap(),
DateTime::try_new_iso_datetime(0, 2, 1, 0, 0, 0).unwrap(),
DateTime::try_new_iso_datetime(1, 1, 1, 0, 0, 0).unwrap(),
];
for (i, i_date) in dates_in_order.iter().enumerate() {
for (j, j_date) in dates_in_order.iter().enumerate() {
let result1 = i_date.cmp(j_date);
let result2 = j_date.cmp(i_date);
assert_eq!(result1.reverse(), result2);
assert_eq!(i.cmp(&j), i_date.cmp(j_date));
}
}
}
}
4 changes: 2 additions & 2 deletions components/calendar/src/ethiopian.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,11 @@ pub enum EthiopianEraStyle {
/// the `"incar"` and `"pre-incar"` eras, 1 Incarnation is 9 CE. In the Amete Alem scheme, it instead has a single era,
/// `"mundi`, where 1 Anno Mundi is 5493 BCE. Dates before that use negative year numbers.
// The bool specifies whether dates should be in the Amete Alem era scheme
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq, PartialOrd, Ord)]
pub struct Ethiopian(pub(crate) bool);

/// The inner date type used for representing [`Date`]s of [`Ethiopian`]. See [`Date`] and [`Ethiopian`] for more details.
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
pub struct EthiopianDateInner(ArithmeticDate<Ethiopian>);

impl CalendarArithmetic for Ethiopian {
Expand Down
2 changes: 1 addition & 1 deletion components/calendar/src/gregorian.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use tinystr::tinystr;
#[allow(clippy::exhaustive_structs)] // this type is stable
pub struct Gregorian;

#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
/// The inner date type used for representing [`Date`]s of [`Gregorian`]. See [`Date`] and [`Gregorian`] for more details.
pub struct GregorianDateInner(IsoDateInner);

Expand Down
4 changes: 2 additions & 2 deletions components/calendar/src/indian.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ use tinystr::tinystr;
/// # Era codes
///
/// This calendar has a single era: `"saka"`, with Saka 0 being 78 CE. Dates before this era use negative years.
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Default, Eq, PartialEq, PartialOrd, Ord)]
#[allow(clippy::exhaustive_structs)] // this type is stable
pub struct Indian;

/// The inner date type used for representing [`Date`]s of [`Indian`]. See [`Date`] and [`Indian`] for more details.
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
pub struct IndianDateInner(ArithmeticDate<Indian>);

impl CalendarArithmetic for Indian {
Expand Down
4 changes: 2 additions & 2 deletions components/calendar/src/iso.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ const EPOCH: i32 = 1;
///
/// This calendar supports one era, `"default"`

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, Hash)]
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[allow(clippy::exhaustive_structs)] // this type is stable
pub struct Iso;

#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
/// The inner date type used for representing [`Date`]s of [`Iso`]. See [`Date`] and [`Iso`] for more details.
pub struct IsoDateInner(pub(crate) ArithmeticDate<Iso>);

Expand Down
2 changes: 1 addition & 1 deletion components/calendar/src/japanese.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub struct Japanese {
#[derive(Clone, Debug, Default)]
pub struct JapaneseExtended(Japanese);

#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
/// The inner date type used for representing [`Date`]s of [`Japanese`]. See [`Date`] and [`Japanese`] for more details.
pub struct JapaneseDateInner {
inner: IsoDateInner,
Expand Down