Expose equals and compare over FFI#269
Conversation
…me, and PlainYearMonth
…lainTime, and PlainYearMonth
|
Thanks for regenerating the FFI bindings Probably good to add C++ tests in https://github.com/boa-dev/temporal/blob/main/temporal_capi/cpp_tests/simple.cpp CC @Manishearth |
| ) | ||
| } | ||
|
|
||
| pub fn compare_iso_year_month(year1: i32, month1: u8, year2: i32, month2: u8) -> i32 { |
There was a problem hiding this comment.
suggestion: write this as a match on (year1, month1).cmp((year2, month2)) instead
And link to the spec entry
There was a problem hiding this comment.
also, thought: I don't think it would be a bad idea to just have a method return std::cmp::Ordering here, that converts to the integer anyway, and makes it easier to add comparator support later
There was a problem hiding this comment.
should i do this for all the new compare funcitons?
| pub fn compare_iso_month_day(month1: u8, day1: u8, month2: u8, day2: u8) -> i32 { | ||
| if month1 > month2 { | ||
| 1 | ||
| } else if month1 < month2 { | ||
| -1 | ||
| } else if day1 > day2 { | ||
| 1 | ||
| } else if day1 < day2 { | ||
| -1 | ||
| } else { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Issue: PlainMonthDay does not have a compare function
| ) | ||
| } | ||
|
|
||
| pub fn compare_iso_date( |
There was a problem hiding this comment.
Please use the library's compare function. Don't write your own. only write a thing wrapper.
There was a problem hiding this comment.
Thanks for working on this! I'm adding a couple early review comments as I believe you're still looking at adding some tests.
I think using the native rust methods wherever possible is the best approach as it assures that the compare methods work the same between the Rust implementation and the FFI.
EDIT: Just noticed, Manish and Shane's comments. Feel free to disregard my review coments as I believe we have the same comments.
| self.0 == other.0 | ||
| } | ||
|
|
||
| pub fn compare(one: &Self, two: &Self) -> i32 { |
There was a problem hiding this comment.
suggestion: I believe there should be a simpler way to implement these.
The built-ins that have an IsoDate internally should have a method called compare_iso that does this work for you.
The exception to this I believe is the time oriented types, which should implement PartialOrd and Ord. In these cases, you can use Ord's method cmp and cast Ordering into an i8. Although, ZonedDateTime implements a compare_instant method instead of an Ord implementation.
| .iter() | ||
| .find(|&&ord| ord != std::cmp::Ordering::Equal) | ||
| .map_or(0, |ord| match ord { | ||
| std::cmp::Ordering::Greater => 1, |
There was a problem hiding this comment.
thought: Ordering is repr(i8), so you should be able to cast it. Somthing like: i32::from(ord as i8)
There was a problem hiding this comment.
Also diplomat supports returning Ordering over FFI, since it supports custom comparators.
| microsecond2: u16, | ||
| nanosecond2: u16, | ||
| ) -> i32 { | ||
| let comparisons = [ |
There was a problem hiding this comment.
issue: no, this should just use (year1, month1, ...).cmp((year2, month2, ...))
…ay, and PlainYearMonth for improved clarity and efficiency
|
Tried to follow your feedback, please give me more feedback if i havent implemented it correctly. |
|
|
||
| pub fn compare_iso_year_month(year1: i32, month1: u8, year2: i32, month2: u8) -> i32 { | ||
| match (year1, month1).cmp(&(year2, month2)) { | ||
| std::cmp::Ordering::Less => -1, |
There was a problem hiding this comment.
nit: core::cmp::Ordering
There was a problem hiding this comment.
but also, you can just as i32 here
but also, I think all of these APIs should just return core::cmp::Ordering directly.
There was a problem hiding this comment.
Hey, thanks for the respone, should i do this for all the files.
I.e:
plain_date_time.rs
plain_date.rs
plain_month_day.rs
plain_time.rs
plain_year_month.rs
im sorry if this might be a silly question, but im not so used to the github lingo just yet
Here is my proposed new code for compare in plain_month_day.rs
pub fn compare_iso_year_month(
year1: i32,
month1: u8,
year2: i32,
month2: u8,
) -> core::cmp::Ordering {
(year1, month1).cmp(&(year2, month2))
}
pub fn compare(one: &Self, two: &Self) -> core::cmp::Ordering {
Self::compare_iso_year_month(
one.iso_year(),
one.iso_month(),
two.iso_year(),
two.iso_month(),
)
}
```There was a problem hiding this comment.
Yes. Is there a reason you want separate compare vs compare_iso_year_month methods? It doesn't seem like the spec needs them, just compare() is fine
There was a problem hiding this comment.
No, not really i just initally had issues using just compare(), but i think i can make it work with just one.
There was a problem hiding this comment.
For reference why compare_iso vs. compare.
There was a problem hiding this comment.
I don't mind the name, it's specifically that we have two functions. We should just have one.
There was a problem hiding this comment.
Made them into one function now, also used core insted of std
…y, PlainYearMonth, and PlainTime to return core::cmp::Ordering and simplify logic
|
This just needs a rebase / merge to resolve the conflicts, and then it should be good to go. |
Solves #266 Is there a way to add tests for these new method? We belive we found a solution for the issue, but we havent been able to test it. We would like some feedback. Worked with @HenrikTennebekk --------- Co-authored-by: Henrik Tennebekk <henrik.ten@gmail.com>
5 participants
Solves #266
Is there a way to add tests for these new method? We belive we found a solution for the issue, but we havent been able to test it. We would like some feedback.
Worked with @HenrikTennebekk