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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MonadUnliftIO instance #240

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

newhoggy
Copy link
Contributor

No description provided.

@alaendle
Copy link
Member

Could we please merge this? It makes really sense to support this (guess this issue always pops up in advanced scenarios where a monad transformer stack is build). Sure - this brings in a new dependency - but unliftio-core is super lightweight.

@alaendle
Copy link
Member

Maybe a few thoughts why I believe this is helpful. Generally speaking co-log isn't very user friendly if you really want to use LoggerT to create custom monad stack. Think of:

LoggerT msg (TraceT m) a

Sure we could provide a orphan instance for MonadUnliftIO - but that's not good design; so we wrap this stack in a newtype

newtype AppT msg m a = AppT { runAppT :: LoggerT msg (TraceT m) a }

Fortunately we can derive mostly all the needed monads via LoggerT msg (TraceT m) 馃サ and could add a MonadUnliftIO for AppT. Two things are still unattractive - AppT needs a weird reader MonadReader (LogAction (LoggerT msg (TraceT m)) msg) (which could be also derived) and we need instance for HasLog (LogAction (LoggerT msg (TraceT m)) msg) msg - which could not be derived automatically because of the lens/higher-kinded type variable/coercion - but is relatively easy implement because we just need to wrap/unwrap things:

instance HasLog (LogAction (LoggerT msg (TraceT m)) msg) msg (AppT msg m) where
  getLogAction (LogAction f) = LogAction (AppT . f)
  setLogAction (LogAction f) _ = LogAction (runAppT . f)

To summarize my thoughts: Encapsulating LoggerT is complicated - but that's another story; for easier scenarios it might be better to just eschew the newtype (and maybe use an alias) - but for this scenario it would be nice if MonadUnliftIO is directly supported.

Hope you could follow me; somehow it's hard to explain.

@alaendle alaendle self-requested a review as a code owner September 6, 2023 04:38
@alaendle alaendle merged commit e294cbe into co-log:main Sep 6, 2023
6 checks passed
@alaendle
Copy link
Member

alaendle commented Sep 6, 2023

@newhoggy Many thanks for your contribution!

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

Successfully merging this pull request may close these issues.

None yet

2 participants