rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

Inconsistent insertion order for addMethodDeclaration fixed

Open Riyazul555 opened this issue 1 year ago • 3 comments

What's changed?

In the previous code, the addMethodDeclaration method had a comparator that compared instances of Statement using UUIDs when they weren't instances of J.MethodDeclaration. This fallback to UUID comparison could lead to inconsistent placement of MethodDeclaration instances.

Here are the key changes made in the modified code:

Refined addMethodDeclaration Method:

Previously, the comparator used a fallback to UUID comparison for non-MethodDeclaration statements. This was changed to explicitly compare J.MethodDeclaration instances using the provided idealOrdering comparator. This ensures that new MethodDeclaration instances are consistently placed according to the ordering defined by idealOrdering, improving predictability. No Other Structural Changes:

The structure and organization of the classes and methods remain the same as in the previous code. The rest of the code continues to provide coordinate builders (JavaCoordinates) for various Java syntax elements (Statement, Expression, Annotation, etc.). These changes focus on enhancing the reliability and consistency of placing MethodDeclaration instances within Java code constructs, addressing the issue you encountered with intermittent test failures due to placement inconsistencies.

What's your motivation?

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

  • Fixes #4203

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

Riyazul555 avatar Jun 29 '24 11:06 Riyazul555

@timtebeek please checkout this PR If any changes are required do ping me Thanks

Riyazul555 avatar Jun 29 '24 11:06 Riyazul555

Hi @Riyazul555 ; it looks like you've converted the ternary operator into an if statement, without changing the logic. I think it might be best to add a unit test to guarantee that we make the correct change, as I don't expect any improvement yet.

timtebeek avatar Jun 29 '24 11:06 timtebeek

Ya I was just testing in another way Ok I will add it Thanks

Riyazul555 avatar Jun 29 '24 11:06 Riyazul555

Let's close this PR until such a test is added, just to make it clear this is not yet fixed. We can reopen the PR when a tests is added to the branch.

timtebeek avatar Jul 08 '24 14:07 timtebeek