Temporal duration compare #186

sffc · 2025-01-27T14:39:25Z

Fixes #154

Wrote this along with @lockels, @Neelzee, @sebastianjacmatt, @Magnus-Fjeldstad, @HenrikTennebekk.

Please see question regarding the return type of DateDurationDays.

nekevss

Thanks for contributing! This is looking really great overall!

nekevss · 2025-01-27T15:38:35Z

src/builtins/core/duration/date.rs

+
+    /// DateDurationDays
+    /// TODO(review): Question: what the correct return type?
+    pub fn days(&self, relative_to: &PlainDate) -> TemporalResult<i64> {


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.

nekevss · 2025-01-27T15:45:37Z

src/builtins/core/duration.rs

@@ -572,6 +631,7 @@ impl Duration {
                    self.weeks(),
                    self.days().checked_add(&FiniteF64::from(balanced_days))?,
                )?;
+                // TODO: Should this be using AdjustDateDurationRecord?


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.

nekevss · 2025-01-27T15:51:57Z

src/builtins/core/duration/date.rs

+            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(


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.

nekevss · 2025-01-27T15:55:22Z

src/builtins/core/duration/normalized.rs

@@ -66,6 +66,7 @@ impl NormalizedTimeDuration {

    // NOTE: `days: f64` should be an integer -> `i64`.
    /// Equivalent: 7.5.23 Add24HourDaysToNormalizedTimeDuration ( d, days )
+    /// Add24HourDaysToTimeDuration??


note: looks like the abstract op name changed in the docs. This can be updated.

nekevss · 2025-01-27T16:05:07Z

src/builtins/core/duration.rs

+    /// 
+    /// `7.2.3 Temporal.Duration.compare ( one, two [ , options ] )`
+    #[must_use]
+    pub fn compare(


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.

nekevss · 2025-01-27T16:10:16Z

src/builtins/core/duration.rs

@@ -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)]


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.

nekevss · 2025-01-27T16:20:46Z

src/builtins/core/duration.rs

+                // e. If after1 > after2, return 1𝔽.
+                // f. If after1 < after2, return -1𝔽.
+                // g. Return +0𝔽.
+                return Ok(after1.cmp(&after2));


note: this will probably need to be updated on a rebase / merge to main due to #175.

For reference on the above comment:

https://github.com/boa-dev/temporal/blob/main/src/builtins/core/zoneddatetime.rs#L434

nekevss · 2025-01-27T16:41:07Z

src/builtins/core/duration/date.rs

+    }
+
+    /// AdjustDateDurationRecord
+    pub fn adjust(


suggestion: pub(crate)

Since this is an abstract op, it would be best to keep these pub(crate) until otherwise needed.

sffc · 2025-02-05T12:08:44Z

@lockels will drive this PR, coordinating with the other authors.

lockels · 2025-02-14T15:48:54Z

src/builtins/core/duration.rs

@@ -277,6 +277,73 @@ impl Duration {
    pub fn is_time_within_range(&self) -> bool {
        self.time.is_within_range()
    }
+
+    /// Compares self with other on a field by field basis.
+    fn cmp_raw(&self, other: &Self) -> Option<Ordering> {


Should this method be moved to a different section of the code that's private with pub(crate)? This is not part of the public API, but instead a helper method for the actual implementation of compare

Yes! Good question / catch! Preferably it should live in a different block.

Alright:) Which block exactly is the best place to put it? Private creation methods? Or maybe outside the any of the impl blocks? None of them make sense as a natural spot to place this function to me

Is this even needed? I would just inline the comparison on compare_with_provider, which makes for a simpler code:

if self.date == two.date && self.time == two.time { return Ok(Ordering::Equal); } // compare by relative

oh, that's true! thanks a lot, i'll remove the redundant function and just inline it instead

src/builtins/compiled/duration.rs

nekevss · 2025-02-14T15:51:29Z

src/builtins/core/duration.rs

@@ -277,6 +277,73 @@ impl Duration {
    pub fn is_time_within_range(&self) -> bool {
        self.time.is_within_range()
    }
+
+    /// Compares self with other on a field by field basis.
+    fn cmp_raw(&self, other: &Self) -> Option<Ordering> {


Yes! Good question / catch! Preferably it should live in a different block.

src/options.rs

…o "other"

nekevss

This looks fantastic! Thanks for working on this.

I do have on nit that's not necessarily blocking, and this PR needs to be updated to main, but then this should be good to merge 😄

nekevss · 2025-02-19T16:54:45Z

src/builtins/core/duration/date.rs

@@ -105,4 +108,59 @@ impl DateDuration {
    pub fn sign(&self) -> Sign {
        super::duration_sign(&self.fields())
    }
+
+    /// DateDurationDays
+    /// TODO(review): Question: what the correct return type?


nit: remove the TODO

I believe this was answered in the call that i64 should be sufficient.

Yes, i forgot to remove that! thanks reminding me!

sffc added 2 commits January 27, 2025 06:33

Initial implementation of Temporal.Duration.compare

6241577

Fix pre-existing docs comment

749504c

nekevss reviewed Jan 27, 2025

View reviewed changes

SebastianJM and others added 3 commits January 28, 2025 13:34

changed back to 1 indexed iso_date_to_epoch_days

e845af6

Remove PartialEq and PartialOrd from Duration

3ead5d1

Changed methods on DateDuration to pub (crate) and ran cargo fmt

fd6feba

lockels added 2 commits February 14, 2025 11:10

refactor to take in provider as argument

4a63a87

correct function parameters for Duration.compare in compiled

0909baa

lockels force-pushed the temporal-duration-compare branch from 1c80ef0 to 0909baa Compare February 14, 2025 13:27

lockels added 3 commits February 14, 2025 15:29

added tests in correct spot for compare

5656dfd

cargo fmt

ab3682d

modify code to pass lint test with clippy

e2fd8e5

lockels reviewed Feb 14, 2025

View reviewed changes

nekevss reviewed Feb 14, 2025

View reviewed changes

lockels added 2 commits February 14, 2025 17:45

inline redundant function "cmp_raw" and change parameter name "two" t…

f4db69d

…o "other"

block test on windows due to time zone disambiguation

a5c2047

nekevss approved these changes Feb 19, 2025

View reviewed changes

lockels and others added 2 commits February 20, 2025 14:29

Merge branch 'main' into temporal-duration-compare

139138c

remove redundant TODO review question about integer return types

a456d99

nekevss merged commit 1566796 into boa-dev:main Feb 20, 2025
7 checks passed

jedel1043 added C-enhancement New feature or request C-api Changes related to the public API labels Feb 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporal duration compare #186

Temporal duration compare #186

sffc commented Jan 27, 2025 •

edited

Loading

nekevss left a comment

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 27, 2025

nekevss Jan 28, 2025

nekevss Jan 27, 2025

sffc commented Feb 5, 2025

lockels Feb 14, 2025

nekevss Feb 14, 2025

lockels Feb 14, 2025

jedel1043 Feb 14, 2025 •

edited

Loading

lockels Feb 14, 2025

nekevss Feb 14, 2025

nekevss left a comment

nekevss Feb 19, 2025

lockels Feb 20, 2025

Temporal duration compare #186

Temporal duration compare #186

Conversation

sffc commented Jan 27, 2025 • edited Loading

nekevss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jan 27, 2025 •

edited

Loading

jedel1043 Feb 14, 2025 •

edited

Loading