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.

Sign up for GitHub

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

Jump to bottom

Add structured equivalents of Writer & WriterT loggers #719

Merged
merged 6 commits into from
Apr 16, 2023
Merged

Add structured equivalents of Writer & WriterT loggers #719

merged 6 commits into from
Apr 16, 2023

Conversation

morgen-peschke
Copy link
Contributor

Adds WriterStructuredLogger and WriterTStructuredLogger as equivalents of WriterLogger and WriterTLogger, respectively.

Implements #718

rossabaker
rossabaker reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I like it, for surfacing the test code as much as the WriterT.

core/shared/src/main/scala/org/typelevel/log4cats/extras/WriterStructuredLogger.scala Outdated Show resolved Hide resolved
testing/README.md Show resolved Hide resolved
core/shared/src/main/scala/org/typelevel/log4cats/extras/WriterTStructuredLogger.scala Outdated Show resolved Hide resolved
rossabaker
rossabaker approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I like this a lot. Useful with the right safety precautions, disastrous without, and a clear explanation which is which.

@rossabaker rossabaker added this to the 3.6.0 milestone Feb 20, 2023
danicheg
danicheg approved these changes Apr 5, 2023
View reviewed changes
Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

Wow, such great PR is stuck for no reason! I dropped a minor question, so feel free to pass it over.

core/shared/src/main/scala/org/typelevel/log4cats/extras/WriterLogger.scala
* A `SelfAwareLogger` implemented using `cats.data.Writer`.
*
* >>> WARNING: READ BEFORE USAGE! <<<
* /~https://github.com/typelevel/log4cats/blob/main/core/shared/src/main/scala/org/typelevel/log4cats/extras/README.md
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to put these warnings on the microsite and leave here the link on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC neither of these classes is on the microsite, so putting the warning there might draw people to them, rather than nudge them towards less niche classes.

@danicheg danicheg merged commit 95e26a1 into typelevel:main Apr 16, 2023
@morgen-peschke morgen-peschke deleted the add-writerT-structured-logger branch April 17, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danicheg danicheg danicheg approved these changes

@ChristopherDavenport ChristopherDavenport ChristopherDavenport approved these changes

@rossabaker rossabaker rossabaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@morgen-peschke @rossabaker @ChristopherDavenport @danicheg