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

Feature/337 slf4j logging #339

Closed
wants to merge 16 commits into from

Conversation

digulla
Copy link

@digulla digulla commented Jun 20, 2019

Notes:

  • I didn't remove all calls to getLog() since some are used to set the flag "verbose". I don't know whether Maven will propagate the log level to all slf4j loggers.
  • For the same reason, I left the "if (isVerbose())" before the log calls but removed most if (LOG.is...Level())
  • I kept the if (LOG.is...Level()) is it protected a big code block.
  • I replaced "..." + variable with "... {}", variable in all places I noticed. This should now be the default.
  • I replaced "... +" Arrays.asList(x) where x is an array with "{}", (Object)x. Without the case, the Java compiler would try to copy the array elements into the varargs parameter of slf4j with the result that only the first element of the array would be logged.

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @digulla , comments inline.

Aaron Digulla added 10 commits June 21, 2019 09:44
… same version is used in the whole dependency tree.

All versions should be here but I don't want to change too much.
Use missingFile over "missing file" to avoid confusion.
Got rid of verbose flag in getProjectsWithNoLicense()
…d problems, moved to our dependencyManagement element so everyone will use our preferred version.
@digulla
Copy link
Author

digulla commented Jun 21, 2019

The last two commits fix a deprecation and a warning I got in the POM. Since they are small changes, I didn't want to create a PR for them.

pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@ppalaga
Copy link
Contributor

ppalaga commented Jun 21, 2019

Summary: two last things open from my PoV:

  1. org.codehaus.mojo.license.logback.* removal

  2. <scope> in depManagement removal.

@digulla
Copy link
Author

digulla commented Jun 21, 2019

I'm now running out of time. I'll be at a conference next week, see you in July.

@ppalaga
Copy link
Contributor

ppalaga commented Jun 22, 2019

@digulla it is a pitty. This is not the first time that reviewing your large PR costs me time with no result. Please send PRs that have chance to get reviewed and merged quickly. Please discuss potentially controversial things like adding new dependencies upfront. To move forward with the next release, unless you veto within a reasonable time, I am going to make a new PR out of the acceptable parts of this PR.

ppalaga pushed a commit to ppalaga/mojohaus-license-maven-plugin that referenced this pull request Jun 22, 2019
@ppalaga
Copy link
Contributor

ppalaga commented Jun 22, 2019

Replaced by #342

@ppalaga ppalaga closed this Jun 22, 2019
ppalaga pushed a commit to ppalaga/mojohaus-license-maven-plugin that referenced this pull request 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants