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

Use slf4j over old logging APIs in Maven #337

Closed
digulla opened this issue Jun 4, 2019 · 3 comments
Closed

Use slf4j over old logging APIs in Maven #337

digulla opened this issue Jun 4, 2019 · 3 comments
Milestone

Comments

@digulla
Copy link

digulla commented Jun 4, 2019

Change the code to use slf4j loggers everywhere.

Reasons:

  • We have Mojo code which uses the Mojo logger
  • We have other code which uses a Plexus logger
  • We have code used from both places which would benefit from logging and jumps to hoops to make it happen.

If we used the underlying slf4j logging framework directly, every part of the code could use the same logger type and everyone could ask for a logger (right now, you have to have it injected via Plexus or call getLog() in the Mojo), eliminating the need to get loggers and then pass them around.

Changes: Get a logger using this code at the top of a class which needs logging:

private final Logger LOG = LoggerFactory.getLogger( getClass() );

or using

private static final Logger LOG = LoggerFactory.getLogger( TypeName.class );

Note: The latter is slightly faster but if you forget to replace the type name, the logger will have the wrong name.

Additional advantage: If you enable logger names in the log output, you can find places where some message was logged much more quickly.

@digulla
Copy link
Author

digulla commented Jun 4, 2019

Necessary to fix #201
Without this, some methods will have too many parameters.

@digulla
Copy link
Author

digulla commented Jun 5, 2019

Please let me know if you'd accept a PR for this.

@ppalaga
Copy link
Contributor

ppalaga commented Jun 19, 2019

Sorry for the delay, I was travelling and had PTO in recent weeks.

Thanks for the proposal and for being ready to contribute.

I agree with the idea and I prefer the private static final variant.

Please use the place holders (log.info("Processed {} resources", resourcesCount)) rather than string concatenation (log.info("Processed "+ resourcesCount +" resources"))

The only issue I can think of related to this change is that this will potentially change the logger names for the existing log messages. I do not think it should prevent us from doing this. In the end, I'll bump the major with the next release for other reasons.

ppalaga pushed a commit to ppalaga/mojohaus-license-maven-plugin that referenced this issue Jun 22, 2019
ppalaga added a commit to ppalaga/mojohaus-license-maven-plugin that referenced this issue Jun 22, 2019
ppalaga added a commit to ppalaga/mojohaus-license-maven-plugin that referenced this issue Jun 22, 2019
ppalaga added a commit to ppalaga/mojohaus-license-maven-plugin that referenced this issue Jun 22, 2019
ppalaga added a commit to ppalaga/mojohaus-license-maven-plugin that referenced this issue Jun 22, 2019
ppalaga added a commit that referenced this issue Jun 22, 2019
Fix #337 Use slf4j over old logging APIs in Maven
@ppalaga ppalaga added this to the 2.0.0 milestone Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants