Skip to content

Fix aeson under 0.7.0 - #70

Merged
snoyberg merged 3 commits into
snoyberg:masterfrom
xkollar:fix-aeson-under-0.7.0
Sep 6, 2015
Merged

Fix aeson under 0.7.0#70
snoyberg merged 3 commits into
snoyberg:masterfrom
xkollar:fix-aeson-under-0.7.0

Conversation

@xkollar

@xkollar xkollar commented Sep 5, 2015

Copy link
Copy Markdown
Contributor

Fixing issue #69 ... plus some polish.

@xkollar

xkollar commented Sep 5, 2015

Copy link
Copy Markdown
Contributor Author

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

@snoyberg

snoyberg commented Sep 6, 2015

Copy link
Copy Markdown
Owner

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

xkollar commented Sep 6, 2015

Copy link
Copy Markdown
Contributor Author

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.)

Comment thread Data/Yaml/Builder.hs

Copy link
Copy Markdown
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

xkollar commented Sep 6, 2015

Copy link
Copy Markdown
Contributor Author

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

snoyberg commented Sep 6, 2015

Copy link
Copy Markdown
Owner

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

xkollar commented Sep 6, 2015

Copy link
Copy Markdown
Contributor Author

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

@snoyberg

snoyberg commented Sep 6, 2015

Copy link
Copy Markdown
Owner

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

xkollar commented Sep 6, 2015

Copy link
Copy Markdown
Contributor Author

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