Enhancement: negative integer support in DateTime.from_unix
#5128
Conversation
lib/elixir/lib/calendar.ex
Outdated
| Unix times are always in UTC and therefore the DateTime | ||
| will be returned in UTC. | ||
|
Thanks for your comments! I've added tests for |
lib/elixir/lib/calendar.ex
Outdated
| end | ||
|
|
||
| def to_unix(%DateTime{year: year}, _unit) do | ||
| raise ArgumentError, "Can only convert %DateTime{} to Unix time with a year >= 0, got: #{year}" |
There was a problem hiding this comment.
We don't capitalize the first letter of raised error messages in Elixir, so this would be "can only convert..." :)
Qqwy
force-pushed
the
enhancement/negative_datetime_from_unix
branch
from
2d1792a to
9c44a65
Compare
Qqwy
force-pushed
the
enhancement/negative_datetime_from_unix
branch
from
9c44a65 to
76fa458
Compare
lib/elixir/lib/calendar.ex
Outdated
| {:ok, %DateTime{year: year, month: month, day: day, | ||
| hour: hour, minute: minute, second: second, microsecond: {microsecond, precision}, | ||
| std_offset: 0, utc_offset: 0, zone_abbr: "UTC", time_zone: "Etc/UTC"}} | ||
| if total < -62167219200000000 do |
There was a problem hiding this comment.
Could we use an attribute here to avoid a "magic number" please
There was a problem hiding this comment.
We have @unix_epoch already which is 62167219200 and could be used.
There was a problem hiding this comment.
I completely agree. I was looking for a away to remove the magic number here, but I hadn't thought that the computed value of @unix_epoch would match it. Thank you, @fishcakez and @lau! :-)
…o microseconds) in documentation.
|
@lau: Could you look at this one more time, to see if it's |
-ready?
lib/elixir/lib/calendar.ex
Outdated
| end | ||
|
|
||
| def to_unix(%DateTime{year: year}, _unit) do | ||
| raise ArgumentError, "can only convert %DateTime{} to Unix time with a year >= 0, got: #{year}" |
There was a problem hiding this comment.
Better to not add this clause as this error can be misleading, for example, for a DateTime with non-ISO calendar.
It's acceptable to raise FunctionClauseError, we do that in similar cases.
There was a problem hiding this comment.
Raising an ArgumentError seems more logical to me, because it states 'the input is wrong', rather than 'there is a clause missing'.
But that probably is something for a different discussion. For now, as similar cases raise FunctionClauseError, I will remove this extra clause.
| year: 46302, zone_abbr: "UTC"}} | ||
| Negative Unix times are supported, up to -#{@unix_epoch} seconds, |
|
❤️ 💚 💙 💛 💜 |
Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
6 participants
Fixes #5124:
DateTime.from_unix/2.:errorwill be returned instead of{:ok, datetime}. The bang-version of the method will raise an ArgumentError in these cases.DateTime.to_unixhas similarly been altered to support all DateTimes withyear >= 0, which matches the new cutoff-point forDateTime.from_unix.DateTime.from_unix, a descriptive ArgumentError is raised.