Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing MonadBase instance for ClientM #663

Closed
edsko opened this issue Jan 13, 2017 · 11 comments
Closed

Missing MonadBase instance for ClientM #663

edsko opened this issue Jan 13, 2017 · 11 comments
Milestone

Comments

@edsko
Copy link
Contributor

edsko commented Jan 13, 2017

It's easily defined

instance MonadBase IO ClientM where
  liftBase = ClientM . liftBase

but I don't know if this will cause an additional unwanted dependency.

Incidentally, why is the ClientM monad hardcoded? It would be nicer if we could define our own, just like we can for the server.

@phadej
Copy link
Contributor

phadej commented Jan 13, 2017

For the record, transformers-base doesn't bring in anything else than itself. Also monad-control adds only itself (and transformers-base). I'm ok with defining instances for classes in these packages.

@phadej
Copy link
Contributor

phadej commented Jan 13, 2017

Hmm. Maybe it can be made so client returns forall m. (MonadIO m, MonadError e m) => m ..., but will we gain anything from that change (I know we will lose things)

@edsko
Copy link
Contributor Author

edsko commented Jan 13, 2017

I was thinking something more along the line of ClientT ... m, just like for ServerT.

@phadej
Copy link
Contributor

phadej commented Jan 13, 2017

@edsko On the other hand, I'm not sure there is that many applications having

client HasServer CustomCombinator :> api where
    type ServerT (CustomCombinator :> api) m = ServerT api (CustomT m)

as in practice you have to run the CustomT in the route implementation. But if you have some cool tricks, please share. We are looking for "advanced examples" for the tutorial.

In fact, all combinators in servant-server (AFAICS) could be rewritten with more simple:

class HasServer api where
  type ServerM api :: *

  route ::
       Proxy api
    -> Context context
    -> Delayed env (ServerM api)
    -> Router env

Also if you want to do something like that for Client (i.e. have a use case in mind), please open another issue.

@phadej
Copy link
Contributor

phadej commented Jan 13, 2017

And to return to this issue: @alpmestan I'm very lenient with adding "not far from Haskell Platform" upto lens -dependencies, as we use e.g. swagger2 so it's all there anyway (later or sooner). What is servant's policy?

@edsko
Copy link
Contributor Author

edsko commented Jan 13, 2017

No, but in the server it's easy to replace the underlying monad completely (which I do all the time since EitherT does not play nice with exceptions as it does not and cannot implement MonadMask). But in the client I cannot use a application specific client monad at all.

@edsko
Copy link
Contributor Author

edsko commented Jan 13, 2017

While we're at it, this instance is also missing (and it wrecks my head every time I have to define it :)

type ClientM' = ReaderT ClientEnv (ExceptT ServantError IO)

instance MonadBaseControl IO ClientM where
  type StM ClientM a = StM ClientM' a

  liftBaseWith :: (RunInBase ClientM IO -> IO a) -> ClientM a
  liftBaseWith f = ClientM (liftBaseWith (\g -> f (g . runClientM')))

  restoreM :: StM ClientM a -> ClientM a
  restoreM st = ClientM (restoreM st)

@edsko
Copy link
Contributor Author

edsko commented Jan 13, 2017

That one relies on monad-control.

@alpmestan
Copy link
Contributor

To be entirely honest, I don't feel like it's appropriate to reject instances that "force" us to bring in 2 more small deps, when our transitive dependency chain is so large already. I mean, we should not be looking to double the number of transitive deps for servant, but sensible instances that are needed and that don't make us pull another 20 deps should be accepted IMO.

Regarding the policy itself, I think that's pretty close to what we've been doing so far, but it's not set in stone :)

@phadej
Copy link
Contributor

phadej commented Jan 13, 2017

@edsko do you want to make a PR?

@edsko
Copy link
Contributor Author

edsko commented Jan 21, 2017

Thanks @phadej !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants