Adds support for EnableExternalDTDLoad property to Checkstyle Plugin #22036
Conversation
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
# Conflicts: # subprojects/docs/src/docs/release/notes.md
- The base class tests depend upon CC's eager resolution failing due to a lack of repositories definied. So if we define them in the setup method, those builds succeed, when expected to fail, thus failing tests. - Need to specific repo in every new test then.
| @@ -112,7 +112,7 @@ The `init` task now adds compile-time Maven dependencies to Gradle's `api` confi | |||
| when converting a Maven project. This sharply reduces the number of compilation errors. | |||
| For more information about Maven conversions, see the [Build Init Plugin](userguide/build_init_plugin.html#sec:pom_maven_conversion). | |||
|
|
|||
| #### Introduced network timeout configuration for wrapper download | |||
| #### Introduced network timeout configuration for wrapper download | |||
There was a problem hiding this comment.
I can restore the trailing spaces at the ends of lines, but why do we want them? Isn't not having them a good practice? I have a script I automatically run upon commit to make sure I don't allow any of these in the files I've changed.
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Show resolved
Hide resolved
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
| if (getEnableExternalDtdLoad().isPresent()) { | ||
| spec.getForkOptions().getSystemProperties().put("checkstyle.enableExternalDtdLoad", getEnableExternalDtdLoad().get()); | ||
| } |
There was a problem hiding this comment.
I would do this slightly differently. Instead of checking if the property is present, let's just put a convention(false) on enableExternalDtdLoad in the constructor. I would then also drop the @Optional annotation below.
That way if someone uses the Checkstyle task without the Checkstyle plugin, they'll have a value. This is similar to what we're doing with the toolchain.
This is against the usual pattern (conventions should be set in the plugin), but as long as we support Checkstyle tasks to be used outside of the plugin, we have to keep them working.
There was a problem hiding this comment.
I was trying to avoid setting the convention in the constructor, and also to avoid needlessly setting an extra system property set to false (which means the same as absence) when running the checkstyle process.
Are you sure we shouldn't leave the @Optional and use getOrNull() to match the other fork options (these do NOT create system props, but still)?
subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java
Outdated
Show resolved
Hide resolved
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
...Test/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
… task constructor; always set the system property when forking the checkstyle process
Co-authored-by: Sterling Greene <big-guy@users.noreply.github.com>
…lugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy Co-authored-by: Sterling Greene <big-guy@users.noreply.github.com>
|
@bot-gradle test this |
|
OK, I've already triggered the following builds for you: |
…ystem-property-npe * origin/release: (124 commits) CompileAllProduction -> CompileAll Support skipping all dependency builds Disable stacktrace check for MavenPublishS3ErrorsIntegrationTest Refline performance.baselines paramater in performance test trigger Set docsTest timeout to 45 minutes Restore TAPI compatibility with Java 6 and old Gradle versions Put back some laziness Improve code Fix test Rename "implementationClass" to "resolverClass" Rename "repositories" to "javaRepositories" Rename "toUri()" to "resolve()" Solve remaining TODOs Reduce number of file changes for soak test on macOS Update documentation Update file system watching link Update file system watching link Update link to File System Watching Add link to file system watching Add file system watching page to toc tree ...
|
@bot-gradle test and merge |
|
OK, I've already triggered a build for you. |
Fixes #21624