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

Parse `Scientific` directly, avoiding loss in precision. #68

Merged
merged 2 commits into from Sep 6, 2015

Conversation

@bitonic
Copy link
Collaborator

commented Sep 3, 2015

For example, before:

λ> Yaml.decode "x: 9.78159610558926e-5" :: Maybe Yaml.Value
Just (Object (fromList [("x",Number 9.781596105589261e-5)]))

after:

λ> Yaml.decode "x: 9.78159610558926e-5" :: Maybe Yaml.Value
Just (Object (fromList [("x",Number 9.78159610558926e-5)]))

@bitonic bitonic force-pushed the bitonic:fix-number-parsing branch 3 times, most recently from 5d12bcb to db243c7 Sep 3, 2015

yaml.cabal Outdated
@@ -63,6 +63,7 @@ library
other-modules:
Data.Yaml.Internal
ghc-options: -Wall
c-options: -w

This comment has been minimized.

Copy link
@snoyberg

snoyberg Sep 4, 2015

Owner

Is this change related?

This comment has been minimized.

Copy link
@bitonic

bitonic Sep 4, 2015

Author Collaborator

Nope, sorry about that.

@@ -172,6 +172,10 @@ textToValue _ _ t
where x `isLike` ref = x `elem` [ref, T.toUpper ref, titleCased]
where titleCased = toUpper (T.head ref) `T.cons` T.tail ref

#if MIN_VERSION_aeson(0, 7, 0)
textToScientific :: Text -> Either String Scientific
textToScientific = Atto.parseOnly (Atto.scientific <* Atto.endOfInput)

This comment has been minimized.

Copy link
@snoyberg

snoyberg Sep 4, 2015

Owner

Do we need a new lower bound on attoparsec for this?

This comment has been minimized.

Copy link
@bitonic

bitonic Sep 4, 2015

Author Collaborator

I thought aeson >= 0.7 would imply that we have an attoparsec with Atto.scientific, but this does not seem to be the case -- aeson-0.7.0.0 has attoparsec >= 0.11.1.0, which does not have Atto.scientific.

With attoparsec >= 0.11.1.0 && < 0.12 aeson used Atto.rational to parse Scientific numbers, which is not so good (see http://hackage.haskell.org/package/attoparsec-0.13.0.1/docs/Data-Attoparsec-Text.html#v:rational )

So I think we have to options:

  • Add attoparsec >= 0.12
  • More CPP to use rational if attoparsec >= 0.11.1.0 && < 0.12

I think the first option is the best, what do you think?

This comment has been minimized.

Copy link
@snoyberg

snoyberg Sep 4, 2015

Owner

I agree, option 1 it is.

Parse `Scientific` directly, avoiding loss in precision.
For example, before:

    λ> Yaml.decode "x: 9.78159610558926e-5" :: Maybe Yaml.Value
    Just (Object (fromList [("x",Number 9.781596105589261e-5)]))

after:

    λ> Yaml.decode "x: 9.78159610558926e-5" :: Maybe Yaml.Value
    Just (Object (fromList [("x",Number 9.78159610558926e-5)]))

@bitonic bitonic force-pushed the bitonic:fix-number-parsing branch from db243c7 to 621c89b Sep 4, 2015

Add lower bound for `attoparsec`...
...making sure that it includes `scientific`.
@bitonic

This comment has been minimized.

Copy link
Owner Author

commented on yaml.cabal in 056b465 Sep 4, 2015

In the end I realized this was the version where scientific was introduced.

In theory, we could add this constraint only if aeson >= 0.7.0.0. I don't even know if it's possible in cabal, and I don't think it's worth it anyway.

snoyberg added a commit that referenced this pull request Sep 6, 2015
Merge pull request #68 from bitonic/fix-number-parsing
Parse `Scientific` directly, avoiding loss in precision.

@snoyberg snoyberg merged commit eb8b0c7 into snoyberg:master Sep 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
snoyberg added a commit that referenced this pull request Sep 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.