Skip to content

feat: add comparison function for Duration #14472

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.

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

Closed
wants to merge 1 commit into from

Conversation

yordis
Copy link
Contributor

@yordis yordis commented May 6, 2025

Context

I recently discovered that using <= did not work (unless I misunderstood something), so I tried to look for Duration.compare/2 to match other modules such as DateTime.compare/2

iex> remaining_duration = Duration.new!(second: 18)
...> threshold_duration = Duration.new!(second: 10, microsecond: {0, 6})
...> remaining_duration <= threshold_duration
true

@yordis yordis force-pushed the add-duration-compare-2 branch from 2348dce to 1f08865 Compare May 6, 2025 18:53
@kevinschweikert
Copy link
Contributor

kevinschweikert commented May 6, 2025

Wouldn't it be better to align with DateTime.compare/2 and return :lt | :eq | :gt?

Comment on lines 319 to 321
d1_timeout = Kernel.to_timeout(d1)
d2_timeout = Kernel.to_timeout(d2)
Copy link
Contributor

@v0idpwn v0idpwn May 6, 2025

Choose a reason for hiding this comment

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

This is not considering microseconds, probably should. Also has issues with months and years

@yordis yordis force-pushed the add-duration-compare-2 branch 2 times, most recently from 275a574 to e8cd5c7 Compare May 6, 2025 19:32
@yordis yordis force-pushed the add-duration-compare-2 branch from e8cd5c7 to 2aee507 Compare May 6, 2025 19:48
@yordis yordis requested a review from v0idpwn May 6, 2025 19:55
@hour_in_us @minute_in_us * 60 # 1 hour = 60 minutes
@day_in_us @hour_in_us * 24 # 1 day = 24 hours
@week_in_us @day_in_us * 7 # 1 week = 7 days
@month_in_us @day_in_us * 30 # 1 month = 30 days
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can compare durations without knowing the date context since months are not always 30 days long.

As an example:

duration_a = Duration.new(month: 1)
duration_b = Duration.new(day: 30)

Duration.compare(duration_a, duration_b) # => :eq
Date.shift(~D[2020-02-01], duration_a)  #=> ~D[2020-03-01]
Date.shift(~D[2020-02-01], duration_b)  #=> ~D[2020-03-02]

Copy link
Member

Choose a reason for hiding this comment

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

And for DateTime.shift it might even depend on the timezone if there's DST changes.

Copy link
Contributor Author

@yordis yordis May 6, 2025

Choose a reason for hiding this comment

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

@maennchen yeah I was wondering if year must be kept outside the comparing; the Kernel.to_timeout/1 already raise an exception based on month and year ... It would be the same with days 😭

Copy link
Member

@maennchen maennchen May 6, 2025

Choose a reason for hiding this comment

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

We could have something like this for convenience:

def compare(relative_to) do
  fn a, b ->
    # calc based on relative_to
  end
end

But essentially all you would be doing is:

[Naive]Date[Time].compare(
  [Naive]Date[Time].shift(relative_to, a),
  [Naive]Date[Time].shift(relative_to, b)
)

I'm wondering if the better course it to just add some docs pointing users to compare shifted dates instead of comparing durations directly and explain why.

@nwjlyons
Copy link
Contributor

nwjlyons commented May 6, 2025

How will this work with months? The number of days in a month depends on a relative date.

eg.

Duration.compare(Duration.new!(day: 30), Duration.new!(month: 1))

You could propose this in the mailing list for discussion https://github.com/elixir-lang/elixir?tab=readme-ov-file#proposing-new-features

@josevalim
Copy link
Member

You cannot compare durations without having a reference point. For example, this says that 12 months is the same as 360 days, but if I add these two durations to the same date, I will get different results, which therefore means they are not the same.

Depending on what you want to compare, it may be best to convert them to timeouts and then compare it.

@yordis yordis marked this pull request as draft May 6, 2025 20:01
@yordis
Copy link
Contributor Author

yordis commented May 6, 2025

As I was adding more test I realized why it wasn't included ...

@yordis yordis closed this May 6, 2025
@yordis yordis deleted the add-duration-compare-2 branch May 6, 2025 20:03
@axelson
Copy link
Contributor

axelson commented May 6, 2025

It would be nice to give some suggestions/guidance for how to compare durations in the module doc.

@yordis
Copy link
Contributor Author

yordis commented May 6, 2025

@axelson yeah ... the mistake made of comparing duration as I shown could be a classic situation to happens 😭

josevalim added a commit that referenced this pull request May 7, 2025
@josevalim
Copy link
Member

I have pushed some docs. FWIW, the type system should be getting better and better at catching those comparisons too and warning on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants