-
Notifications
You must be signed in to change notification settings - Fork 67
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
Try removing MonadIO superclass #119
base: master
Are you sure you want to change the base?
Conversation
This superclass isn't really needed by the Katip class itself. Basically it prevents you from adding a MonadIO constraint if you do use logging, but: 1. You get confusing redundant constraint errors when you also have a MonadIO constraint. 2. It forces you to add MonadIO in some cases where you're not actually logging. I had 10 minutes to kill so I thought I'd see how hard it'd be to remove. This is in reference to #88
Also there is lots of pure core, or code just using Reader/State which you want to do logging from as it computes (ie not log everything once it is done computing), but you don't want it to be able to access IO. I strongly second that PR. |
I don't disagree with the spirit of this change and it seems to be generally harmless. Further, delaying the superclass constraint in general makes sense, so I also support this change. However, since all logging actions get the It may be worth deliberating whether we can take a typeclass approach on the actions themselves - e.g. one of them could be a typeclass member, enabling all others to automatically inherit. This way, you could have logging actions that don't need |
I agree that this would be nice to have. I like isolating effects (e.g. MTL-style), which is a bit pointless in the presence of -- copy Katip functions we want here
class Monad m => MonadLogger m where
logFm :: Severity -> LogStr -> m ()
addNamespace :: Namespace -> m a -> m a
...
-- AppT has instances for Katip, KatipContext
instance MonadIO m => MonadLogger (AppT e m) where
logFm = Katip.logFM
...
-- No more MonadIO!
foo :: MonadLogger m => m ()
foo = do
logFm InfoS (LogStr "Info...")
... This isn't too bad, though it's pretty boilerplate-heavy. It would be nice if this wasn't necessary, but if the |
A tangible benefit of this change is that it makes integration with effect systems like effectful easier. For example, with a import Effectful
import Effectful.Dispatch.Static
import Katip.Monadic
data KatipContextE :: Effect
type instance DispatchOf KatipContextE = 'Static 'WithSideEffects
newtype instance StaticRep KatipContextE = KatipContextE {unKatipContextE :: KatipContextTState}
-- These don't compile with MonadIO as a superclass of Katip and KatipContext
instance (KatipContextE :> es) => Katip (Eff es) where
getLogEnv = ltsLogEnv . unKatipContextE <$> getStaticRep
localLogEnv f = localStaticRep $ \(KatipContextE lts) -> KatipContextE (lts {ltsLogEnv = f (ltsLogEnv lts)})
instance (KatipContextE :> es) => KatipContext (Eff es) where
getKatipContext = ltsContext . unKatipContextE <$> getStaticRep
getKatipNamespace = ltsNamespace . unKatipContextE <$> getStaticRep
localKatipContext f = localStaticRep $
\(KatipContextE lts) -> KatipContextE (lts {ltsContext = f (ltsContext lts)})
localKatipNamespace f = localStaticRep $
\(KatipContextE lts) -> KatipContextE (lts {ltsNamespace = f (ltsNamespace lts)}) Moving |
@ozataman It seems like from the commentary here that there are a few cases here that would benefit from deferring the |
I'd just like to add that I ran into this issue myself when trying to integrate Katip into my setup using Effectful, as @jmorag indicated. Is there anything blocking this PR that I can help with? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there's now https://hackage.haskell.org/package/katip-effectful that would benefit from this PR being merged, then at least the author wouldn't have to overwrite class methods with versions that don't include the MonadIO
constraint.
@@ -811,12 +811,12 @@ closeScribes le = do | |||
-- of the log env that are reverted when the supplied monad | |||
-- completes. 'katipNoLogging', for example, uses this to temporarily | |||
-- pause log outputs. | |||
class MonadIO m => Katip m where | |||
class Katip m where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be
class Katip m where | |
class Monad m => Katip m where |
then you won't have to include these Monad
constraints below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arybczak is that preferable though? I thought part of the idea of this change was to not require unused constraints. It makes sense to me if an implementation requires Monad that it should demand it but the base class doesn't seem to demand it. Maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought part of the idea of this change was to not require unused constraints
The problem isn't really unused constraints, it's just that you're forcing MonadIO
on downstream users of Katip
and MonadIO
is huge (in a sense of what you can do with it). Monad
on the other hand isn't, so including it as a superclass allows you to omit writing (Katip m, Monad m)
in type signatures (which is somewhat annoying) and doesn't give any practical downsides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, full "de-IOnification" would require you putting some of the generic logging functions into the Katip
or KatipContext
class so they also don't require MonadIO
constraint, but that is further API breaking.
@MichaelXavier You can have a look at https://hackage.haskell.org/package/log-base-0.12.0.1/docs/Log-Class.html for comparison - MonadLog
defines generic logging operations and then there are some specific logging helper functions, but you don't need MonadIO
in scope to call them because they just use MonadLog
methods.
@@ -511,7 +511,7 @@ instance MonadIO m => KatipContext (NoLoggingT m) where | |||
|
|||
-- | Convenience function for when you have to integrate with a third | |||
-- party API that takes a generic logging function as an argument. | |||
askLoggerIO :: (Applicative m, KatipContext m) => m (Severity -> LogStr -> IO ()) | |||
askLoggerIO :: (Applicative m, KatipContext m, MonadIO m) => m (Severity -> LogStr -> IO ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this requires MonadIO
, right? You return an IO
function, but the constraint doesn't seem needed to do that.
It looks similar to https://hackage.haskell.org/package/log-base-0.12.0.1/docs/Log-Monad.html#v:getLoggerIO.
@@ -811,12 +811,12 @@ closeScribes le = do | |||
-- of the log env that are reverted when the supplied monad | |||
-- completes. 'katipNoLogging', for example, uses this to temporarily | |||
-- pause log outputs. | |||
class MonadIO m => Katip m where | |||
class Katip m where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought part of the idea of this change was to not require unused constraints
The problem isn't really unused constraints, it's just that you're forcing MonadIO
on downstream users of Katip
and MonadIO
is huge (in a sense of what you can do with it). Monad
on the other hand isn't, so including it as a superclass allows you to omit writing (Katip m, Monad m)
in type signatures (which is somewhat annoying) and doesn't give any practical downsides.
@@ -937,7 +937,7 @@ katipNoLogging = localLogEnv (\le -> set logEnvScribes mempty le) | |||
-- | Log with everything, including a source code location. This is | |||
-- very low level and you typically can use 'logT' in its place. | |||
logItem | |||
:: (A.Applicative m, LogItem a, Katip m) | |||
:: (A.Applicative m, LogItem a, Katip m, MonadIO m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why still require Applicative m
? Shouldn't that be implied by MonadIO
?
This superclass isn't really needed by the Katip class
itself. Basically it prevents you from adding a MonadIO constraint if
you do use logging, but:
MonadIO constraint.
actually logging.
I had 10 minutes to kill so I thought I'd see how hard it'd be to
remove.
This is in reference to #88
@ozataman LMK if you're okay with this.