-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Configuration for Kotlin's All-Open Plugin for JPA Entities #1576
Add Configuration for Kotlin's All-Open Plugin for JPA Entities #1576
Conversation
- Rename methods - Simplify allOpen.invoke
- Add Configure kotlin + allOpen
- Add author tag of JavaDoc
I apologize for not thoroughly checking the code before opening this PR. I'll tag you once everything is resolved. Feel free to close the PR, and I'll open a new one after the fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @YangSiJun528,
thanks for the PR. I've left some comments.
if (this.buildMetadataResolver.hasGroupId(build, "jakarta.persistence")) { | ||
customizeAllOpenWithJakarta(build); | ||
} | ||
else if (this.buildMetadataResolver.hasGroupId(build, "javax.persistence")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to support JavaEE anymore. Our baseline is Spring Boot 3.2, which already uses JakartaEE.
if (this.buildMetadataResolver.hasGroupId(build, "jakarta.persistence")) { | ||
customizeAllOpenWithJakarta(configuration); | ||
} | ||
else if (this.buildMetadataResolver.hasGroupId(build, "javax.persistence")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to support JavaEE anymore.
} | ||
|
||
@Test | ||
void customizeWhenJavaxPersistencePresentShouldCustomizeAllOpenWithJavax() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to support JavaEE anymore.
@@ -31,12 +31,6 @@ | |||
|
|||
import static org.assertj.core.api.Assertions.assertThat; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this?
} | ||
|
||
@Test | ||
void customizeWhenJavaxPersistencePresentShouldCustomizeAllOpenWithJavax() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to support JavaEE anymore.
I have fixed the bug and incorporated your feedback, @mhalbritter :
|
Thanks @YangSiJun528 ! |
Currently, theKotlinJpaGradleBuildCustomizer
class is responsible for configuring eitherjavax
orjakarta
dependencies. I believe a cleaner approach would be to split this functionality into two separate classes:KotlinJavaxJpaGradleBuildCustomizer
KotlinJakartaJpaGradleBuildCustomizer
I'm unsure whether these new classes should inherit fromKotlinJpaGradleBuildCustomizer
or be designed as independent classes that implementBuildCustomizer
. (similar toKotlinJpaMavenBuildCustomizer
)I would appreciate feedback on this approach.The strikethrough was used because JavaEE support is no longer needed. This makes the previous concerns irrelevant.
Fixes gh-1572