-
Notifications
You must be signed in to change notification settings - Fork 90
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
Use a custom string_to_float/1 function instead of plain String.to_float for ensure_numeric #221
Conversation
The latter doesn't work when the textual representation is an integer, Float.parse/1 however works with it but might have its own side effects that haven't been tested for (likely support for things like underscores and other elixir-esque formatting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Mind adding a test along the same lines as the existing "Decode seamlessly converts coordinates that are numbers-as-strings" that tests the integer case?
lib/geo/utils.ex
Outdated
end | ||
end | ||
|
||
@spec string_to_float(String.t()) :: float() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spec string_to_float(String.t()) :: float() | |
@spec string_to_float!(String.t()) :: float() | no_return() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, TIL about no_return, I'll add the test shortly
c26aa45
to
dca8223
Compare
And a decode test to ensure it works as intended with integers EDIT: You know I should read properly next time EDIT.2: Made the formatter happy
dca8223
to
2befd05
Compare
Okay everything should be in order now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks so much for your contribution. I'll get it released as v4.0.1.
String.to_float will not handle integer representations, instead Float.parse can be used, I almost wanted to roll something myself until I remembered Float.parse exists.
Related
#220