massivefermion / birl

datetime handling for gleam
https://hex.pm/packages/birl
Apache License 2.0
72 stars 9 forks source link

Comparing durations #14

Closed ffigiel closed 9 months ago

ffigiel commented 9 months ago

Hello!

TL;DR is it ok to unwrap the Int value from Duration? If not, should there be a duration.compare function?


Given a package release date, I need to check if it was released within the last hour. I didn't find a function for comparing durations in the duration module, so I'm wondering if there is a preferred way of doing that?

First, I chose to unwrap the underlying integer values and compare them, but I thought I shouldn't be depending on the implementation details of the Duration type.

fn format_date(datetime: Time) -> String {
  let now = birl.now()
  let Duration(package_age_ms) = birl.difference(now, datetime)
  let Duration(age_threshold_ms) = duration.hours(1)
  case package_age_ms < age_threshold_ms {
    True -> "Just now!"
    False -> birl.legible_difference(now, datetime)
  }
}

Then I noticed there is a blur_to function, which lets me write less code, but its behavior is somewhat uncommon

if the duration is not an integer multiple of the unit, the remainder will be disgarded if it’s less than two thirds of the unit

fn format_date(datetime: Time) -> String {
  let now = birl.now()
  let package_age = birl.difference(now, datetime)
  case duration.blur_to(package_age, duration.Hour) < 1 {
    True -> "Just now!"
    False -> birl.legible_difference(now, datetime)
  }
}

Do you think adding a new function such as fn compare(a: Duration, b: Duration) -> Bool is warranted?


Just FYI: While writing this and collecting my thoughts, I noticed the birl.compare function and found a third solution to this

fn format_date(datetime: Time) -> String {
  let now = birl.now()
  let one_hour_ago = birl.subtract(now, duration.hours(1))
  case birl.compare(datetime, one_hour_ago) {
    order.Gt -> "Just now!"
    _ -> birl.legible_difference(now, datetime)
  }
}
massivefermion commented 9 months ago

Thanks for the suggestion. I agree, compare(Duration, Duration) -> Order could be helpful.

Then I noticed there is a blur_to function, which lets me write less code, but its behavior is somewhat uncommon

Well, that's kind of because of legible_difference. It uses blur under the hood, so I wrote blur in a way that is useful for legible_difference and part of that is making it behave in a way that is natural to how we think about time. It feels to me like we wouldn't say e.g. 8 days ago is two weeks ago! So when I wrote blur_to, I just did the same for approximations as for blur. How do you think it should behave?

ffigiel commented 9 months ago

I could add the duration.compare function then. On that note, would you also want to change Duration to an opaque type?

The blur functions do make sense for the legible_difference use case and I wouldn't change anything about them. I mentioned them because they seemed like a potential solution before I discovered there is also birl.compare 👍

massivefermion commented 9 months ago

Thanks for the offer, but I've already written duration.compare, along with some other minor changes(the new version is out). But I don't think making Duration an opaque type is a good idea. I only make a type opaque if there is a possibility that the developer can create an invalid/unacceptable(for whatever reason) instance of it, so it's better to be able to create new instances only using predefined functions. But there is no such possibility for Duration. Any integer is acceptable for a Duration so it seems to me making it opaque is an unnecessary restriction. There is no reason that developers shouldn't be able to destruct a Duration or create a new instance of it directly.

ffigiel commented 9 months ago

Thanks for the explanation. In addition I don't image that the implementation of the Duration type might change in the future, so opaque type is not necessary indeed.

Just tried out the v1.5 release, works great. Thank you 🙂