pubnub / haskell

PubNub Haskell SDK
Other
19 stars 16 forks source link

Ghc 7.10.1 compatibility #6

Closed mhitza closed 9 years ago

mhitza commented 9 years ago

In GHC 7.10.1 mapM has been generalized to:

mapM :: (Monad m, Traversable t) => (a -> m b) -> t a -> m (t b)

Since I didn't know the type of resp, in my second commit I've forced a list concatenation to help the typechecker. Obviously that commit must be changed with the appropriate type signature, and not using that hack.

tsloughter commented 9 years ago

Hey @mhitza any chance we can figure out the fix for getting the appropriate type signature so we can merge this? I took a look but wasn't sure how to have an element in a record (resp in SubscribeResponse in this case) be of the Traversable class... Since you can't set a type class for a record element.

mhitza commented 9 years ago

Before the BBP (burning bridges proposal) hit GHC I know that the type of mapM was the following

mapM :: Monad m => (a -> m b) -> [a] -> m [b]

Forcing list concatenation helped GHC infer the Traversable [] instance in that case. However the definition of SubscribeResponse is:

data SubscribeResponse a = SubscribeResponse (a, Timestamp)
                         deriving (Show, Generic)

Polymorphic in a without any other hint at the fact that it is a Traversable/list. I'm not that familiar with the codebase, and aeson (since we're dealing with FromJSON instances as well); but there are two workarounds I can think of in this case (excluding the hack I did for the PR).

  1. Wrap a inside a list in the datatype definition
data SubscribeResponse a = SubscribeResponse ([a], Timestamp)
                         deriving (Show, Generic)
  1. Write a monomorphic version of mapM (compatible with the one used in GHC < 7.10.1) inside PubNub.hs file and use that instead of the one imported currently from Control.Monad

The downside of both these options is that idk if the semantics of the function are preserved. I would think so, given that I know the definition of mapM before 7.10.1, but there are no tests to validate the changes.

tsloughter commented 9 years ago

Forgot about this. I went with the [a] fix. Thanks!