Temporal duration compare #186
Conversation
src/builtins/core/duration/date.rs
Outdated
|
|
||
| /// DateDurationDays | ||
| /// TODO(review): Question: what the correct return type? | ||
| pub fn days(&self, relative_to: &PlainDate) -> TemporalResult<i64> { |
There was a problem hiding this comment.
note: What the return type should be here is a good question. Based off the specification, DateDurationDays is an abstract op that is only called in Temporal.Duration.compare. Given that context, my immediate reaction would be that this method should be pub(crate) (unless there is an argument for an abstract operation being public immediatley) and return TemporalResult<i64>.
This is slightly related to a long running question, I've had about whether temporal_rs would be able to move to a Duration using integers vs. floating points. But I haven't had the time to sit down and audit the spec to confirm.
| @@ -572,6 +631,7 @@ impl Duration { | |||
| self.weeks(), | |||
| self.days().checked_add(&FiniteF64::from(balanced_days))?, | |||
| )?; | |||
| // TODO: Should this be using AdjustDateDurationRecord? | |||
There was a problem hiding this comment.
Good catch! This implementation is from before some of the more recent specification changes. There's a good argument to be made that round should be updated to take advantage of the methods implemented in this PR.
That may be best scoped to another PR as that may include a a couple more changes in the code then specifically in round. I'll leave that up to you.
| Some(ArithmeticOverflow::Constrain), | ||
| )?; | ||
| // 4. Let epochDays1 be ISODateToEpochDays(plainRelativeTo.[[ISODate]].[[Year]], plainRelativeTo.[[ISODate]].[[Month]] - 1, plainRelativeTo.[[ISODate]].[[Day]]). | ||
| let epoch_days_1 = iso_date_to_epoch_days( |
There was a problem hiding this comment.
issue: iso_date_to_epoch_days deviates from the specification here and takes an exact month vs. month - 1
This is sort of a mea culpa, I should have documented this change better (and thought that I had!). We don't have to subtract 1 from month here because the neri-schneider calculations take the month exactly vs month - 1. For reference, here are the tests.
| @@ -66,6 +66,7 @@ impl NormalizedTimeDuration { | |||
|
|
|||
| // NOTE: `days: f64` should be an integer -> `i64`. | |||
| /// Equivalent: 7.5.23 Add24HourDaysToNormalizedTimeDuration ( d, days ) | |||
| /// Add24HourDaysToTimeDuration?? | |||
There was a problem hiding this comment.
note: looks like the abstract op name changed in the docs. This can be updated.
| /// | ||
| /// `7.2.3 Temporal.Duration.compare ( one, two [ , options ] )` | ||
| #[must_use] | ||
| pub fn compare( |
There was a problem hiding this comment.
question: Should compare take &self for the first Duration parameter or mimic the specification and have it be a standalone function on Duration?
This is more a question of what is the best way to translate compare in Rust vs. JavaScript. In the literal sense to the specification, this is implemented exactly as it should be. But in Rust where PartialOrd exists as a trait, it's common to have one.cmp(&two) conceptually. So is it best for this to be a method similar to PartialOrd, e.g. one.compare(&two, None)??
I've tended to prefer this approach, mostly, because I think it's nice to adhere to similarly structured syntax. But I could be convinced to this current implementation if others think it's bad to overload the concept of one.cmp(&two). The most important thing to be consistent across similar methods and functions in temporal_rs for the builtins.
src/builtins/core/duration.rs
Outdated
| @@ -77,7 +77,7 @@ impl PartialDuration { | |||
| /// `Duration` is made up of a `DateDuration` and `TimeDuration` as primarily | |||
| /// defined by Abtract Operation 7.5.1-5. | |||
| #[non_exhaustive] | |||
| #[derive(Debug, Clone, Copy, Default)] | |||
| #[derive(Debug, Clone, Copy, Default, PartialEq, PartialOrd)] | |||
There was a problem hiding this comment.
issue: Duration should not implement PartialOrd and potentially not PartialEq.
The compare method supersedes the cmp impl from PartialOrd, implementing them both could lead to confusion. PartialEq is not necessarily needed by the specification. It doesn't define an equals method on Duration, but PartialEq is incredibly useful for testing purposes.
| // e. If after1 > after2, return 1𝔽. | ||
| // f. If after1 < after2, return -1𝔽. | ||
| // g. Return +0𝔽. | ||
| return Ok(after1.cmp(&after2)); |
There was a problem hiding this comment.
note: this will probably need to be updated on a rebase / merge to main due to #175.
There was a problem hiding this comment.
For reference on the above comment:
src/builtins/core/duration/date.rs
Outdated
| } | ||
|
|
||
| /// AdjustDateDurationRecord | ||
| pub fn adjust( |
There was a problem hiding this comment.
suggestion: pub(crate)
Since this is an abstract op, it would be best to keep these pub(crate) until otherwise needed.
Fixes #154
Wrote this along with @lockels, @Neelzee, @sebastianjacmatt, @Magnus-Fjeldstad, @HenrikTennebekk.
Please see question regarding the return type of DateDurationDays.