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

Check date value before calling to_iso8601 #7769

Merged

Conversation

garte
Copy link
Contributor

@garte garte commented Mar 6, 2018

When deserializing a date value the value has to be a string when calling to_iso8601. Otherwise it fails with a match error due to a is_binary() guard.

When deserializing a date value the value has to be a string when
calling to_iso8601. Otherwise it fails with a match error due to a
is_binary() guard.
@wing328
Copy link
Contributor

wing328 commented Mar 9, 2018

@garte thanks for the PR.

cc @niku @chingor13 for review.

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

We also ran into this in elixir-google-api

value = Map.get(model, field)
case is_binary(value) do
true -> case DateTime.from_iso8601(value) do
{:ok, datetime} ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Another bug is that DateTime.from_iso8601 returns a tuple of size 3.

So it should actually be {:ok, datetime, _offset}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest commit should fix this, would that be ok?

@wing328 wing328 merged commit f9b2839 into swagger-api:master Mar 19, 2018
@wing328
Copy link
Contributor

wing328 commented Mar 19, 2018

@garte thanks for the PR, which has been merged into master.

@chingor13 thanks for reviewing the change

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

Successfully merging this pull request may close these issues.

3 participants