-
Notifications
You must be signed in to change notification settings - Fork 77
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
Make rewrite-maven-plugin search for Kotlin source files in src/*/java if src/*/kotlin does not exist #937
Make rewrite-maven-plugin search for Kotlin source files in src/*/java if src/*/kotlin does not exist #937
Conversation
which fails initially because 0 kotlin source files will be found in the non-existent src/main/kotlin
Thanks for the work so far @reisners ! Look forward to the fix. 🙏🏻 |
… for Kotlin sources in src/*/java instead
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.
Looking good so far already, although I only have my mobile here to review. @MBoegers would you mind having a look somewhere in the coming week?
Sure first thing tomorrow and hi @reisners |
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.
I left a few suggestions and questions that popped up in my mind while following your changes.
Your changes are fully functional, but I think we should add a little bit more coverage.
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
…otlinTestSourceDir (was kotlinSourceDir)
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.
Some suggestions could not be made:
- src/test/resources-its/org/openrewrite/maven/RewriteDryRunIT/multi_module_project/b/src/main/java/sample/EmptyBlockSample.java
- lines 3-3
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
…o two independent testcases
@reisners Do you have any addition in mind or can we propagate the PR from draft? |
I had to answer to one comment from @timtebeek, waiting for feedback. When that is resolved, we good to proceed. |
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.
Some suggestions could not be made:
- src/test/resources-its/org/openrewrite/maven/RewriteDryRunIT/multi_module_project/b/src/main/java/sample/EmptyBlockSample.java
- lines 3-3
Thanks both! Great to have these additional source locations covered |
Not sure if this is still relevant, but I just ran mvn install locally for
the PR branch. Not sure if that's what you meant me to check though 🤔
…On Tue, Jan 28, 2025 at 6:24 PM Tim te Beek ***@***.***> wrote:
Thanks both! Great to have these additional source locations covered
—
Reply to this email directly, view it on GitHub
<#937 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPKZTQW5DQIVDJSVWXTFLD2M64OPAVCNFSM6AAAAABVXR2OBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJZGYZDSMZYGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
What's changed?
Added an integration test case that verifies that a Kotlin source file under src/main/java was detected and modified by a recipe (Autoformat). This test case initially fails because the rewrite-maven-plugin does not detect Kotlin source files under src/main/java.
What's your motivation?
To demonstrate the issue described in #936
Anything in particular you'd like reviewers to focus on?
I will push the fix in a separate commit.
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Any additional context
Checklist