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

J2ee10 - remove_parm soap_element_factory recipes #303

Merged

Conversation

ranuradh
Copy link
Contributor

@ranuradh ranuradh commented Sep 25, 2023

What's changed?

image image ## What's your motivation?

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

@timtebeek @joanvr

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 IDEA auto-formatter on affected files

@ranuradh ranuradh self-assigned this Sep 26, 2023
@timtebeek timtebeek requested a review from joanvr September 26, 2023 13:58
joanvr
joanvr previously approved these changes Sep 28, 2023
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.

I'm seeing a failed test, missing a recipe called org.openrewrite.java.migrate.jakarta.RemoveNewInstance?

@joanvr joanvr self-requested a review September 28, 2023 15:13
@joanvr joanvr dismissed their stale review September 28, 2023 15:13

missing recipe

Copy link
Contributor

Choose a reason for hiding this comment

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

why we have this file? I've checked the contents and it's just a Test1 class that it's not being used and a stub for MethodExpression. Why don't we add the actual jakarta.el-api jar or add the stub in the test itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we can use the jakarta.el-api.jar instead

Comment on lines 60 to 62
void doY() {
String x = "test";
x.toUpperCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this extra stuff in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extra stuff was just to ensure nothing happened to it and I have used it as a negative test..

@joanvr
Copy link
Contributor

joanvr commented Sep 29, 2023

After a bit more of diving into the recipe:
SOAPElementFactory.newInstance() method has no direct replacement but probably should be replaced by SOAPFactory.newInstance() when you are replacing the whole type?
I'm not sure about this part of the recipe

@joanvr
Copy link
Contributor

joanvr commented Sep 29, 2023

Pushed some changes:

  • Improved format on tests
  • Removed unnecessary bits on tests
  • Removed multiple versions of jakarta.xml.soap-api
  • Added jakarta.el-api
  • Removed recipe that removes SOAPElementFactory.newInstance() -> Instead just let it be changed to SOAPFactory.newInstance()

Can you take a look at it and discuss about it?

@ranuradh
Copy link
Contributor Author

ranuradh commented Sep 29, 2023

I approve the slight test changes and adding the .jars. ,Regarding The static method SOAPElementFactory.newInstance() is removed without replacement. See the Jakarta EE 9.1 documentation. I thought we needed to remove it not replace it .Since the documentation doesn't ask us to replace it and our rule description asks us to remove without replacement. I guess we could replace it with the SOAPFactory.newInstance(). Just checked this would be fine..Could we merge your changes.Thanks!

@joanvr joanvr merged commit b373d1b into openrewrite:main Sep 29, 2023
@joanvr
Copy link
Contributor

joanvr commented Sep 29, 2023

Merged! Thank you very much for your time and contributions @ranuradh :)

@ranuradh
Copy link
Contributor Author

Thanks a ton @joanvr!

@ranuradh ranuradh deleted the j2ee10_remove_parm_soapelementfactory branch October 2, 2023 15:30
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.

4 participants