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

Add withLogger and generalized LoggingT' #43

Closed
wants to merge 22 commits into from
Closed

Conversation

chris-martin
Copy link
Contributor

@chris-martin chris-martin commented May 30, 2024

This is technically a breaking change, though not breaking for any of our apps.

This change allows "application monads" (such as AppT and AppExample in freckle-app) to remove LoggingT from their monad stack. Instead, they can derive MonadLogger via LoggingT' app m, when the constraint HasLogger app is satisfied. (I've tried this in freckle-app and confirmed it works.)

LoggingT' is like LoggingT but the reader context is generalized from Logger to HasLogger env => env. LoggingT is just a type alias for LoggingT' Logger for compatibility. It would look nicer if we just call the more generic one LoggingT and remove the specialized type alias, but leaving it this way makes upgrading smoother.

A new function withLogger is provided, which will be used instead of runLoggerLoggingT to wrap the setup and teardown of an app. Typical usage looks like:

withApp continue = do
  loggingSettings <- _
  withLogger loggingSettings $ \logger -> do
    _
    continue App{logger, ..}

@chris-martin chris-martin requested a review from pbrisbin May 30, 2024 19:51
@chris-martin chris-martin force-pushed the chris/loggingT branch 4 times, most recently from 4efee5a to b0540f3 Compare May 30, 2024 21:09
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@chris-martin
Copy link
Contributor Author

While I'm here, I'm trying to get the GHA build down to 0 warnings. I've replaced the deprecated hlint-run action with a new thing called hlint-scan. I think it seems good?

@chris-martin
Copy link
Contributor Author

Dropping support for GHC 8.4 because it doesn't support DerivingVia. I hope this is an acceptable loss.

Comment on lines +119 to +124
runLoggerLoggingT
:: (MonadUnliftIO m, HasLogger env) => env -> LoggingT m a -> m a
runLoggerLoggingT env f =
runLoggingT f logger `finally` flushLogStr logger
where
logger = env ^. loggerL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you continue to use the real LoggingT here, and implement this as something like,

runLoggerLoggingT
  :: (MonadUnliftIO m, HasLogger env) => env -> LoggingT m a -> m a
runLoggerLoggingT env (LoggingT f) =
  f (runLogAction logAction) `finally` flushLogStr logger
 where
  logger = env ^. loggerL
  logAction = loggerLogAction logger

Comment on lines +11 to +19
loggerLogAction :: Logger -> LogAction IO
loggerLogAction logger =
LogAction $ \loc source level msg ->
when (lShouldLog logger source level) $
defaultOutputWith options loc source level msg
where
options = defaultOutputOptions $
\logLevel bytes ->
pushLogStrLn logger $ toLogStr $ getLoggerReformat logger logLevel bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think LogAction m is actually very useful. Why not do something simpler:

runLogAction
  :: ToLogStr msg => Logger -> Loc -> LogSource -> LogLevel -> msg -> IO ()
runLogAction logger lloc source level msg ->
    when (lShouldLog logger source level) $
      defaultOutputWith options loc source level msg
 where
  options = defaultOutputOptions $
    \logLevel bytes ->
      pushLogStrLn logger $ toLogStr $ getLoggerReformat logger logLevel bytes

Then in my other comment, it's similarly simplified:

runLoggerLoggingT
  :: (MonadUnliftIO m, HasLogger env) => env -> LoggingT m a -> m a
runLoggerLoggingT env (LoggingT f) =
  f (runLogAction logger) `finally` flushLogStr logger
 where
  logger = env ^. loggerL

I think you'd drop about 220 lines out of this PR, lose all your annoying CPP and older-ghc fuss -- with no difference in functionality.

We would still want to provide something for users of the library to derive MonadLogger, but I think that can also be simpler:

newtype ReaderLogger m a = ReaderLogger (m a)
  deriving newtype
    ( Functor
    , -- ...
    )

instance (MonadReader env m, HasLogger env) => MonadAWS (ReaderLogger m) where
  monadLoggerLog loc source level msg = do
    logger <- view loggerL
    runLogAction logger loc source level msg

(This is just the same pattern I use most places, e.g. amazonka-mtl)

@chris-martin
Copy link
Contributor Author

Starting over with #44

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.

2 participants