Skip to content

Mergeable if statements should be combined - RSPEC-S1066#734

Open
olwispe wants to merge 13 commits intoopenrewrite:mainfrom
olwispe:combine-mergeable-if-statements-rspec-s1066
Open

Mergeable if statements should be combined - RSPEC-S1066#734
olwispe wants to merge 13 commits intoopenrewrite:mainfrom
olwispe:combine-mergeable-if-statements-rspec-s1066

Conversation

@olwispe
Copy link
Contributor

@olwispe olwispe commented Sep 8, 2025

What's changed?

This PR implements RSPEC-S1066 which was requested in :

What's your motivation?

Among others, help migrate to pattern matching for instanceof when combined with org.openrewrite.staticanalysis.InstanceOfPatternMatch :

if (o instanceof String) {
  String s = (String) o;
  if (s.isEmpty()) {
     System.out.println("OK");
  }
}
if (o instanceof String s && s.isEmpty()) {
  System.out.println("OK");
}

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Sep 8, 2025
Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great to see @olwispe ! Added some light polish and I've been looking at the changes at scale.

Looking at these two examples I wonder if perhaps we should move the inner if conditional to a new line, and keep the comments in more of their original position.

image image image

I've pushed a quick test update that shows a comment lost. What are your thoughts to the above proposal?

@olwispe
Copy link
Contributor Author

olwispe commented Sep 8, 2025

Thanks for your comments.
I tend to prefer to have each merged conditional on a new line as you suggest and therefore keep the comments in their original position.

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in two weeks. PRs may be reopened when there is renewed interest.

@github-actions github-actions bot added the Stale label Jan 26, 2026
@timtebeek timtebeek removed the Stale label Jan 27, 2026
@olwispe
Copy link
Contributor Author

olwispe commented Jan 29, 2026

Hi @timtebeek!
I've implemented your earlier suggestions which were quite tricky to implement given my knowledge of OpenRewrite.
Anyway, could you review these changes?

@olwispe olwispe force-pushed the combine-mergeable-if-statements-rspec-s1066 branch from 2018f25 to f0c659e Compare February 3, 2026 18:32

@Getter
final String displayName = "Mergeable `if` statements should be combined";
@Getter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Getter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants