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

Bugfix/multiple curly braces in one verification block #603

Conversation

yurii-yu
Copy link
Contributor

What's changed?

This is a change similar to the one I made before: Pull Request 596

Cope with a corner case in refactoring JMockit Verifications.
Sometimes programmers will use more than one curly braces in one Verification block.
This is bizarre and confusing, because it looks like there are multiple "anonymous class" here while in fact these curly braces do not make any effective difference. It will be ideal if such code can also be refactored automatically.

Usually people write code like this:

new Verifications() {{
    myObject.wait();
    myObject.wait(anyLong, anyInt);
    myObject.wait(anyLong);
    times = 2;
}};

while some programmers prefer grouping related statements with redundant curly braces

new Verifications() {
    {
    myObject.wait();
    myObject.wait(anyLong, anyInt);
    }
    {
    myObject.wait(anyLong);
    times = 2;
    }
};

Such code snippet can not be refactored automatically.

What's your motivation?

We encountered problem in dealing with several legacy projects.
Some codes can not be refactored automatically, while manually changing them seems tedious, since they are not complex at all. We located and solved this problem, which can save us quite a lot of time.
Benefited a lot from the OpenRewrite project previously, we believe that more people and more legacy projects should be able to take advantage of it.

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

This change is similar to the one I made for "multiple blocks in one Expectations block", I only modified the IF statement to cope with not only Expectation block but also Verifications block. Moreover, I adhere to the convention of the changes made by @timtebeek for my last pull request.
I read the best practice so I kept the LST untouched.
The change has been tested by the entire test suite. As far as I can see, everything works fine.
Please note that the new code might need to be formatted to comply with the existing convention.

Anyone you would like to review specifically?

@timtebeek I would be very happy if you can review this, it is an easy fix.

Have you considered any alternatives or workarounds?

Maybe changing the LST is a better solution, but I do not think it is worthy since this is not a common case.
Users can also use a script to remove the curly braces before applying this recipe, but that is an invasive way.

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

Copy link
Contributor

@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 this reuse; thanks for the fix again!

@timtebeek timtebeek merged commit a8e9320 into openrewrite:main Sep 16, 2024
2 checks passed
@yurii-yu
Copy link
Contributor Author

@timtebeek It's so nice working with you, since the experience is wonderful.

To me, there are still some mysterious problems when applying OpenRewrite to our legacy project. For example, in the verification block, anyLong can be refactored to anyLong() correctly, whereas this.anyLong (anyLong is a member of the Verifications class)can not, because the former is granted as a J$Identifier while the latter is granted as a J$FieldAccess.
We used a bash script to remove all this. which cause problems. But I guess it would be ideal if other users can apply OpenRewrite in such scenario without any extra work.
I am still looking into this problem but I can not say how long it takes. I will come back to you if I (hopefully) solve this problem.
Regards

@timtebeek
Copy link
Contributor

Thanks for the kind words and continued improvements @yurii-yu ! It's great to see how quickly you've taken to adding improvements here.

For the J$Identifier vs J$FieldAccess it might be best to start a draft PR with a unit test, and then step through with the debugger & TreeVisitingPrinter to inspect and handle each individual case. I hope that helps!

@yurii-yu
Copy link
Contributor Author

Yes, thank you for this insightful suggestion. That's exactly what I am thinking about.
I guess the problem lies not in rewriting-testing-framework but in the rewrite project. I have some initial clue, but I still need more time since the issue is much complicated than I thought before because there are numerous scenarios concerning this., I must be cautious. And, after all, this is a "non-official project" for me.
I swear "I will be back". You have my words :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants