Re: [Snowdrift-dev] [PATCH] Use appStripe as stripe client in handlers

[Bryan Richter]

On Tue, Jul 11, 2017 at 06:57:03PM -0700, Bryan Richter wrote:
> Much of this patch makes sense to me. I'm pretty sure I know where the
> type errors are originating from, too.

I looked at this yesterday, and I realized what really needs to happen
is we get rid of StripeI. It is completely superfluous; I never want
to test at that level.

We should get rid of StripeI, but still make the stripe runner (e.g.
Web.Stripe.stripe) an API parameter.

> 
> Throw rocks at me if I don't get to this in the next 24 hours or so...
> 
> On Tue, Jul 11, 2017 at 06:16:58AM +0300, fr33domlover wrote:
> > From: fr33domlover 
> > 
> > This is a new version of the patch, it uses runStripe in appStripe. It
> > avoids the StripeClient mess but still has a type error due to
> > StripeRunner being having the 'forall io' thing. The errors can be fixed
> > using type annotations probably, but using those in every single place
> > 'snowstripe' is used would be ridiculous.
> > 
> > Honestly idk much about Rank N types. I read about it and did some web
> > search. And I still don't understand how to properly fix this type error
> > thing. I think the error is simpler now but idk how to *elegantly* fix it.
> > ---
> >  crowdmatch/crowdmatch.cabal|  3 ++-
> >  crowdmatch/src/Crowdmatch.hs   |  7 +++
> >  website/src/AppDataTypes.hs|  8 
> >  website/src/Application.hs |  7 ++-
> >  website/src/Handler/PaymentInfo.hs | 11 ++-
> >  website/src/Handler/Util.hs|  9 +
> >  6 files changed, 22 insertions(+), 23 deletions(-)
> > 
> > diff --git a/crowdmatch/crowdmatch.cabal b/crowdmatch/crowdmatch.cabal
> > index d378766..2c602f1 100644
> > --- a/crowdmatch/crowdmatch.cabal
> > +++ b/crowdmatch/crowdmatch.cabal
> > @@ -28,7 +28,8 @@ library
> >  hs-source-dirs: src
> >  default-language: Haskell2010
> >  build-depends:
> > -base >=4.8 && <4.9
> > +  aeson
> > +, base >=4.8 && <4.9
> >  , bytestring >= 0.10.6.0
> >  , errors
> >  , lens
> > diff --git a/crowdmatch/src/Crowdmatch.hs b/crowdmatch/src/Crowdmatch.hs
> > index fe61387..a857c2a 100644
> > --- a/crowdmatch/src/Crowdmatch.hs
> > +++ b/crowdmatch/src/Crowdmatch.hs
> > @@ -58,10 +58,12 @@ import Control.Error (ExceptT(..), runExceptT, note)
> >  import Control.Lens ((^.), from, view, Iso', iso)
> >  import Control.Monad (void)
> >  import Control.Monad.IO.Class (MonadIO, liftIO)
> > +import Data.Aeson (FromJSON)
> >  import Data.Function (on)
> >  import Data.Int (Int32)
> >  import Data.Ratio
> >  import Data.Time (UTCTime, getCurrentTime, utctDay)
> > +import Data.Typeable (Typeable)
> >  import Database.Persist
> >  import Database.Persist.Sql (SqlPersistT)
> >  import System.IO
> > @@ -73,6 +75,7 @@ import Web.Stripe.Customer
> >  , createCustomer
> >  , deleteCustomer)
> >  import Web.Stripe.Error (StripeError)
> > +import Web.Stripe.StripeRequest (StripeRequest, StripeReturn)
> >  
> >  import Crowdmatch.Model hiding (Patron(..))
> >  import qualified Crowdmatch.Model as Model
> > @@ -87,6 +90,10 @@ import qualified Crowdmatch.Skeleton as Skeleton
> >  -- | A method that runs 'SqlPersistT' values in your environment.
> >  type SqlRunner io env = forall a. SqlPersistT io a -> env a
> >  
> > +type StripeClient = forall a.
> > +(Typeable (StripeReturn a), FromJSON (StripeReturn a)) =>
> > +StripeConfig -> StripeRequest a -> IO (Either StripeError 
> > (StripeReturn a))
> > +
> >  -- | A method that runs 'StripeI' instructions in IO. A default that uses
> >  -- 'stripe' is provided by 'runStripe'.
> >  type StripeRunner = forall io.
> > diff --git a/website/src/AppDataTypes.hs b/website/src/AppDataTypes.hs
> > index 39ea400..50d2526 100644
> > --- a/website/src/AppDataTypes.hs
> > +++ b/website/src/AppDataTypes.hs
> > @@ -16,6 +16,7 @@ import Yesod.Core.Types (Logger)
> >  import Yesod.GitRev
> >  
> >  import AuthSite
> > +import Crowdmatch
> >  import Settings
> >  
> >  -- | The God-object available to every Handler. This is the site's
> > @@ -30,10 +31,9 @@ data App = App
> >  , appAuth:: AuthSite
> >-- | The function for doing stripe API calls. Swapped out for a mock
> >-- thing in tests.
> > -, appStripe  :: forall a. (Typeable (StripeReturn a), FromJSON 
> > (StripeReturn a))
> > - => StripeConfig
> > - -> StripeRequest a
> > - -> IO (Either StripeError (StripeReturn a))
> > +, appStripe  :: StripeConfig
> > + -> forall a. StripeI a
> > + -> HandlerT App IO (Either StripeError a)
> >  }
> >  
> >  -- This function generates the route types, and also generates the
> > diff --git a/website/src/Application.hs b/website/src/Application.hs
> > index 761d3f8..b6b20db 100644
> > --- a/website/src/Application.hs
> > +++ b/website/src/Application.hs
> > @@ -33,6 

