Skip to content
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

Type Time and NaiveTime differently #30

Open
jrstrunk opened this issue May 18, 2024 · 2 comments
Open

Type Time and NaiveTime differently #30

jrstrunk opened this issue May 18, 2024 · 2 comments

Comments

@jrstrunk
Copy link

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
Copy link
Author

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
Copy link
Owner

massivefermion commented May 23, 2024

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

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

No branches or pull requests

2 participants