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

[java] Rename rule InvalidSlf4jLoggingFormat to InvalidLogMessageFormat #2054

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

adangel
Copy link
Member

@adangel adangel commented Oct 5, 2019

Follow-up on #2012 / #336

@pmd-test
Copy link

pmd-test commented Oct 5, 2019

1 Message
📖 This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel changed the title [java] Rename invalid slf4j logging format [java] Rename rule InvalidSlf4jLoggingFormat to InvalidLogMessageFormat Oct 5, 2019
@adangel adangel added this to the 6.19.0 milestone Oct 25, 2019
@jsotuyod
Copy link
Member

Should we do it this way through a deprecation? or simply rename it in the XML and use the RuleSetFactoryCompatibility to rewrite references?

@adangel
Copy link
Member Author

adangel commented Oct 26, 2019

Should we do it this way through a deprecation? or simply rename it in the XML and use the RuleSetFactoryCompatibility to rewrite references?

I'd prefer to do it via deprecation. The ruleset factory compatibility is currently handling only /rulesets/... and not the categories. I was going to say, that the deprecation method has the advantage, that we log it - but I see, we do this for the compatibility filters as well. So, the user sees in any case, that his ruleset might not work with a later version of PMD.

Looking at the old release notes, we didn't rename a single rule since we switched to categories. We only deprecated some rules, that we are going to replace with other rules.

I'd still prefer the deprecation mechanism over the rewriting of the ruleset, because

I'll double check, that we really get the deprecation warning and the documentation:

The documentation looks like this:
grafik
This is automatically generated by just tagging the rule as deprecated and reference the new rule.

And it is getting logged on the CLI:

Oct 26, 2019 10:16:26 AM net.sourceforge.pmd.RuleSetFactory parseRuleReferenceNode
WARNING: Use Rule name category/java/errorprone.xml/InvalidLogMessageFormat instead of the deprecated Rule name category/java/errorprone.xml/InvalidSlf4jMessageFormat. PMD 7.0.0 will remove support for this deprecated Rule name usage.
Oct 26, 2019 10:16:26 AM net.sourceforge.pmd.RuleSetFactory parseRuleReferenceNode
WARNING: Use Rule name category/java/errorprone.xml/InvalidLogMessageFormat instead of the deprecated Rule name category/java/errorprone.xml/InvalidSlf4jMessageFormat. PMD 7.0.0 will remove support for this deprecated Rule name usage.

for some reason, it is logged twice...
This is being fixed via #2086 ✔️

@adangel
Copy link
Member Author

adangel commented Oct 26, 2019

With the logging, there seem to be more issues:

  • deprecation warning is logged twice, although the rule is referenced only once
  • if the complete category ruleset is referenced, the deprecation warning is logged once - but I think, in that case, we should not consider renamed rules and ignore such deprecated rules
  • if the complete category ruleset is referenced and the deprecated rule is excluded, we still log the deprecation warning. this looks totally wrong.

This logging is somehow related to #2059, which is just a special case.
Fixing these issues is outside of the scope of this PR -> fixed via #2086 ✔️

@adangel adangel merged commit c7f1435 into pmd:master Oct 30, 2019
@adangel adangel deleted the rename-invalid-slf4j-logging-format branch October 30, 2019 19:33
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.

3 participants