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

Use a custom string_to_float/1 function instead of plain String.to_float for ensure_numeric #221

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

IceDragon200
Copy link
Contributor

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

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)
Copy link
Contributor

@s3cur3 s3cur3 left a 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@spec string_to_float(String.t()) :: float()
@spec string_to_float!(String.t()) :: float() | no_return()

Copy link
Contributor Author

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

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
@IceDragon200
Copy link
Contributor Author

Okay everything should be in order now

Copy link
Contributor

@s3cur3 s3cur3 left a 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.

@s3cur3 s3cur3 merged commit f51e581 into felt:master Sep 23, 2024
21 checks passed
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.

2 participants