Migrate to SLF4J for logging#1120
Conversation
nedtwigg
left a comment
nedtwigg
left a comment
There was a problem hiding this comment.
Thanks very much, this LGTM. I'd like one other committer to sign-off on this before I press merge though.
lutovich
left a comment
lutovich
left a comment
There was a problem hiding this comment.
Looks good to me 👍
Just curious, why do we use LoggerFactory.getLogger(Type.class.getName()) instead of LoggerFactory.getLogger(Type.class)?
| } | ||
| logger.log( | ||
| Level.SEVERE, | ||
| logger.error( |
There was a problem hiding this comment.
Perhaps use a {} placeholder here instead of mavenCoords
|
@jamietanna can you address the points @lutovich raised above? |
I'd kept this in line with the existing code to do this, as I'd assumed that there was a reason behind it. Looking at the code for I'm happy amending it, as it then looks more common for SLF4J, and that can then perform the |
As noted in diffplug#1116, it'd be ideal if we moved to SLF4J as our logging interface. We can do this pretty easily, making sure we remove our `package-list` reference, as well as making sure the SLF4J API is only used for compilation. We also need to map the error logging levels that most closely mirror what `java.util.logging` uses. Closes diffplug#1116.
As it removes unnecessary String concatenation at runtime, if the log levels aren't enabled.
As SLF4J allows passing in a `Class<?>`, instead of a `String` for the class name, we can simplify interactions with the logger creations.
|
If we're adopting log4j, let's do it canonically. Also take a look at "Perhaps use a {} placeholder here instead of mavenCoords". Thanks! |
jamietanna
force-pushed
the
feature/slf4j
branch
from
d387c37 to
85d15b5
Compare
|
Done, thanks for the speedy reply! |
|
Thanks! In the future, please add new commits instead of force-pushing, easier to review :) |
|
Sorry about that. I would've |
|
Looks good. Thanks for addressing my comments! |
|
Thanks a ton for taking on the SLF4J migration @jamietanna! |
|
Published in |
3 participants
Closes #1116.