Add support for :hour and :minute time units #10185
Conversation
lib/elixir/lib/calendar/time.ex
Outdated
| are measured according to `Calendar.ISO`. | ||
| If the first time value is earlier than the second, a negative number is | ||
| returned. The answer can be returned in any `unit` available from | ||
| `t:time_unit/0`. |
There was a problem hiding this comment.
These two sections seemed contradictory so I removed the second segment. Looking at it now I realize that there isn't any mention of Calendar.ISO units, so I need to put that back.
This enhances time unit aware functions in the `Time` module so that they can operate on `:hour` and `:minute`, along with standard `System.time_unit/0` values. This eliminates the need to convert all values to seconds, which allows more expressive time manipulation. Along with changes to `Time.add/3` and `Time.diff/3`, this adds a new extended `time_unit` type to `Time`.
sorentwo
force-pushed
the
feature/minute-hour-time-units
branch
from
817013c
to
d84c281
Compare
|
This is the first of four changes (Time, Date, DateTime, NaiveDateTime). I opened the PR to get feedback on the approach before proceeding on the next few changes. The changes are atomic and stand alone so I opted for separate PRs, but I'm happy to combine them instead if desired. |
lib/elixir/lib/calendar/time.ex
Outdated
| end | ||
|
|
||
| def diff(%{calendar: Calendar.ISO} = time1, %{calendar: Calendar.ISO} = time2, :minute) do | ||
| (time1.hour - time2.hour) * 60 + (time1.minute - time2.minute) |
There was a problem hiding this comment.
This will return the wrong result for this:
Time.diff(~T[00:01:20], ~T[00:00:40])
It should return zero. I am afraid you will have to add all seconds and microseconds together, as in the clause below, and only then extracts minutes and hours.
There was a problem hiding this comment.
Excellent point. It felt like something was missing here.
There was a problem hiding this comment.
I've modified it to use the base diff implementation and div/2 the result 👍
Co-authored-by: José Valim <[email protected]>
This enhances time unit aware functions in the `Time` module so that they can operate on `:hour` and `:minute`, along with standard `System.time_unit/0` values. This eliminates the need to convert all values to seconds, which allows more expressive time manipulation. Along with changes to `Time.add/3` and `Time.diff/3`, this adds a new extended `time_unit` type to `Time`.
|
Thanks @sorentwo! 💜 |
This reverts commit 35f6ff8.
|
It looks this PR got reverted, @josevalim Sorry to interrupt, why it got reverted? |
|
see this comment and discussion around it for the reason: #10199 (comment) |
This enhances time unit aware functions in the
Timemodule so that they can operate on:hourand:minute, along with standardSystem.time_unit/0values. This eliminates the need to convert all values to seconds, which allows more expressive time manipulation.Along with changes to
Time.add/3andTime.diff/3, this adds a new extendedtime_unittype toTime.