Re: [Snowdrift-dev] [PATCH] Use appStripe as stripe client in handlers

[Bryan Richter]

Much of this patch makes sense to me. I'm pretty sure I know where the
type errors are originating from, too.

Throw rocks at me if I don't get to this in the next 24 hours or so...

On Tue, Jul 11, 2017 at 06:16:58AM +0300, fr33domlover wrote:
> From: fr33domlover 
> 
> This is a new version of the patch, it uses runStripe in appStripe. It
> avoids the StripeClient mess but still has a type error due to
> StripeRunner being having the 'forall io' thing. The errors can be fixed
> using type annotations probably, but using those in every single place
> 'snowstripe' is used would be ridiculous.
> 
> Honestly idk much about Rank N types. I read about it and did some web
> search. And I still don't understand how to properly fix this type error
> thing. I think the error is simpler now but idk how to *elegantly* fix it.
> ---
>  crowdmatch/crowdmatch.cabal|  3 ++-
>  crowdmatch/src/Crowdmatch.hs   |  7 +++
>  website/src/AppDataTypes.hs|  8 
>  website/src/Application.hs |  7 ++-
>  website/src/Handler/PaymentInfo.hs | 11 ++-
>  website/src/Handler/Util.hs|  9 +
>  6 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/crowdmatch/crowdmatch.cabal b/crowdmatch/crowdmatch.cabal
> index d378766..2c602f1 100644
> --- a/crowdmatch/crowdmatch.cabal
> +++ b/crowdmatch/crowdmatch.cabal
> @@ -28,7 +28,8 @@ library
>  hs-source-dirs: src
>  default-language: Haskell2010
>  build-depends:
> -base >=4.8 && <4.9
> +  aeson
> +, base >=4.8 && <4.9
>  , bytestring >= 0.10.6.0
>  , errors
>  , lens
> diff --git a/crowdmatch/src/Crowdmatch.hs b/crowdmatch/src/Crowdmatch.hs
> index fe61387..a857c2a 100644
> --- a/crowdmatch/src/Crowdmatch.hs
> +++ b/crowdmatch/src/Crowdmatch.hs
> @@ -58,10 +58,12 @@ import Control.Error (ExceptT(..), runExceptT, note)
>  import Control.Lens ((^.), from, view, Iso', iso)
>  import Control.Monad (void)
>  import Control.Monad.IO.Class (MonadIO, liftIO)
> +import Data.Aeson (FromJSON)
>  import Data.Function (on)
>  import Data.Int (Int32)
>  import Data.Ratio
>  import Data.Time (UTCTime, getCurrentTime, utctDay)
> +import Data.Typeable (Typeable)
>  import Database.Persist
>  import Database.Persist.Sql (SqlPersistT)
>  import System.IO
> @@ -73,6 +75,7 @@ import Web.Stripe.Customer
>  , createCustomer
>  , deleteCustomer)
>  import Web.Stripe.Error (StripeError)
> +import Web.Stripe.StripeRequest (StripeRequest, StripeReturn)
>  
>  import Crowdmatch.Model hiding (Patron(..))
>  import qualified Crowdmatch.Model as Model
> @@ -87,6 +90,10 @@ import qualified Crowdmatch.Skeleton as Skeleton
>  -- | A method that runs 'SqlPersistT' values in your environment.
>  type SqlRunner io env = forall a. SqlPersistT io a -> env a
>  
> +type StripeClient = forall a.
> +(Typeable (StripeReturn a), FromJSON (StripeReturn a)) =>
> +StripeConfig -> StripeRequest a -> IO (Either StripeError (StripeReturn 
> a))
> +
>  -- | A method that runs 'StripeI' instructions in IO. A default that uses
>  -- 'stripe' is provided by 'runStripe'.
>  type StripeRunner = forall io.
> diff --git a/website/src/AppDataTypes.hs b/website/src/AppDataTypes.hs
> index 39ea400..50d2526 100644
> --- a/website/src/AppDataTypes.hs
> +++ b/website/src/AppDataTypes.hs
> @@ -16,6 +16,7 @@ import Yesod.Core.Types (Logger)
>  import Yesod.GitRev
>  
>  import AuthSite
> +import Crowdmatch
>  import Settings
>  
>  -- | The God-object available to every Handler. This is the site's
> @@ -30,10 +31,9 @@ data App = App
>  , appAuth:: AuthSite
>-- | The function for doing stripe API calls. Swapped out for a mock
>-- thing in tests.
> -, appStripe  :: forall a. (Typeable (StripeReturn a), FromJSON 
> (StripeReturn a))
> - => StripeConfig
> - -> StripeRequest a
> - -> IO (Either StripeError (StripeReturn a))
> +, appStripe  :: StripeConfig
> + -> forall a. StripeI a
> + -> HandlerT App IO (Either StripeError a)
>  }
>  
>  -- This function generates the route types, and also generates the
> diff --git a/website/src/Application.hs b/website/src/Application.hs
> index 761d3f8..b6b20db 100644
> --- a/website/src/Application.hs
> +++ b/website/src/Application.hs
> @@ -33,6 +33,7 @@ import Web.Stripe
>  import Web.Stripe.Error
>  import qualified Yesod.GitRev as G
>  
> +import Crowdmatch
>  import Handler
>  import Handler.Dashboard
>  import Handler.Discourse
> @@ -61,11 +62,7 @@ makeFoundation appSettings = do
>  (if appMutableStatic appSettings then staticDevel else static)
>  (appStaticDir appSettings)
>  
> -let appStripe :: (Typeable (StripeReturn a), FromJSON (StripeReturn a))
> -  => StripeConfig
> -  -> StripeRequest a
> -  -> IO (Either 

Re: [Snowdrift-dev] [PATCH] Use appStripe as stripe client in handlers

[fr33domlover]

I tried replacing all the 'StripeClient' with the actual type. It gives exactly
the same type error. So, no harm in having the type alias.


pgpK943rc6S5J.pgp
Description: OpenPGP digital signature
___
Dev mailing list
Dev@lists.snowdrift.coop
https://lists.snowdrift.coop/mailman/listinfo/dev

Re: [Snowdrift-dev] [PATCH] Use appStripe as stripe client in handlers

[fr33domlover]

On Mon, 10 Jul 2017 18:44:44 -0700
Bryan Richter  wrote:

> To whom it may concern:
> 
> fr33domlover and I know this doesn't compile.. it was submitted for
> discussion.
> 
> Now, on with the discussing:
> 
> On Tue, Jul 11, 2017 at 03:57:31AM +0300, fr33domlover wrote:
> > From: fr33domlover 
> > 
> > ---
> >  crowdmatch/crowdmatch.cabal|  3 ++-
> >  crowdmatch/src/Crowdmatch.hs   | 32 +++-
> >  website/src/AppDataTypes.hs|  6 ++
> >  website/src/Handler/PaymentInfo.hs | 11 ++-
> >  website/src/Handler/Util.hs| 14 ++
> >  5 files changed, 39 insertions(+), 27 deletions(-)
> > 
> > diff --git a/crowdmatch/crowdmatch.cabal b/crowdmatch/crowdmatch.cabal
> > index d378766..2c602f1 100644
> > --- a/crowdmatch/crowdmatch.cabal
> > +++ b/crowdmatch/crowdmatch.cabal
> > @@ -28,7 +28,8 @@ library
> >  hs-source-dirs: src
> >  default-language: Haskell2010
> >  build-depends:
> > -base >=4.8 && <4.9
> > +  aeson
> > +, base >=4.8 && <4.9
> >  , bytestring >= 0.10.6.0
> >  , errors
> >  , lens
> > diff --git a/crowdmatch/src/Crowdmatch.hs b/crowdmatch/src/Crowdmatch.hs
> > index fe61387..ad0489c 100644
> > --- a/crowdmatch/src/Crowdmatch.hs
> > +++ b/crowdmatch/src/Crowdmatch.hs
> > @@ -14,7 +14,9 @@ module Crowdmatch (
> >  , SqlRunner
> >  
> >  -- * Interface with stripe
> > +, StripeClient
> >  , StripeRunner
> > +, runStripeWith
> >  , runStripe
> >  
> >  -- * Store/delete payment tokens
> > @@ -58,10 +60,12 @@ import Control.Error (ExceptT(..), runExceptT, note)
> >  import Control.Lens ((^.), from, view, Iso', iso)
> >  import Control.Monad (void)
> >  import Control.Monad.IO.Class (MonadIO, liftIO)
> > +import Data.Aeson (FromJSON)
> >  import Data.Function (on)
> >  import Data.Int (Int32)
> >  import Data.Ratio
> >  import Data.Time (UTCTime, getCurrentTime, utctDay)
> > +import Data.Typeable (Typeable)
> >  import Database.Persist
> >  import Database.Persist.Sql (SqlPersistT)
> >  import System.IO
> > @@ -73,6 +77,7 @@ import Web.Stripe.Customer
> >  , createCustomer
> >  , deleteCustomer)
> >  import Web.Stripe.Error (StripeError)
> > +import Web.Stripe.StripeRequest (StripeRequest, StripeReturn)
> >  
> >  import Crowdmatch.Model hiding (Patron(..))
> >  import qualified Crowdmatch.Model as Model
> > @@ -87,6 +92,10 @@ import qualified Crowdmatch.Skeleton as Skeleton
> >  -- | A method that runs 'SqlPersistT' values in your environment.
> >  type SqlRunner io env = forall a. SqlPersistT io a -> env a
> >  
> > +type StripeClient = forall a.
> > +(Typeable (StripeReturn a), FromJSON (StripeReturn a)) =>
> > +StripeConfig -> StripeRequest a -> IO (Either StripeError
> > (StripeReturn a))
> > +  
> 
> I had lots of pain trying to use (or avoid using) the type signatures
> around my mock stripe functions. I have a suspicion that this one, in
> particular, is not something we want or need.

This type is simply the type of the 'stripe' function from stripe-haskell
package! With an added Typeable constraint because you used it in appStripe,
idk why but I just followed it.

Obviously we can use that long ugly type instead of the short alias. It's
internal anyway, so doesn't matter.

> 
> >  -- | A method that runs 'StripeI' instructions in IO. A default that uses
> >  -- 'stripe' is provided by 'runStripe'.
> >  type StripeRunner = forall io.
> > @@ -441,20 +450,19 @@ data StripeI a where
> >  ChargeCustomerI :: CustomerId -> Cents -> StripeI Charge
> >  BalanceTransactionI :: TransactionId -> StripeI BalanceTransaction
> >  
> > --- | A default stripe runner
> > -runStripe
> > +runStripeWith
> >  :: MonadIO io  
> > -=> StripeConfig -> StripeI a -> io (Either StripeError a)  
> > -runStripe c = \case
> > +=> StripeClient -> StripeConfig -> StripeI a -> io (Either StripeError
> > a) +runStripeWith strp c = \case  
> 
> We definitely don't want this. At best, we may want a better name than
> 'runStripe'. It is intended to be a default StripeI runner that
> actually uses the real 'stripe'.
> 
> StripeI is already the abstraction -- abstracting over its runner
> shouldn't be necessary. (See more comments below, at the definition of
> 'snowstripe'.)

There is a reason I added 'runStripeWith': It allows to create a runner not
based on 'stripe'. Now, you may ask, but isn't that what 'dummyStripe' is for?
The answer is yes, but you chose to have 'appStripe' in 'App', and respecting
that decision, I had to have a variant of 'runStripe' that wouldn't hard-code
'stripe' in it, and instead would take it as a parameter. That way, you can
pass to it whatever 'appStripe' is in the web app.

An alternative is to change the type of 'appStripe' to have the same type
signature as the 'runStripe' function, and then you can pass 'runStripe' or

Re: [Snowdrift-dev] [PATCH] Use appStripe as stripe client in handlers

[Bryan Richter]

To whom it may concern:

fr33domlover and I know this doesn't compile.. it was submitted for
discussion.

Now, on with the discussing:

On Tue, Jul 11, 2017 at 03:57:31AM +0300, fr33domlover wrote:
> From: fr33domlover 
> 
> ---
>  crowdmatch/crowdmatch.cabal|  3 ++-
>  crowdmatch/src/Crowdmatch.hs   | 32 +++-
>  website/src/AppDataTypes.hs|  6 ++
>  website/src/Handler/PaymentInfo.hs | 11 ++-
>  website/src/Handler/Util.hs| 14 ++
>  5 files changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/crowdmatch/crowdmatch.cabal b/crowdmatch/crowdmatch.cabal
> index d378766..2c602f1 100644
> --- a/crowdmatch/crowdmatch.cabal
> +++ b/crowdmatch/crowdmatch.cabal
> @@ -28,7 +28,8 @@ library
>  hs-source-dirs: src
>  default-language: Haskell2010
>  build-depends:
> -base >=4.8 && <4.9
> +  aeson
> +, base >=4.8 && <4.9
>  , bytestring >= 0.10.6.0
>  , errors
>  , lens
> diff --git a/crowdmatch/src/Crowdmatch.hs b/crowdmatch/src/Crowdmatch.hs
> index fe61387..ad0489c 100644
> --- a/crowdmatch/src/Crowdmatch.hs
> +++ b/crowdmatch/src/Crowdmatch.hs
> @@ -14,7 +14,9 @@ module Crowdmatch (
>  , SqlRunner
>  
>  -- * Interface with stripe
> +, StripeClient
>  , StripeRunner
> +, runStripeWith
>  , runStripe
>  
>  -- * Store/delete payment tokens
> @@ -58,10 +60,12 @@ import Control.Error (ExceptT(..), runExceptT, note)
>  import Control.Lens ((^.), from, view, Iso', iso)
>  import Control.Monad (void)
>  import Control.Monad.IO.Class (MonadIO, liftIO)
> +import Data.Aeson (FromJSON)
>  import Data.Function (on)
>  import Data.Int (Int32)
>  import Data.Ratio
>  import Data.Time (UTCTime, getCurrentTime, utctDay)
> +import Data.Typeable (Typeable)
>  import Database.Persist
>  import Database.Persist.Sql (SqlPersistT)
>  import System.IO
> @@ -73,6 +77,7 @@ import Web.Stripe.Customer
>  , createCustomer
>  , deleteCustomer)
>  import Web.Stripe.Error (StripeError)
> +import Web.Stripe.StripeRequest (StripeRequest, StripeReturn)
>  
>  import Crowdmatch.Model hiding (Patron(..))
>  import qualified Crowdmatch.Model as Model
> @@ -87,6 +92,10 @@ import qualified Crowdmatch.Skeleton as Skeleton
>  -- | A method that runs 'SqlPersistT' values in your environment.
>  type SqlRunner io env = forall a. SqlPersistT io a -> env a
>  
> +type StripeClient = forall a.
> +(Typeable (StripeReturn a), FromJSON (StripeReturn a)) =>
> +StripeConfig -> StripeRequest a -> IO (Either StripeError (StripeReturn 
> a))
> +

I had lots of pain trying to use (or avoid using) the type signatures
around my mock stripe functions. I have a suspicion that this one, in
particular, is not something we want or need.

>  -- | A method that runs 'StripeI' instructions in IO. A default that uses
>  -- 'stripe' is provided by 'runStripe'.
>  type StripeRunner = forall io.
> @@ -441,20 +450,19 @@ data StripeI a where
>  ChargeCustomerI :: CustomerId -> Cents -> StripeI Charge
>  BalanceTransactionI :: TransactionId -> StripeI BalanceTransaction
>  
> --- | A default stripe runner
> -runStripe
> +runStripeWith
>  :: MonadIO io
> -=> StripeConfig -> StripeI a -> io (Either StripeError a)
> -runStripe c = \case
> +=> StripeClient -> StripeConfig -> StripeI a -> io (Either StripeError a)
> +runStripeWith strp c = \case

We definitely don't want this. At best, we may want a better name than
'runStripe'. It is intended to be a default StripeI runner that
actually uses the real 'stripe'.

StripeI is already the abstraction -- abstracting over its runner
shouldn't be necessary. (See more comments below, at the definition of
'snowstripe'.)

>  CreateCustomerI cardToken ->
> -liftIO (stripe c (createCustomer -&- cardToken))
> +liftIO (strp c (createCustomer -&- cardToken))
>  UpdateCustomerI cardToken cust ->
> -liftIO (stripe c (updateCustomer cust -&- cardToken))
> +liftIO (strp c (updateCustomer cust -&- cardToken))
>  DeleteCustomerI cust ->
> -void <$> liftIO (stripe c (deleteCustomer cust))
> +void <$> liftIO (strp c (deleteCustomer cust))
>  ChargeCustomerI cust cents ->
>  liftIO
> -. stripe c
> +. strp c
>  . (-&- cust)
>  -- Supported upstream as of 2016-10-06, but not in our resolver 
> yet
>  -- . (-&- ExpandParams ["balance_transaction"])
> @@ -462,7 +470,13 @@ runStripe c = \case
>  . view chargeCents
>  $ cents
>  BalanceTransactionI transId ->
> -liftIO (stripe c (getBalanceTransaction transId))
> +liftIO (strp c (getBalanceTransaction transId))
> +
> +-- | A default stripe runner
> +runStripe
> +:: MonadIO io
> +=> StripeConfig -> StripeI a -> io (Either StripeError a)
> +runStripe = runStripeWith