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

Added extra condition to stop '-' or '+' returning a `Float` from `String.toInt`. #834

Merged
merged 1 commit into from Mar 26, 2017

Conversation

Projects
None yet
3 participants
@ChrisWellsWood
Contributor

ChrisWellsWood commented Feb 3, 2017

Current Issue

Currently String.toInt can return a Float, as the JavaScript function parseInt will return NaN (which is type Float in Elm) if you supply it with '+' or '-'. This will get around the type checker, so the compiler will think that the 'NaN' is an Int. See #831 for more details of the bug.

Solution

I've added a new condition so that an error will be raised if the input string is a single '-' or '+' character.

If you'd like to check the logic, I've mocked it up on repl.it.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Feb 3, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Feb 3, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@ChrisWellsWood ChrisWellsWood changed the title from Added extra condition to catch '-' or '+'. to Added extra condition to stop '-' or '+' returning NaN from `toInt`. Feb 3, 2017

@ChrisWellsWood ChrisWellsWood changed the title from Added extra condition to stop '-' or '+' returning NaN from `toInt`. to Added extra condition to stop '-' or '+' returning a `Float` from `String.toInt`. Feb 3, 2017

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 26, 2017

Member

Nice find! That boolean expression is getting out of control, but I'm not really sure how to simplify it in a nice way. Anyway, thanks for the fix!

Member

evancz commented Mar 26, 2017

Nice find! That boolean expression is getting out of control, but I'm not really sure how to simplify it in a nice way. Anyway, thanks for the fix!

@evancz evancz merged commit 9d1c13a into elm:master Mar 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment