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

Implement Duration.to_iso8601/1 and Duration.from_iso8601/1 #13599

Closed

Conversation

tanguilp
Copy link
Contributor

No description provided.

@josevalim
Copy link
Member

We discussed this in the past but, opposite to other ISO formats, this one is quite rare in practice and we would need to make some extensions. Do you have a use case for it?

@Wigny
Copy link

Wigny commented May 25, 2024

I think a great use case is parsing ISO durations in a custom Absinthe scalar, transforming a Duration back and forth to a string, so it can be used on a GraphQl field and read by front-end clients...

iex> Duration.new!(second: 30) |> Duration.to_iso8601()
"PT30S"
iex> Duration.new!(hour: 2, microsecond: {1000, 6}) |> Duration.to_iso8601()
"PT2H0.001S"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need tests for negative durations.

"#{year}#{month}#{week}#{day}"
end

defp to_iso8601_right_part(%Duration{hour: 0, minute: 0, second: 0, microsecond: {0, 0}}) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should generally match on {0, _} for microseconds.

@josevalim
Copy link
Member

Sounds good. I have reopened a PR for from_iso8501: #13473 so we can tackle these problems individually.

@tanguilp
Copy link
Contributor Author

Do you have a use case for it?

Had it on a REST API not long ago for flight duration.

My bad, I didn't intend to publish this draft on this repo 🙈 Closing it for now, I'll follow the work on the reopened PR and participate if useful. #13473 implements from_iso8601/1, not sure if we should implement to_iso8601 in the same PR or in a separated one 🤔

@tanguilp tanguilp closed this May 25, 2024
@josevalim
Copy link
Member

@tanguilp it is ok to have this PR introducing to_iso8601 and the other introduces from_iso8601. :)

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

Successfully merging this pull request may close these issues.

3 participants