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

RemoveExtraSemicolons removes newline if semicolon is in front of statement #100

Conversation

pstreef
Copy link
Contributor

@pstreef pstreef commented May 9, 2023

Add test with @ExpectedToFail
#99

Add test with @ExpectedToFail
@timtebeek
Copy link
Contributor

Thanks for adding the replicating test already! I expect the fix is not far off either; likely prepend the prefix from the semicolon to the statement following the semicolon, if both are on the same line(?). Not expected of you to immediately fix that; but thought to note it here for when the pipeline succeeds to briefly explore before a merge.

@timtebeek timtebeek self-requested a review May 15, 2023 08:24
@timtebeek timtebeek added the bug Something isn't working label May 15, 2023
Remove empties on block level but keep prefix for next statements.
@pstreef
Copy link
Contributor Author

pstreef commented May 15, 2023

Something like this? 🤔

Comment on lines 177 to 191
int a = 1; //first we set a to 1
;a = 2;//then we set a to 2
a = 3;//then we set a to 3
;a = 4;;;;;//then we set a to 4
;a = 5;//then we set a to 5
a = 6;;//then we set a to 6
if (a == 6) { //if a is 6
;a = 7;;//then if a is 6 we set a to 7
}
;;
;//next we set a to 8
;a = 8;
return a;
;
//we are done!
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add in block comments /* foo; */ just to be sure,
but otherwise I really like the variations you've applied here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@timtebeek
Copy link
Contributor

Just merged main into this branch; we're still dependent on two branches in other repositories:
openrewrite/rewrite#3118 for PreCondition
openrewrite/rewrite-recipe-bom#16 for rewrite-analysis version

@timtebeek
Copy link
Contributor

Glad to see everything passing now that 8.0 has been merged! Didn't expect anything else, but still nice to see. Would you have time for a review @joanvr

@timtebeek timtebeek requested a review from joanvr May 31, 2023 12:35
Copy link
Contributor

@joanvr joanvr left a comment

Choose a reason for hiding this comment

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

Looks really good! Thanks for the fix and new tests! Just a minor unused import in the tests.

Co-authored-by: Joan Viladrosa <joan@moderne.io>
@joanvr joanvr merged commit d267c8f into openrewrite:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants