Conversation
23Skidoo
left a comment
There was a problem hiding this comment.
LGTM, 👍 for not forgetting to bump the major version.
log-base/src/Log/Class.hs
Outdated
| localData :: [Pair] -> m a -> m a | ||
| -- | Extend the current application domain locally. | ||
| localDomain :: T.Text -> m a -> m a | ||
| -- | Capture current context of a logger for use in the 'IO' monad. |
There was a problem hiding this comment.
I'd reword this comment as "Returns an IO action that logs messages using the current MonadLog context. Useful for interfacing with libraries such as aws or amazonka that accept logging callbacks that operate in IO." to make the purpose of this function more clear.
log-base/src/Log/Class.hs
Outdated
| -- | Extend the current application domain locally. | ||
| localDomain :: T.Text -> m a -> m a | ||
| -- | Capture current context of a logger for use in the 'IO' monad. | ||
| getLoggerIO :: m (UTCTime -> LogLevel -> T.Text -> Value -> IO ()) |
There was a problem hiding this comment.
This makes MonadLog unimplementable for Non-IO based monads.
I'd rather prefer separate class, e.g. MonadLogIO (though NoLogT m could still have an instance for all m). That wouldn't require major bump, and be a little more flexible.
There was a problem hiding this comment.
Hmm, I see. I'd like to avoid additional class. How about replacing getLoggerIO with getLoggerEnv :: m LoggerEnv? It's more general as you can use it in conjunction with logMessageIO to get IO logging, but doesn't introduce any IO into the class.
There was a problem hiding this comment.
@arybczak that sounds 👍 if it works for you, though it will still be breaking change :)
|
I updated the PR, it should be fine now. |
Needed for passing Logger to libraries such as
awsoramazonkathat operate in the IO monad.