-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support for Lombok log annotations in three recipes #115
Conversation
rewriteRun( | ||
spec -> spec.recipe(new PrintStackTraceToLogError(null, null, null)) | ||
.parser(JavaParser.fromJavaVersion().classpath("slf4j-api", "lombok")) | ||
.typeValidationOptions(TypeValidation.builder().identifiers(false).build()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the fields are absent, we have missing type information in the result. I figured that's ok & to be expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is no way to get the type information from the annotation processor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't run the Lombok annotation processor, so we don't have that information available; it's good enough to make this code change; not quite sure if any followup recipes would work, but the JavaTemplates we use should have the type information, as per
rewrite-logging-frameworks/src/main/java/org/openrewrite/java/logging/SystemErrToLogging.java
Lines 178 to 185 in 53f8787
public <P> JavaTemplate getErrorTemplateNoException(JavaVisitor<P> visitor) { | |
switch (framework) { | |
case SLF4J: | |
return JavaTemplate | |
.builder("#{any(org.slf4j.Logger)}.error(#{any(String)})") | |
.contextSensitive() | |
.javaParser(JavaParser.fromJavaVersion().classpath("slf4j-api")) | |
.build(); |
J.Identifier logField = loggers.iterator().next().getVariables().get(0).getName(); | ||
m = replaceMethodInvocation(m, logField); | ||
} else if (clazz.getAllAnnotations().stream().anyMatch(lombokLogAnnotationMatcher::matches)) { | ||
String fieldName = loggerName == null ? "log" : loggerName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default fields are named log
, as per: https://projectlombok.org/features/log
Didn't quite feel like parsing any lombok configuration files in case folks set a different lombok.log.fieldName
.
If they do that, then can set the loggerName
option to have that be used for the logging statements instead.
These three recipes are not included in composite recipes, mostly because they have options that folks might want to set. As such there's limited risk in these changes; folks will only run these explicitly, and can define their own options to tweak the behavior. |
What's changed?
Detect for instance
@Slf4j
in recipes that move away fromSystem.out
,System.err
andprintStackTrace
.What's your motivation?
Anything in particular you'd like reviewers to focus on?
Should we cover more recipes already?
Have you considered any alternatives or workarounds?
We could have generically fixed this by adding synthetic log fields to the associated classes, but I figured that's harder to do for a quick win here.
Any additional context