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 WriterT logger #120

Merged
merged 4 commits into from
Jan 25, 2019
Merged

Conversation

DennisVDB
Copy link

Here's an implementation of a WriterT logger (see #80). I based myself on how the current Writer logger is implemented.

I went through a handful of iterations and I'm still not sure if it's correct (it compiles). Especially, run: WriterT[F, G[LogMessage], ?] ~> F seems too constrained. I tried implementation it with the shape WriterT[F, G[LogMessage], ?] ~> H but couldn't.

@ChristopherDavenport
Copy link
Member

As mentioned on gitter my holdout on this is that we either should put very large warning labels on it, or not include it. WriterT suffers that if any errors occur in F all values written are lost. Which makes sense for the abstraction but can be very surprising.

I'm inclined to the large warning label documentation, and then not include it in any supplied docs so that its available if people are certain thats what they want and go looking for it, but we sufficiently keep it away from wide use. It certainly has a place.

@DennisVDB
Copy link
Author

The documentation I've added should straight to the point. What would you think about putting it under ...log4cats.extras.unsafe?

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Choose a reason for hiding this comment

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

I think this is a fine location with the new warning.

@ChristopherDavenport ChristopherDavenport merged commit ecfa62f into typelevel:master Jan 25, 2019
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