bitemyapp / esqueleto

New home of Esqueleto, please file issues so we can get things caught up!
BSD 3-Clause "New" or "Revised" License
370 stars 107 forks source link

Can `ReaderT` usage be replaced with `MonadReader` #340

Closed tysonzero closed 1 year ago

tysonzero commented 1 year ago

We are having to lift fairly often in our codebase when calling select and similar due to using db code alongside other useful monad transformers.

IIRC someone mentioned in the past that it was too unfriendly to type inference. However I'm not sure that is still the case given the switch to concrete types instead of the tagless final stuff.

parsonsmatt commented 1 year ago

Yeah, should be - I'd imagine you can write:

mySelect :: (MonadReader backend m, HasSqlBackend backend, MonadIO m, SqlSelect a r) => SqlQuery a -> m r
mySelect query = do
  sqlBackend <- asks getSqlBackend 
  liftIO $ runReaderT (select query) sqlBackend

and that ought to work fine.

belevy commented 1 year ago

Feel free to lift this in your own prelude but I do not think this is a good addition to the library.

tysonzero commented 1 year ago

@belevy Is there any particular reason for preferring ReaderT over MonadReader?

Behind the scenes it literally does:

import qualified Control.Monad.Trans.Reader as R

select query = do
    res <- rawSelectSource SELECT query
    conn <- R.ask
    liftIO $ with res $ flip R.runReaderT conn . runSource

Why the preference for that over:

import qualified Control.Monad.Reader as R

select query = do
    res <- rawSelectSource SELECT query
    conn <- R.ask
    liftIO $ with res $ flip R.runReaderT conn . runSource

The only difference is the import, and you get much nicer ergonomics when interoperating with other transformers.

belevy commented 1 year ago

ReaderT is used consistently in persistent and would prefer to remain consistent. I personally have never felt this pain that you are feeling and am not convinced it's universally better. @parsonsmatt may have different feelings.

tysonzero commented 1 year ago

To be clear this proposal would in the long run ideally be a full on ReaderT-ectomy, replacing just about all uses of Control.Monad.Trans.Reader.ask with Control.Monad.Reader.ask.

I'm not aware of any non-trivial downsides, assuming I am correct in my assumption of inference not being meaningfully affected.