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

Fix aeson under 0.7.0 #70

Merged
merged 3 commits into from
Sep 6, 2015
Merged

Conversation

xkollar
Copy link
Contributor

@xkollar xkollar commented Sep 5, 2015

Fixing issue #69 ... plus some polish.

@xkollar
Copy link
Contributor Author

xkollar commented Sep 5, 2015

Btw: would you be interested in more "polish" changes? (HLint, ...)

@snoyberg
Copy link
Owner

snoyberg commented Sep 6, 2015

What's the use case for building with the older versions of aeson? Those versions have some pretty severe DoS security vulnerabilities, so I'd be more tempted to just set a lower bound.

Polish patches would be welcome, but please do include them in separate PRs to make the review easier.

@xkollar
Copy link
Contributor Author

xkollar commented Sep 6, 2015

Use case: building with haskell-platform-2014.2.0.1 (has ghc-7.8.4 and text-1.1.0.0) which will install aeson-0.6.2.1 when I run cabal sandbox init; cabal install yaml. On that setup I'm trying to use https://github.com/anchor/git-vogue, so no networking .

Bumping bounds in cabal file would work too, but then I would strongly suggest cleaning source from all the #if MIN_VERSION_aeson(0, 7, 0), as in #else parts have some issues...

Main fixing has been done in xkollar@53f54d5 ... others fix residual compiler warnings. If you wish, I can move them to separate PR but I thought they would not hurt in this case as they are in separate commits. (Of course, other polish things would not go into this PR.)

@@ -97,7 +103,7 @@ number :: Scientific -> YamlBuilder
number = scientific
#else
number :: Number -> YamlBuilder
number n rest = YamlBuilder (EventScalar (S8.pack $ show n) IntTag PlainNoTag Nothing :)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type error

@xkollar
Copy link
Contributor Author

xkollar commented Sep 6, 2015

PS: Even if you decide for bumping requirements for aeson, please accept this change first... it would feel better. (Then I can do setting of lower bound and removing #ifs.)

snoyberg added a commit that referenced this pull request Sep 6, 2015
@snoyberg snoyberg merged commit 24803de into snoyberg:master Sep 6, 2015
@snoyberg
Copy link
Owner

snoyberg commented Sep 6, 2015

Cool, merged. I'd strongly recommend against sticking with that old version of aeson. If you want to see why, try parsing {"DoS":1e100000000000000000000000000000000000000000000000000000}.

@xkollar
Copy link
Contributor Author

xkollar commented Sep 6, 2015

Thank you :-), really appreciated. Would you like me to set the lower bound for the aeson an do the related work?

@snoyberg
Copy link
Owner

snoyberg commented Sep 6, 2015

I'll make a point release with the changes you just made. A PR with cleanups and the tightened upper bound would be great, thanks.

snoyberg added a commit that referenced this pull request Sep 6, 2015
@xkollar
Copy link
Contributor Author

xkollar commented Sep 6, 2015

Ok, thanks once again for the very quick response.

@xkollar xkollar deleted the fix-aeson-under-0.7.0 branch September 6, 2015 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants