search

massivefermion / birl

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

Type Time and NaiveTime differently #30

Open jrstrunk opened 1 month ago

jrstrunk commented 1 month ago

Hey everyone, just wanted to start a discussion. Since Gleam has a really nice type system and as a language heavily encourages you to use types to reduce the possibility of bugs or crashes, I thought it would really help reduce time related bugs if Time and NaiveTime were different types. I know a big downside is that it would add a lot more code to this package and probably bump birl up a major version, but it will really make sure that when people do comparisons and conversions between times, they know how time offsets will be handled. For example, if I wanted to compare two time values, say 2019-01-02T14:00:00.000 and 2019-01-02T14:00:00.000-04:00, it is not obvious to me how the compare function will behave in this case. Maybe only the naive times are compared, maybe they are both converted to UTC and compared, I cannot know until I run some tests. If instead I was forced to convert them both to the same time type myself, then I could be sure how they are being compared: I could give the first an offset and compare it to the second with the compare function. Or I could make the second naive by calling to_naive (and rename the current func to to_naive_string) and compare them with a compare_naive function. This gives me, the application implementer, explicit control over how I deal with time offsets and zones, instead of assuming the birl package will handle them the way I want every time.

jrstrunk commented 1 month ago

For another example, without running tests or knowing the implementation, can anyone guess what this will return:

  let assert Ok(t) = birl.parse("2019-01-01T14:00:00.003")
  birl.get_offset(t)
  |> io.debug

The result is "-01:00". I wouldn't have guessed that myself, but maybe we shouldn't be able to pass a NaiveTime into a function like get_offset. Making them two types would prevent this. Love the package, just want it to be the best it can be!

massivefermion commented 1 month ago

As has been mentioned in #29, this is a bug in parse, it should return an error for that input. But your suggestion about having a separate type for naive values makes sense, I think Eilxir's standard library has that. I'll definitely consider this for version 2. Thanks