PicnicSupermarket/error-prone-support

Canonical SLF4J Logger usage

rickie opened this issue ยท 4 comments

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 ๐Ÿ˜„.

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 ๐Ÿค”.)

In the issue description:

  • Is the check declared with the modifiers private static final?

Do you mean "Is the variable declared"?

rickie commented

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

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)