Feature/mtl refactor

[solomon-b]

I wanted to see what the API would look like if converted to a monad transformer stack. I think it cleans things up quite nicely. What do you think? If we decide to merge this, I'll definitely add more docs.

[TristanCacqueray]

I meant to keep this library as simple as possible (and show how good haskell is, even when not using fancy extensions), but on the other hand, mtl is such a major design that is worth learning so I'm ok to merge this.

For the tutorial purpose, I wonder if we could expose a createWithSession version of the createSession which would return the runMatrix helper directly, so that the documentation would become:

withSession <- createWithSession "https://matrix.org" token
Right userId <- withSession getTokenOwner

Similarly, it might be good to expose a try helper to remove the except layer, so that api calls that are expected to fail are easy to catch, e.g. something like MatrixIO a -> MatrixIO (Either MatrixError a).

Otherwise, I also think this cleans things up nicely, thanks you!

[solomon-b]

I meant to keep this library as simple as possible (and show how good haskell is, even when not using fancy extensions), but on the other hand, mtl is such a major design that is worth learning so I'm ok to merge this.

I get where you are coming from and I don't think we should go too crazy with the library. I think its good to treat it as a fairly low level binding to the Matrix REST API that doesn't do anything too magical.

I think Monad Transformers are standard enough that its reasonable to use them here. Its really nice to not have to thread the ClientSession through everywhere explicitly or case on Eithers explicitly when it isn't necessary.

[solomon-b]

For the tutorial purpose, I wonder if we could expose a createWithSession version of the createSession which would return the runMatrix helper directly, so that the documentation would become:

how do this look?

createWithSession ::
  -- | The matrix client-server base url, e.g. "https://matrix.org"
  T.Text ->
  -- | The user token
  MatrixToken ->
  -- | The matrix action to perform
  MatrixIO a ->
  IO (Either MatrixError a)
createWithSession baseUrl' token' action = do
  session <- createSession baseUrl' token'
  runMatrixIO session action

Or should we make it polymorphic on the m?

createWithSession ::
  MonadIO m =>
  -- | The matrix client-server base url, e.g. "https://matrix.org"
  T.Text ->
  -- | The user token
  MatrixToken ->
  -- | The matrix action to perform
  MatrixM m a ->
  m (Either MatrixError a)
createWithSession baseUrl' token' action = do
  session <- liftIO $ createSession baseUrl' token'
  runMatrixM session action

We can have both i suppose:

createWithSessionIO ::
  -- | The matrix client-server base url, e.g. "https://matrix.org"
  T.Text ->
  -- | The user token
  MatrixToken ->
  -- | The matrix action to perform
  MatrixIO a ->
  IO (Either MatrixError a)
createWithSessionIO = createWithSession

[solomon-b]

Similarly, it might be good to expose a try helper to remove the except layer, so that api calls that are expected to fail are easy to catch, e.g. something like MatrixIO a -> MatrixIO (Either MatrixError a).

I'm not sure how this would work, or I am misunderstanding the idea. MatrixIO is ExceptT MatrixError (ReaderT ClientSession IO) a so if we unwrapped the ExceptT then we wouldn't have a MatrixIO anymore?

[TristanCacqueray]

Thank you for the update, I'll give this a try and update the tutorial. My suggestions are mostly for basic ghci usage, where previously you would do:

> sess <- createSession "https://matrix.org" token
> Right room <- joinRoom sess "#test"
> sendMessage sess room message

With the createWithSession, that would be:

> withSession <- createWithSession "https://matrix.org" token
> Right room <- withSession $ joinRoom "#test"
> withSession $ sendMessage room message

Thus I think it should return a callback, otherwise it seems like it would re-create the manager each time:

createWithSession ::
  MonadIO m =>
  -- | The matrix client-server base url, e.g. "https://matrix.org"
  T.Text ->
  -- | The user token
  MatrixToken ->
  -- | The matrix action to perform
  m (MatrixM m a -> m (Either MatrixError a))
createWithSession baseUrl' token' = do
  session <- liftIO $ createSession baseUrl' token'
  pure $ \action -> runMatrixM session action

Well, otherwise I'm not sure how the Tutorial module can be updated.

And since we are updating the library API, I think it would be better to add the http manager to the argument of createSession and createIdentitySession so that the caller can pick http-client-openssl, or http-mock if needed. Then only the simplest createWithSession would create it automatically.

If that sounds good to you, I can add the change to the PR later.

[TristanCacqueray]

Similarly, it might be good to expose a try helper to remove the except layer, so that api calls that are expected to fail are easy to catch, e.g. something like MatrixIO a -> MatrixIO (Either MatrixError a).

I'm not sure how this would work, or I am misunderstanding the idea. MatrixIO is ExceptT MatrixError (ReaderT ClientSession IO) a so if we unwrapped the ExceptT then we wouldn't have a MatrixIO anymore?

In the event a MatrixError is expected, it seems like the ExceptT is going to shortcut the user defined action unless catchError is used. Well I guess that's ok, let's not bother with that for now.

[TristanCacqueray]

Here is a rebase to include the loginToken. I'll now update the tutorial and propose adding the Manager to the login/createsSession arguments.

[TristanCacqueray]

Sorry about the createWithSession suggestion, I was able to update the Tutorial without it, and with hindsight, it may not be useful. My last remaining concern is the 'login' function which I would like to change to a withLogin to force the logout call on exit. But that could be done in a separate PR.

If that is ok with you, then I think we should merge this. Thanks again for doing the refactor.

[berberman]

IMO we'd better call this MatrixT as it's a monad transformer. Also, we can have a interface like:

class MonadMatrix m where
  getClientSession :: m ClientSession
  throwMatrixError :: MatrixError -> m a
  performHttpRequest :: FromJSON a => HTTP.Request -> m a

Then MatrixT could be a default implementation of MonadMatrix:

instance MonadIO m => MonadMatrix (MatrixT m) where
  getClientSession = ask
  throwMatrixError = throwError
  performHttpRequest = ...

and users may create their own monads with or wihout MatrixT.

[TristanCacqueray]

Presently, this is going to be tricky when using both a regular session and a identity session, I think we need to implement a ClientSession that support both endpoints, otherwise I think it's going to be tricky to implement such Monad for both use-case.

[solomon-b]

I haven't worked on this project in a while but I should have some time coming up in a few weeks. Is there anything y'all would like me to do to assist here?

[TristanCacqueray]

I guess changing the name from MatrixM to MatrixT is reasonable, though I don't get what's the benefit of having a MonadMatrix class, e.g. I find the 'throwMatrixError' method confusing.

Thanks @solomon-b, would you have time to rebase the PR?

[TristanCacqueray]

In the CHANGELOG, I would add a migration note, something like:

• Migrate to the new API by replacing call site from Network.Matrix.Client.method session arg to runMatrixIO session (method arg)

It's also a bit annoying that the Identity API requires a different token and the present type doesn't prevent using the wrong session. I guess we could tag the Session and/or include both token...