-
Notifications
You must be signed in to change notification settings - Fork 3.4k
duration should implement to_string/1 #14516
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
Comments
FYI this has been proposed and we decided not to implement it, you can find the conversation here. |
Thanks @sabiwara. Things have changed since that issue, as we know have a
So we need to decide if we want to raise and force folks to pick their units or have something that works out of the box (and then perhaps they are surprised once they see the default units). Although one could argue that |
I'm a bit behind the conversation so apologies if my question is naive, but was there an issue using ISO8601 time unit abbreviations as the default unit designator? It feels to me like the default should be round-trippable (parse-to_string-parse) but perhaps that too is overly simplistic? |
@kipcole9 |
Personally leaning towards the camp of "raise and force folks to pick their units", since it's not so common anyway, and it seems reasonable to have people converting to a string the way they see fit? (ISO, human with ISO unit, human with human unit...) |
Got it, and understood. You're right about the readability unless you spent too much time reading iso8601 (guilty!) |
@kipcole9 Ha, I didn’t mean to directly imply you but I think I could, to a certain extent, say the same about myself on dates and the ISO format. Then the question is: do we include a default rendering or not? |
I think defaults would be helpful, but not essential, personally. I wonder if using the symbols from |
Do you mean using the same symbols as in "%Y-%m-%d %H:%M:%S"? |
Yessir, that's what jumped into my mind (not including the %) |
We would need to come up with an entry for weeks and milliseconds is not the best (%f), I also think people would struggle to know when you have minutes or months if there is nothing between years and seconds. :( |
All true. So in which case, no defaults seems like the pragmatic solution. |
I've caused some debate. Thinking about this some more perhaps there should be a sigil for Duration and to_string would simply return that consistently with Date, Time, DateTime, NaiveDateTime. I propose ~P (for period): '~P[PT1H]' where we are using ISO8601 with the P on the sigil and in the 'payload' although perhaps redundant and could be just '~P[T1H]' Maybe not everyone loves ISO8601 duration but would be nice IMHO to have to_string, sigils, parse roundtrip on duration out of the box consistent with other temporal modules. |
I've updated the PR #14517 with sigil ~P |
@matt-beanland we also discussed |
Uh oh!
There was an error while loading. Please reload this page.
Elixir and Erlang/OTP versions 1.18.3 / Erlang/OTP 27
Given other Calendar types implement String.Chars, Duration should too.
This is very trivial, as would just call Duration.to_string/2 with no opts.
`iex(2)> to_string(Duration.new!(hour: 1))
** (Protocol.UndefinedError) protocol String.Chars not implemented for type Duration (a struct)
Got value:
iex(2)> Duration.to_string(Duration.new!(hour: 1))
"1h"
iex(3)> `
Operating system
MacOS
Current behavior
`iex(2)> to_string(Duration.new!(hour: 1))
** (Protocol.UndefinedError) protocol String.Chars not implemented for type Duration (a struct)
Got value:
`
Expected behavior
iex(2)> to_string(Duration.new!(hour: 1)) "1h"
The text was updated successfully, but these errors were encountered: