Skip to content

[java] New rule: LiteralsFirstInComparisons#2478

Merged
adangel merged 3 commits into
pmd:masterfrom
John-Teng:issue_2145
May 18, 2020
Merged

[java] New rule: LiteralsFirstInComparisons#2478
adangel merged 3 commits into
pmd:masterfrom
John-Teng:issue_2145

Conversation

@John-Teng

@John-Teng John-Teng commented May 12, 2020 •
edited by adangel
Loading

Copy link
Copy Markdown
Contributor

Describe the PR

Adds a new rule LiteralsFirstInComparisons that examines the positioning of literals for all String comparison operations. This rule's logic is identical to AbstractPositionLiteralsFirstInComparisons with the exception of the AbstractNode.image check.
The following comparisons are supported:

  • .equals
  • .equalsIgnoreCase
  • .compareTo
  • .compareToIgnoreCase
  • .contentEquals

This is a generalization of the previously defined rules PositionLiteralsFirstInComparisons and PositionLiteralsFirstInCaseInsensitiveComparisons. These now redundant rules will be deprecated in a separate PR.

Included all the original test cases for the original rules, as well as test cases for the new comparisons as well.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

@John-Teng John-Teng mentioned this pull request May 12, 2020
Closed
@ghost

ghost commented May 12, 2020 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 274 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 2 errors and 2 configuration errors.
Full report
✅ This changeset introduces 274 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 2 errors and 2 configuration errors.
Full report

Generated by 🚫 Danger

@oowekyala oowekyala changed the title Issue 2145 [java] New rule: LiteralsFirstInComparisons May 13, 2020
@oowekyala oowekyala added the a:new-rule Proposal to add a new built-in rule label May 13, 2020
@adangel adangel added this to the 6.24.0 milestone May 14, 2020
adangel
adangel reviewed May 14, 2020
View reviewed changes

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, looks promising!
Can you fix the two little things, I mentioned?

I've removed the "fix #2145" mention, since this PR doesn't fix this issue. I guess, we can rename the issue as "Deprecate PositionLiteralsFirstIn(CaseInsensitive)Comparisons in favor of LiteralsFirstInComparison".

Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread ...esources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml Outdated
@John-Teng

John-Teng commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @adangel for the review! I fixed the two things, and I'll begin working on deprecating the old rules on a separate PR.

@John-Teng John-Teng requested a review from adangel May 15, 2020 15:39
@adangel adangel self-assigned this May 18, 2020
adangel added a commit that referenced this pull request May 18, 2020
@adangel
[doc] Update release notes, refs #2478
7138d2e
adangel added a commit that referenced this pull request May 18, 2020
@adangel
Merge branch 'pr-2478'
3449c9a 
[java] New rule: LiteralsFirstInComparisons #2478
@adangel adangel merged commit 72128e1 into pmd:master May 18, 2020
@John-Teng John-Teng mentioned this pull request May 18, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel approved these changes

Assignees

@adangel adangel

Labels

a:new-rule Proposal to add a new built-in rule

Projects

None yet

Milestone

6.24.0

Development

Successfully merging this pull request may close these issues.

3 participants

@John-Teng @adangel @oowekyala