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

fix result of String.toFloat "." (#336) #343

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@pisys
Contributor

pisys commented Aug 11, 2015

which is now a Err instead of Ok NaN.
Also added tests for String conversion.

fix result of String.toFloat "." (#336)
which is now a Err instead of Ok NaN.
Also added tests for String conversion.
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 11, 2015

Member

Thanks!

I wonder if this is how we should be implementing this in the first place. I don't really remember why I chose this route. Maybe it would be better to use a regex like [-+]?[0-9]*\.?[0-9]+([eE][-+]?[0-9]+)? or to just put parseFloat in a try block and trust that it'll do the right thing?

I also wonder why parseFloat('.') == NaN in JavaScript. Can you try to find out why that is true?

Member

evancz commented Aug 11, 2015

Thanks!

I wonder if this is how we should be implementing this in the first place. I don't really remember why I chose this route. Maybe it would be better to use a regex like [-+]?[0-9]*\.?[0-9]+([eE][-+]?[0-9]+)? or to just put parseFloat in a try block and trust that it'll do the right thing?

I also wonder why parseFloat('.') == NaN in JavaScript. Can you try to find out why that is true?

@pisys

This comment has been minimized.

Show comment
Hide comment
@pisys

pisys Aug 12, 2015

Contributor

parseFloat is less strict than the current implementation. It allows whitespaces, exponents, and only demands a prefix of the input string to be a float. Since "." is not a Float parseFloat returns NaN!

However, parseFloat can also parse "Infinity", which is a Number in JavaScript but not in Elm! So I discourage the mere wrapping of parseFloat until there is a explicit Infinity thing in Elm, such as x = Infinity (instead of x = 0/0).

Contributor

pisys commented Aug 12, 2015

parseFloat is less strict than the current implementation. It allows whitespaces, exponents, and only demands a prefix of the input string to be a float. Since "." is not a Float parseFloat returns NaN!

However, parseFloat can also parse "Infinity", which is a Number in JavaScript but not in Elm! So I discourage the mere wrapping of parseFloat until there is a explicit Infinity thing in Elm, such as x = Infinity (instead of x = 0/0).

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 19, 2016

Contributor

How is this pull request related to https://github.com/elm-lang/core/pull/342?

Contributor

jvoigtlaender commented Jan 19, 2016

How is this pull request related to https://github.com/elm-lang/core/pull/342?

This was referenced Jan 19, 2016

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 21, 2016

Contributor

I'm closing this for now, since it appears to be overridden by https://github.com/elm-lang/core/pull/342. Please re-open with comment if you disagree.

Contributor

jvoigtlaender commented Jan 21, 2016

I'm closing this for now, since it appears to be overridden by https://github.com/elm-lang/core/pull/342. Please re-open with comment if you disagree.

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