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

Recipe IllegalArgumentExceptionToAlreadyConnectedException #615

Merged

Conversation

BhavanaPidapa
Copy link
Contributor

What's changed?

This PR contains recipe - org.openrewrite.java.migrate.MethodExceptionReplacerRecipe
Rules:
image

image

What's your motivation?

A generic custom recipe for exception replacement based on method signatures. A common recipe is written to handle to both the rules.

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

image
For this rule already a custom recipe is written but now making use of this generic recipe instead of the previous one.

Anyone you would like to review specifically?

@timtebeek @cjobinabo

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

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek added the recipe Recipe requested label Dec 3, 2024
@timtebeek timtebeek self-requested a review December 6, 2024 13:34
@timtebeek timtebeek changed the title Generic Recipe to exception replacement based on method signature Recipe IllegalArgumentExceptionToAlreadyConnectedException Dec 6, 2024
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.

Thanks so far @BhavanaPidapa ! I've pushed up a small change for a missing import after the migration; figured that'd help ensure the code compiles after the recipe runs.

In the PR description you also mention Class.getAnnotation, but I do not see that mentioned in the recipe or tests; is that still to come?

@BhavanaPidapa
Copy link
Contributor Author

It is already covered in #613

@timtebeek
Copy link
Contributor

Ok; good to merge then. I know @cjobinabo is out; let me know if you have any concerns or difficulties merging.

@BhavanaPidapa
Copy link
Contributor Author

Ok; good to merge then. I know @cjobinabo is out; let me know if you have any concerns or difficulties merging.

Hey Tim, I don't have access to merge it seems. Can you please provide me the access.

@timtebeek timtebeek merged commit f43c8ce into openrewrite:main Dec 9, 2024
3 checks passed
@timtebeek
Copy link
Contributor

Thanks @BhavanaPidapa ; I've added you to the team, and merged the PR myself for now. :)

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

Successfully merging this pull request may close these issues.

2 participants