Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add new version with support of aeson-2.0 and ghc-9 #69

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LupusAnay
Copy link

Hi!

I'm actively using this library, and we're planning to migrate our project to ghc9.
Since there is no open PR with similar changes, I decided to send mine.

I think there are still some questions needs to be resolved, for example updating of travis.ci configuration, and removal of Eq instance from Network.HTTP.Client.Response.
But overall, I think this version should work fine.

Any suggestions are welcome, and I'll try to fix them asap.

Copy link

@jonathanlking jonathanlking left a comment

Choose a reason for hiding this comment

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

Hey, I'm also actively using this library and wanting to do a similar upgrade — thanks for opening this PR! ✨

I've left a few comments about being able to support aeson-1 too, which is something I've done recently in other package upgrades.

My 2 cents would be to ditch Travis and migrate to GitHub actions instead.
It's really easy to set up and there is a great reference at https://kodimensional.dev/github-actions.

twilio.cabal Outdated
@@ -84,11 +84,11 @@ library
Twilio.UsageTrigger,
Twilio.UsageTriggers
hs-source-dirs: src
build-depends: aeson >=0.8 && <0.10 || >=0.11,
build-depends: aeson >=2.0,

Choose a reason for hiding this comment

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

Suggested change
build-depends: aeson >=2.0,
build-depends: aeson >= 0.11 && <1.6 || >=2.0 && <2.2,,

Choose a reason for hiding this comment

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

Also, text ==1.*, would be good to update to text ==1.* || ==2.0.*, to support the new 2.0 release.

@@ -21,7 +21,7 @@ import Control.Error.Safe
import Control.Monad
import Control.Monad.Catch
import Data.Aeson
import qualified Data.HashMap.Strict as HashMap
import qualified Data.Aeson.KeyMap as KeyMap

Choose a reason for hiding this comment

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

Suggested change
import qualified Data.Aeson.KeyMap as KeyMap
#if MIN_VERSION_aeson(2,0,0)
import qualified Data.Aeson.KeyMap as KeyMap
#else
import qualified Data.HashMap.Strict as KeyMap
#endif

If we're "ok" with enabling CPP, we can support both aeson 1 and 2 which might be helpful while people are still migrating over.

This would also require {-# LANGUAGE CPP #-} at the top of the file.

@@ -43,14 +42,14 @@ class FromJSON b => List a b | a -> b where
getList :: a -> [b]

-- | The plural name for the items in the 'List'.
getPlural :: Const Text (a, b)

Choose a reason for hiding this comment

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

Here I think you could do something like:

#if MIN_VERSION_aeson(2,0,0)
#else
type Key = Text
#endif

@LupusAnay
Copy link
Author

@jonathanlking thank you for your review! I applied your suggestions and tested the build (only one build) on a couple of stack resolvers, each with different versions of aeson and text. It seems to be compiling correctly.

Regarding Travis, I totally agree with you, but I'm not sure if the migration should be done in this pull request.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants