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

Remove try catch fail blocks should be selectively applied #382

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

AlekSimpson
Copy link
Contributor

What's changed?

The new try catch recipe was not handling cases where binary expressions were being passed into Assertions.fail correctly. This PR fixes that.

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)

@AlekSimpson AlekSimpson added the bug Something isn't working label Jul 17, 2023
@AlekSimpson AlekSimpson self-assigned this Jul 17, 2023
@timtebeek
Copy link
Contributor

@AlekSimpson As you can see in cb80173 and 6f2b19b I've swapped the logic around to only recognize and apply changes in known cases, rather than what we had previously where might inadvertently make changes by only returning early for known cases. This in my view improves the predictability of the recipe, such that we should be able to safely run it at scale. We now also cover some additional cases, and handle some cases differently, for instance by only retaining the left side of a binary expression, and covering fail(.., Throwable). Hope you agree with those changes, and will apply the same pattern of only making changes for known cases to future recipes.

@joanvr would you be willing to review for a second pair of eyes on my changes?

@timtebeek timtebeek requested a review from joanvr July 18, 2023 11:06
@timtebeek timtebeek merged commit de2555f into main Jul 19, 2023
1 check passed
@timtebeek timtebeek deleted the alek/RemoveTryCatchBlocksFix branch July 19, 2023 15:40
@timtebeek timtebeek changed the title Alek/remove try catch blocks fix Rremove try catch blocks fix Jul 19, 2023
@timtebeek timtebeek changed the title Rremove try catch blocks fix Rremove try catch fail blocks should be selectively applied Jul 19, 2023
@timtebeek timtebeek changed the title Rremove try catch fail blocks should be selectively applied Remove try catch fail blocks should be selectively applied Jul 19, 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.

None yet

2 participants