Fix Spotless targetExclude patterns being overridden#1690
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview🐛 Fixes
|
| "build/generated/java", | ||
| "src/generated/java", | ||
| "src/main/generated_testsrc", | ||
| "generated_testSrc", | ||
| "build/groovy-dsl-plugins/output", |
There was a problem hiding this comment.
can we also test some edge cases which should be included, like src/foo/generated
| "**/build/generated*/**", | ||
| "**/src/generated*/**", | ||
| "**/generated_*src/**", | ||
| "**/generated_*Src/**", | ||
| // build/groovy-dsl-plugins contains Java wrapper classes for | ||
| // https://docs.gradle.org/9.3.1/userguide/implementing_gradle_plugins_convention.html | ||
| // and don't need to be formatted. | ||
| "**/groovy-dsl-plugins/**"); |
There was a problem hiding this comment.
I'll open a PR to suppressible-error-prone aligning with these.
There was a problem hiding this comment.
they should be aligned already, no ? https://github.com/palantir/suppressible-error-prone/blob/0f2c42970e20b98d1b569e15ca9e86afc1c52a66/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java#L225
except for the groovy-dsl-plugins addition - which I am not really sure what it is and wether it is relevant to error prones
There was a problem hiding this comment.
yeah it seems to be the same.
we false positive on a few cases in both repos, but that is a separate change!
foo/generated_foo/blah/src/bar
foo/build/generatedbar/baz
foo/src/generatedxforms/bar
There was a problem hiding this comment.
from internal discussion, ^ these aren't false positives.
| "**/generated_*src/**", | ||
| "**/generated_*Src/**", |
There was a problem hiding this comment.
small correctness thing which might not matter: will this match generated_foo_src?
the current behavior matches SEP
There was a problem hiding this comment.
yes, it does - I used the same logic as in error prone: https://github.com/palantir/suppressible-error-prone/blob/develop/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java#L225.
There was a problem hiding this comment.
if that's the case, let's explicate this in a test case? to document this behavior + make it easy to include generated_foo_src for formatting in the future if we want to .
There was a problem hiding this comment.
I think line 54, 56 are explaining that we exclude generated source directories. This is where people usually set up these generated source sets.
| @Test | ||
| void format_checks_non_generated_files(GradleInvoker gradle, RootProject project) { |
There was a problem hiding this comment.
Lets make this test parameterized as well? It'd be nice to quickly grok what's included and excluded by glancing at the two tests.
Let's add a few non-obvious test cases? It'd be good to explicate this behavior for our future selves.
foo/generated_foo_src/bar
foo/generated_foo/blah/src/bar
foo/build/generatedbar/baz
foo/src/generatedxforms/bar
There was a problem hiding this comment.
added test cases here. Fysa - from the above list only the foo/generated_foo/blah/src/bar will be not-ignored
2 participants
Before this PR
#1568 was not additive, calling
targetExcludemultiple times was overriding previous values instead of accumulating them.After this PR
FLUP for #1568
SpotlessInteropcallingtargetExcludemultiple times, which overrides previous patterns instead of accumulating them - only the last exclude (**/groovy-dsl-plugins/**) was actually appliedSpotlessExcludesTestto verifyspotlessJavaCheckskips files in all excluded directories==COMMIT_MSG==
Fix Spotless targetExclude patterns being overridden
==COMMIT_MSG==
Possible downsides?