Fix the issue 196 #199
Fix the issue 196 #199
Conversation
|
|
||
| public class SpotBugsPlugin implements Plugin<Project> { | ||
| public static final String CONFIG_NAME = "spotbugs"; | ||
| public static final String PLUGINS_CONFIG_NAME = "spotbugsPlugins"; | ||
| public static final String SLF4J_CONFIG_NAME = "spotbugsSlf4j"; | ||
| public static final String EXTENSION_NAME = "spotbugs"; | ||
|
|
||
| private final Logger log = LoggerFactory.getLogger(SpotBugsPlugin.class); |
There was a problem hiding this comment.
This could instead be LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); so that it can be copy-pasted to other classes. Also, why not static?
There was a problem hiding this comment.
I'm not so positive to make copy-paste friendly in this case. The code with MethodHandles isn't so intentional.
To avoid the pointed mistake, since we use JDK11 to build in CI, I wanna use Errorprone with SLF4J plugin that can detect it. I'll make separated PR.
Also, why not static?
It simply has no merit nor demerit. Plugin instance is created just few times per build.
There was a problem hiding this comment.
Ouch, we code in Groovy so Errorprone doesn't work. Now it's time to use SpotBugs! :)
There was a problem hiding this comment.
Note that PMD will flag non-static loggers as a mistake unless passed into the constructor: https://pmd.github.io/latest/pmd_rules_java_errorprone.html#properlogger
Why not make logging statements copy-paste friendly? They're gonna be copy-pasted at some point, may as well avoid the risk of forgetting to update the classname.
There was a problem hiding this comment.
Confirmed that SpotBugs SLF4J plugin cannot detect the issue, because Groovy compiles this code into the following format:
Object var5 = var4[0].call(LoggerFactory.class, Object.class);
this.log = (Logger)ScriptBytecodeAdapter.castToType(var5, Logger.class);
so I'll follow your suggestion at this moment.
There was a problem hiding this comment.
Why not make logging statements copy-paste friendly?
I believe it's not the way to go. And if I do, I will choose LoggerFactory.getLogger(getClass()) which is more intuitive code to read.
BTW static-or-not is not so problem, FAQ says both are OK. Then I personally prefer the less-magic way.
There was a problem hiding this comment.
getClass() works for an instance logger, yes.
|
Kudos, SonarCloud Quality Gate passed!
|
|
🎉 This PR is included in version 4.0.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
close #196