Support http-client == 2.2 #213

bgamari · 2016-11-04T15:34:29Z

This adds support for the http-client-2.2 series, at long last resolving #206. In addition it performs a number of clean-ups throughout the code-base.

This is potentially needed for error handling as HttpException now includes it.

These include, * Redundant imports * Defaulting warnings * Duplicate exports

This allows us to easily silence a number of redundant import warnings and drop some CPP.

parseUrl is deprecated.

aristidb

My comments are mostly questions for things I don't understand

aristidb · 2016-11-07T19:50:05Z

Aws/Aws.hs

@@ -52,6 +52,7 @@ import qualified Data.Text.Encoding           as T
 import qualified Data.Text.IO                 as T
 import qualified Network.HTTP.Conduit         as HTTP
 import           System.IO                    (stderr)
+import           Prelude


Why is this required? I thought Prelude is always implicitly imported?

The import of Control.Applicative is unnecessary with newer base versions, so moving the Prelude import silences the resulting warning in a backwards-compatible way. Edward Kmett introduced me to this trick.

aristidb · 2016-11-07T19:52:01Z

Aws/Aws.hs

-simpleAws cfg scfg request
-  = liftIO $ HTTP.withManager $ \manager ->
-      loadToMemory =<< readResponseIO =<< aws cfg scfg manager request
+simpleAws cfg scfg request = liftIO $ runResourceT $ do


I did quite like the convenience of withManager. Shame that it's gone.

aristidb · 2016-11-07T19:53:04Z

Aws/Core.hs

@@ -111,8 +112,10 @@ import qualified Data.ByteString.UTF8     as BU
 import           Data.Char
 import           Data.Conduit             (($$+-))
 import qualified Data.Conduit             as C
+#if MIN_VERSION_http_conduit(2,2,0)


So, do we still support http-conduit < 2.2 after all these changes?

aristidb · 2016-11-07T19:54:01Z

Aws/Core.hs

@@ -130,14 +133,13 @@ import qualified Network.HTTP.Types       as HTTP
 import           System.Directory
 import           System.Environment
 import           System.FilePath          ((</>))
-#if MIN_VERSION_time(1,5,0)
-import           Data.Time.Format


Was Data.Time.Format an unused import?

aristidb · 2016-11-07T19:56:30Z

Aws/Core.hs

-loadCredentialsFromInstanceMetadata = liftIO $ HTTP.withManager $ \mgr ->
-  do
+loadCredentialsFromInstanceMetadata = do
+    mgr <- liftIO $ HTTP.newManager HTTP.tlsManagerSettings


Is this manager cleaned up after the call to loadCredentialsFromInstanceMetadata? I may be confused about how http-client / http-conduit work nowadays, so please forgive me if this is a stupid question.

Yes. To quote the documentation of newManager,

Create a Manager. The Manager will be shut down automatically via garbage collection.

aristidb · 2016-11-07T20:01:40Z

Aws/Core.hs

+                         -> ResourceT IO a
+#if MIN_VERSION_http_conduit(2,2,0)
+throwStatusCodeException req resp = do
+    let resp' = fmap (const ()) resp


Does this take the response and strip out the body so it's safe to pass around?

It makes the types line up, yes.

aristidb · 2016-11-07T20:02:31Z

Aws/DynamoDb/Core.hs

@@ -399,7 +399,7 @@ instance DynVal UTCTime where

 -------------------------------------------------------------------------------
 pico :: Rational
-pico = toRational $ 10 ^ (12 :: Integer)
+pico = toRational $ (10 :: Integer) ^ (12 :: Integer)


Right, that was generating a lot of warnings! :D

Indeed; ^ is terrible in this respect.

aristidb · 2016-11-07T20:02:58Z

Aws/DynamoDb/Core.hs

@@ -1338,7 +1339,7 @@ getAttr k m = do
 -- | Parse attribute if it's present in the 'Item'. Fail if attribute
 -- is present but conversion fails.
 getAttr'
-    :: forall a. (Typeable a, DynVal a)
+    :: forall a. (DynVal a)


Was Typeable never required, or newly not?

Never required.

aristidb · 2016-11-07T20:06:12Z

aws.cabal

@@ -139,7 +139,7 @@ Library
                       scientific           >= 0.3,
                       tagged               >= 0.7     && < 0.9,
                       text                 >= 0.11,
-                       time                 >= 1.1.4   && < 2.0,
+                       time                 >= 1.4.0   && < 1.7,


Which ghc version has time 1.4? I wonder if we might go further and make this time 1.5+ only. Also, why the tightening of the upper bound?

GHC 7.4.1 through 7.8.4 shipped with 1.4.*. See https://ghc.haskell.org/trac/ghc/wiki/Commentary/Libraries/VersionHistory

bgamari · 2016-11-07T22:41:32Z

Thanks for the review!

ilovezfs · 2016-11-14T05:59:18Z

@bgamari thanks for doing this!

mrkkrp · 2016-12-14T13:54:30Z

Can we release a new version of aws with this change?

aristidb · 2016-12-21T10:35:59Z

@bgamari also has release privileges, but I have some time today to try to make a release.

ilovezfs · 2016-12-21T10:40:08Z

Yes pretty please with sugar on top!

aristidb · 2016-12-21T14:23:21Z

@mrkkrp @ilovezfs 0.15 is released.

bgamari added 5 commits November 4, 2016 10:38

Pass http-client Request to responseConsumer

7209180

This is potentially needed for error handling as HttpException now includes it.

Silence a number of trivial warnings

379b1d1

These include, * Redundant imports * Defaulting warnings * Duplicate exports

Use HTTP.newManager instead of deprecated HTTP.withManager

64fa3fd

Aws.DynamicDb.Core: Remove redundant Typeable constraint

6896a43

Drop support for time < 1.4

73552ea

This allows us to easily silence a number of redundant import warnings and drop some CPP.

bgamari force-pushed the http-client-2.2 branch from 773d836 to bb17322 Compare November 4, 2016 15:35

bgamari added 3 commits November 4, 2016 11:45

Add support for http-client-2.2

b09069d

Ec2.InstanceMetadata: Use parseUrlThrow explicitly

d5b10b7

parseUrl is deprecated.

Drop redundant Monoid (ResponseMetadata a) constraints

f18b362

bgamari force-pushed the http-client-2.2 branch from bb17322 to f18b362 Compare November 4, 2016 15:45

aristidb reviewed Nov 7, 2016

View reviewed changes

aristidb merged commit 3a04e95 into aristidb:master Nov 8, 2016

aristidb mentioned this pull request Nov 13, 2016

Doesn't compile with latest http-conduit 2.2.0 #206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support http-client == 2.2 #213

Support http-client == 2.2 #213

bgamari commented Nov 4, 2016

aristidb left a comment

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016

aristidb Nov 7, 2016

bgamari Nov 7, 2016 •

edited

Loading

bgamari commented Nov 7, 2016

ilovezfs commented Nov 14, 2016

mrkkrp commented Dec 14, 2016

aristidb commented Dec 21, 2016

ilovezfs commented Dec 21, 2016

aristidb commented Dec 21, 2016

Support http-client == 2.2 #213

Support http-client == 2.2 #213

Conversation

bgamari commented Nov 4, 2016

aristidb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgamari Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

bgamari commented Nov 7, 2016

ilovezfs commented Nov 14, 2016

mrkkrp commented Dec 14, 2016

aristidb commented Dec 21, 2016

ilovezfs commented Dec 21, 2016

aristidb commented Dec 21, 2016

bgamari Nov 7, 2016 •

edited

Loading