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

Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals #448

Conversation

pankratz76
Copy link
Contributor

@pankratz76 pankratz76 commented Jan 23, 2025

@pankratz76 pankratz76 force-pushed the Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals branch from 1f33b73 to b38ff0a Compare January 23, 2025 21:08
@pankratz76
Copy link
Contributor Author

I need to fix my setup to get this running. It should be almost there, right? I think it's better to deliver something now rather than waiting longer. Feel free to take over and provide early feedback if this seems like the right direction. Thanks

Vincent Potucek added 4 commits January 25, 2025 17:26
…ns-for-compareTo-and-contentEquals' into Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals
Copy link
Contributor Author

@pankratz76 pankratz76 left a comment

Choose a reason for hiding this comment

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

feel free to takeover and give feedback, thx.

@pankratz76 pankratz76 requested a review from MBoegers February 7, 2025 08:11
@pankratz76 pankratz76 marked this pull request as ready for review February 7, 2025 08:11
@pankratz76 pankratz76 changed the title WIP: Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Feb 7, 2025
@pankratz76 pankratz76 changed the title Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Feb 7, 2025
@pankratz76 pankratz76 changed the title Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals Feb 7, 2025
public class A {
{
String s = null;
if(s.compareTo("test")) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should add & then invert any comparison with < 0 here for instance, as indicated in #362 (comment). As it stands I think we're using an int in an if. 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to keep the flip for compareTo, we have to also migrate the comparison. Otherwise, we would also flip the semantic, see #442.

Copy link
Contributor

@MBoegers MBoegers Feb 10, 2025

Choose a reason for hiding this comment

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

ATM, the implementation exclude the flip of < and > needed for compareTo, this can be done later.

@pankratz76 pankratz76 force-pushed the Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals branch from e493fd6 to decf84b Compare February 7, 2025 10:10
@timtebeek timtebeek marked this pull request as draft February 7, 2025 11:44
Vincent Potucek added 2 commits February 9, 2025 13:14
…compareTo-and-contentEquals

# Conflicts:
#	src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

@pankratz76 pankratz76 force-pushed the Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals branch from aa6fce0 to 0ae11da Compare February 9, 2025 13:42
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

@pankratz76
Copy link
Contributor Author

deepseek managed to inline while chatGPT failed ^^

@pankratz76 pankratz76 marked this pull request as ready for review February 9, 2025 14:24
@pankratz76 pankratz76 requested a review from timtebeek February 9, 2025 14:24
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

Vincent Potucek added 2 commits February 9, 2025 16:13
This reverts commit 74cd190.
@pankratz76 pankratz76 force-pushed the Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals branch from cdf95f7 to 46f143b Compare February 9, 2025 15:15
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

@timtebeek
Copy link
Contributor

Thanks for the work here @punkratz312 ; The automated suggestions are coming in because there's still differences seen after running the following against your branch: https://docs.openrewrite.org/recipes/recipes/rewrite/openrewriterecipebestpractices
Would you mind applying those changes?

@pankratz76
Copy link
Contributor Author

of course my friend. done and dusty

@pankratz76
Copy link
Contributor Author

pankratz76 commented Feb 9, 2025

@timtebeek the auto correction does not work. i applied it once before and wonder why. its. not working. see the. ci. My commit is green. The github action is failing.

checking each commit its failing on compile lvl.

image

@pankratz76 pankratz76 force-pushed the Fix-PMD-rule-LiteralsFirstInComparisons-for-compareTo-and-contentEquals branch from a65c948 to 46f143b Compare February 9, 2025 19:50
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20
  • src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java
    • lines 25-26

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20
  • src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java
    • lines 25-26

MBoegers and others added 2 commits February 10, 2025 16:10
…java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/EmptyBlock.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FallThrough.java
    • lines 19-19
  • src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java
    • lines 20-20
  • src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java
    • lines 25-26

Copy link
Contributor

@MBoegers MBoegers left a comment

Choose a reason for hiding this comment

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

I removed some test that were already covered and merged the two visitors.
Great work!

@MBoegers MBoegers merged commit e166e5a into openrewrite:main Feb 11, 2025
2 checks passed
@MBoegers MBoegers self-assigned this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants