fix: respect includes/excludes in maven resources#133
Conversation
| if (Files.exists(dir)) { | ||
| if (a.getIncludes().isEmpty() && a.getExcludes().isEmpty()) { | ||
| Some(dir) | ||
| List(dir) |
There was a problem hiding this comment.
Could you add a test that verifies the fix?
|
Hey @tgodzik, added a regression test This new test ensures that when no |
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
If you are using AI, please verify that everything is properly tested. It really doesn't help us much if we have to review one PR many times and would be easier if we vibe code it ourselves.
I don't want to be too harsh here, but I do have limited time at my disposal.
| @Test | ||
| def defaultResources() = { | ||
| check( | ||
| "basic_scala/pom.xml", |
There was a problem hiding this comment.
This is an old test, which doesn't contain any includes.
|
Hi @tgodzik, I understand your concern. Let me clarify why I used the basic_scala test: I reused basic_scala/pom.xml for the defaultResources() test because it's a minimal Scala project that doesn't have any or configuration in its resources section. This makes it perfect for testing the default behavior when no includes/excludes are specified, the entire resource directory should be added to the bloop config, not individual files. The issue_85 test specifically tests the opposite case: when ARE specified, individual files should be listed instead of the directory. Regarding AI usage: I did not use AI to write this code. I manually: Read through the existing codebase to understand how resources are processed Identified the bug in MojoImplementation.scala where the includes field was being ignored, created the issue_85 test case based on the issue description, added the defaultResources() test to ensure my fix didn't break the default behavior. I apologize if the code structure seemed suspicious. I'm happy to make any changes you'd like to see. |
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
I am sorry if I sounded harsh. It's fine to use AI, but it's best to first review the output and run the test manually. If you're not using it that's also fine, I just assumed so because your PRs are sometimes quite quick after creating an issue and the summary looks more descriptive than I would ever write manually.
| <build> | ||
| <resources> | ||
| <resource> | ||
| <directory>${project.basedir}/src/main/resources</directory> |
There was a problem hiding this comment.
Could we add a test that uses includes as well? Unless I am misremembering and there is such test already?
|
@tgodzik: No worries at all, I completely understand what you are trying to say. I have a lot of respect for the work you do maintaining bloop, metals and other repos, and I really appreciate the time you take to review my PRs and guide me. I'll definitely be more mindful about reviewing everything thoroughly and ensuring manual verification is solid :) Regarding the test case: I’ve just updated the PR with a new test for as requested. Let me know if that looks good! Thanks again |
2 participants
Description:
This PR addresses issue #85 where the
includesandexcludesfields in the pom.xmlresources configuration were being ignored. This resulted in the entire resource directory being added to the classpath even when specific files were explicitly requested (e.g.,
LICENSEorNOTICE), causing shadowing issues.Changes:
MojoImplementation.scala: Implemented
DirectoryScannerto correctly process resource directories based onincludesandexcludespatterns. Refactored resource processing to handleListof paths correctly.MavenConfigGenerationTest.scala: Added regression test
issue85to verify thatincludesare respected. Cleaned up unused imports.Verification:
Run
mvn test -Dtest=MavenConfigGenerationTest#issue85Validated that all existing tests pass.
Fixing issue:
Fixes #85
@tgodzik @adpi2