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

RemoveUnusedLocalVariables fails inside case blocks (test) #153

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

sfc-gh-dtran
Copy link
Contributor

@sfc-gh-dtran sfc-gh-dtran commented Aug 10, 2023

What's changed?

What's your motivation?

Fixes #152

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

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek
Copy link
Contributor

Appreciate you adding this test! There's a bit of an oddity where you also remove two other tests. Would you mind reverting that?

@timtebeek timtebeek added the bug Something isn't working label Aug 11, 2023
@timtebeek timtebeek changed the title Add failing test for 152 RemoveUnusedLocalVariables fails inside case blocks (test) Aug 11, 2023
@sfc-gh-dtran
Copy link
Contributor Author

Appreciate you adding this test! There's a bit of an oddity where you also remove two other tests. Would you mind reverting that?

Thanks, that was weird indeed. I re-added those missing tests.

@timtebeek
Copy link
Contributor

Thanks! I've ran your test and briefly explored the options; the quickest thing we can do now to avoid making unwanted changes, is to simply skip unused variables defined in case statements. That's a bit unfortunate, but better than making the wrong change right now. We can then later look to also remove unused local variables inside case statements. Hope you agree!

@timtebeek timtebeek merged commit b59e2c7 into openrewrite:main Aug 16, 2023
@sfc-gh-dtran
Copy link
Contributor Author

Thanks! I've ran your test and briefly explored the options; the quickest thing we can do now to avoid making unwanted changes, is to simply skip unused variables defined in case statements. That's a bit unfortunate, but better than making the wrong change right now. We can then later look to also remove unused local variables inside case statements. Hope you agree!

Thanks! I've actually already looked into this and it looks like the problem is with the DeleteStatement recipe not handling statements inside case blocks: adding a simple visitCaseBlock visitor fixed this; I can make a PR to rewrite so you can take a look at it

@timtebeek
Copy link
Contributor

Ah yes please! A PR with a proper fix would be much appreciated; I figured to the least harm quickly, but proper handling is even better!

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.

RemoveUnusedLocalVariables fails inside case blocks
2 participants