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

Consider Lombok annotations for loggers #114

Closed
miguelchico opened this issue Oct 26, 2023 · 2 comments · Fixed by #115
Closed

Consider Lombok annotations for loggers #114

miguelchico opened this issue Oct 26, 2023 · 2 comments · Fixed by #115
Assignees
Labels
enhancement New feature or request

Comments

@miguelchico
Copy link

miguelchico commented Oct 26, 2023

What problem are you trying to solve?

I'm trying to use logging recipes in a project that uses Lombok to declare SLF4J loggers.
The recipes I've tried so far don't change my source code if the logger instance is created by the Lombok Slf4j annotation.

For example try to use the "Use logger instead of printStackTrace()" with the following code:

package justatest;

import org.slf4j.Logger;

@Slf4j
public class Test {

  void thisIsJustATest() {
      try {
      } catch (Exception e) {
        e.printStackTrace()
      }
  }

}

and it won't make any changes to the code.

If I don't use the Lombok annotation and just create a Logger by myself, everything works perfectly

Describe the solution you'd like

It is desirable that Lombok annotations are included in the existing recipes (not just the one in the example above)

Have you considered any alternatives or workarounds?

There is no workaround possible that I'm aware of

@miguelchico miguelchico added the enhancement New feature or request label Oct 26, 2023
@timtebeek
Copy link
Contributor

Hi @migualchico; interesting case! In part this is down to OpenRewrite not yet supporting Lombok annotations; support for that is tracked in:

Because we don't yet support Lombok annotations, this conditional fails, as there's no such field found

Set<J.VariableDeclarations> loggers = FindFieldsOfType.find(clazz, framework.getLoggerType());
if (!loggers.isEmpty()) {

We can likely do two things to hope to resolve your immediate issue:

  1. generically support the lombok logging annotations by adding a synthetic logging field, that's available to recipes, but not modifiable or printed (high effort and complexity)
  2. specifically for this recipe, look for lombok logging annotations, and if found, generate a logging template that uses the default "log" field generated (for simplicity, not taking into account lombok.log.fieldName)

The second approach might be easier to tackle, starting with a draft pull request that merely adds a test to verify the desired behavior. Would you be willing to try your hand at that?

timtebeek added a commit that referenced this issue Oct 26, 2023
@timtebeek timtebeek self-assigned this Oct 26, 2023
timtebeek added a commit that referenced this issue Oct 27, 2023
* Add limited support for Lombok log annotations

Fixes #114

* Remove JetBrains NotNull annotation

* Also support `@Sf4j` in SystemErrToLogging

* Remove unused field from test

* Also cover SystemOutToLogging

* Harden mavenProjectDependenciesUpdated() with new releases
@timtebeek
Copy link
Contributor

timtebeek commented Oct 27, 2023

Thanks for reporting your findings here! This should be fixed in

You can try out this change through our snapshot versions, pending the next release.

Please let us know how this works for you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants