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

Canonical SLF4J Logger usage #632

Closed
2 of 3 tasks
rickie opened this issue May 17, 2023 · 4 comments · Fixed by #783
Closed
2 of 3 tasks

Canonical SLF4J Logger usage #632

rickie opened this issue May 17, 2023 · 4 comments · Fixed by #783
Assignees
Milestone

Comments

@rickie
Copy link
Member

rickie commented May 17, 2023

Problem

Internally we heavily use SLF4J for logging in our classes.
We would like to have a check to canonicalize the usage of such loggers.
For that reason we would like a check that looks at the following things:

  • Is the variable declared with the modifiers private static final?
  • Is the variable name of the logger LOG?
  • Is the correct class name passed to LoggerFactory#getLogger?

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.
  • Improve performance.

I would like to rewrite the following code:

final class SimpleExample {
  public Logger LOGGER = LoggerFactory.getLogger(WrongClass.class);
  ...
}

to:

final class SimpleExample {
  private static final Logger LOG = LoggerFactory.getLogger(SimpleExample.class);
  ...
}

Considerations

For interfaces we have to make an exception when it comes to the private static final, as in interfaces this is not an option.
As raised by @mlrprananta himself, we could parameterize the variable name of the logger. Perhaps we can add an option that people can override the allowed variable name of the logger via the ErrorProneFlags.

For this check we don't need to look at the line on which the logger is defined, we will do that in a different check.

Additional context

This project has some related checks KengoTODA/errorprone-slf4j.
However, we want a check that is specific to our internal standards. Additionlly, in EPS we have our own best practices that we want to use for the code of this check.

Later, we might want to add more features to this check. This would be a good first version 😄.

@Stephan202
Copy link
Member

I wonder whether we should default to a more permissible name pattern (e.g. LOG(GER)?), and then internally fix that to just LOG. This may ease external adoption. (Perhaps using CodeQL we can find which variants are most common in open source software 🤔.)

@benhalasi

This comment was marked as resolved.

@mlrprananta mlrprananta self-assigned this May 22, 2023
@rickie
Copy link
Member Author

rickie commented Jul 13, 2023

@mlrprananta Are you still planning on working on this issue? Otherwise it's no problem but I'll unassign you. I'm just curious 😄.

@mlrprananta mlrprananta removed their assignment Jul 13, 2023
@mlrprananta
Copy link
Contributor

Hey @rickie, I won't have much time to look into this coming weeks, but I'd like to take a look at this later again (I unassigned myself for now, if someone else wants to pick it up that's also fine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants