Skip to content

Commit

Permalink
Add extra checkstyle checks in an IDE (elastic#67650)
Browse files Browse the repository at this point in the history 
Now that Checkstyle can be made useful in an IDE, add extra rules only when
checking in the IDE so that a contributor is given extra help when editing
files, without having the checkstyle task spam the console when running gradle.

Apart from the `BooleanNegation` rule below, the new rules have the `warning`
severity level. The new Javadoc rules reflect the guidelines in
`CONTRIBUTING.md` that we've had for some time.

   * I upgraded Checkstyle, so now it interprets the config file in the same was
     as the IntelliJ plugin. That means that I could move the `LineLength` rule
     up a level and remove its special handling in the `:configureIdeCheckstyle`
     task
   * I added the `SuppressWarningsFilter` and `SuppressWarningsHolder` rules so
     that the `@SuppressWarnings` annotation can be used to silence Checkstyle
     checks. We shouldn't typically need this, but it seemed like a useful thing
     to configure. In contrast to the suppressions file, this approach makes the
     suppression obvious.
   * I changed the `:configureIdeCheckstyle` task to pull in rules from
     `checkstyle_ide_fragment.xml` and merged them into the generated config.
     The rules are as follows:
      * `BooleanNegation` - a task defined using `DescendantToken` that detects
        cases of `!` being used negate a boolean expression. This ought to be in
        the main config, but at present we have a number of violations in the
        source
      * `MissingJavadocMethod` - requires that public methods are documented. I
        set `minLineCount` to 2 so that the rule doesn't trigger on simple
        methods.
      * `MissingJavadocPackage` - require Javadoc in `package-info.java`
      * `MissingJavadocType` - require types to be documented
      * `JavadocMethod` - require params and return type to be documeted
  • Loading branch information
@pugnascotia
pugnascotia authored Jan 21, 2021
1 parent 33fccd0 commit 82fdec1
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 46 deletions.
71 changes: 40 additions & 31 deletions buildSrc/src/main/resources/checkstyle.xml
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
<property name="charset" value="UTF-8" />
Expand All @@ -10,12 +10,16 @@
<property name="file" value="${config_loc}/checkstyle_suppressions.xml" />
</module>

<!-- Checks Java files and forbids empty Javadoc comments -->
<module name="SuppressWarningsFilter" />

<!-- Checks Java files and forbids empty Javadoc comments. -->
<!-- Although you can use the "JavadocStyle" rule for this, it considers Javadoc -->
<!-- that only contains a "@return" line to be empty. -->
<module name="RegexpMultiline">
<property name="id" value="EmptyJavadoc"/>
<property name="format" value="\/\*[\s\*]*\*\/"/>
<property name="fileExtensions" value="java"/>
<property name="message" value="Empty javadoc comments are forbidden"/>
<property name="id" value="EmptyJavadoc" />
<property name="format" value="\/\*[\s\*]*\*\/" />
<property name="fileExtensions" value="java" />
<property name="message" value="Empty javadoc comments are forbidden" />
</module>

<!--
Expand All @@ -25,18 +29,21 @@
such snippets.
-->
<module name="org.elasticsearch.gradle.checkstyle.SnippetLengthCheck">
<property name="id" value="SnippetLength"/>
<property name="max" value="76"/>
<property name="id" value="SnippetLength" />
<property name="max" value="76" />
</module>

<!-- Its our official line length! See checkstyle_suppressions.xml for the files that don't pass this. For now we
suppress the check there but enforce it everywhere else. This prevents the list from getting longer even if it is
unfair. -->
<module name="LineLength">
<property name="max" value="140" />
<property name="ignorePattern" value="^ *\* *https?://[^ ]+$" />
</module>

<module name="TreeWalker">
<!-- Its our official line length! See checkstyle_suppressions.xml for the files that don't pass this. For now we
suppress the check there but enforce it everywhere else. This prevents the list from getting longer even if it is
unfair. -->
<module name="LineLength">
<property name="max" value="140"/>
<property name="ignorePattern" value="^ *\* *https?://[^ ]+$"/>
</module>
<!-- Make the @SuppressWarnings annotations available to Checkstyle -->
<module name="SuppressWarningsHolder" />

<module name="AvoidStarImport" />

Expand All @@ -45,15 +52,19 @@

<!-- Non-inner classes must be in files that match their names. -->
<module name="OuterTypeFilename" />

<!-- No line wraps inside of import and package statements. -->
<module name="NoLineWrap" />

<!-- only one statement per line should be allowed -->
<module name="OneStatementPerLine"/>
<module name="OneStatementPerLine" />

<!-- Each java file has only one outer class -->
<module name="OneTopLevelClass" />

<!-- The suffix L is preferred, because the letter l (ell) is often
hard to distinguish from the digit 1 (one). -->
<module name="UpperEll"/>
hard to distinguish from the digit 1 (one). -->
<module name="UpperEll" />

<module name="EqualsHashCode" />

Expand Down Expand Up @@ -82,7 +93,7 @@
<module name="RedundantModifier" />
<!-- Checks that all java files have a package declaration and that it
lines up with the directory structure. -->
<module name="PackageDeclaration"/>
<module name="PackageDeclaration" />

<!-- We don't use Java's builtin serialization and we suppress all warning
about it. The flip side of that coin is that we shouldn't _try_ to use
Expand All @@ -104,21 +115,19 @@

<!-- Forbid equality comparisons with `true` -->
<module name="DescendantToken">
<property name="tokens" value="EQUAL"/>
<property name="limitedTokens" value="LITERAL_TRUE"/>
<property name="maximumNumber" value="0"/>
<property name="maximumDepth" value="1"/>
<message key="descendant.token.max" value="Do not check for equality with 'true', since it is implied"/>
<property name="id" value="EqualityWithTrue" />
<property name="tokens" value="EQUAL" />
<property name="limitedTokens" value="LITERAL_TRUE" />
<property name="maximumNumber" value="0" />
<property name="maximumDepth" value="1" />
<message key="descendant.token.max" value="Do not check for equality with 'true', since it is implied" />
</module>

<!-- Forbid using '!' for logical negations in favour of checking
against 'false' explicitly. -->
<!-- This is disabled for now because there are many, many violations,
hence the rule is reporting at the "warning" severity. -->

<!-- Forbid using '!' for logical negations in favour of checking against 'false' explicitly. -->
<!-- This is disabled for now because there are many violations, hence the rule is reporting at the "warning" severity. -->
<!--
<module name="DescendantToken">
<property name="severity" value="warning"/>
<property name="id" value="BooleanNegation" />
<property name="tokens" value="EXPR"/>
<property name="limitedTokens" value="LNOT"/>
<property name="maximumNumber" value="0"/>
Expand Down
48 changes: 48 additions & 0 deletions buildSrc/src/main/resources/checkstyle_ide_fragment.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<!-- There are some rules that we only want to enable in an IDE. These -->
<!-- are extracted to a separate file, and merged into the IDE-specific -->
<!-- Checkstyle config by the `:configureIdeCheckstyle` task. -->

<module name="IdeFragment">
<!-- Forbid using '!' for logical negations in favour of checking against 'false' explicitly. -->
<!-- This is only reported in the IDE for now because there are many violations -->
<module name="DescendantToken">
<property name="id" value="BooleanNegation" />
<property name="tokens" value="EXPR"/>
<property name="limitedTokens" value="LNOT"/>
<property name="maximumNumber" value="0"/>
<property name="maximumDepth" value="1"/>
<message
key="descendant.token.max"
value="Do not negate boolean expressions with '!', but check explicitly with '== false' as it is more explicit"/>
</module>

<module name="MissingJavadocMethod">
<property name="severity" value="warning" />
<!-- Exclude short methods from this check - we don't want to have to document getters -->
<property name="minLineCount" value="2" />
<message key="javadoc.missing" value="Public methods should be documented" />
</module>

<module name="MissingJavadocPackage">
<property name="severity" value="warning"/>
<message
key="package.javadoc.missing"
value="A description and other related documentation for a package should be written up in the package-info.java" />
</module>

<module name="MissingJavadocType">
<property name="severity" value="warning"/>
<message key="javadoc.missing" value="Types should explain their purpose" />
</module>

<!-- Public methods must have JavaDoc -->
<module name="JavadocMethod">
<property name="severity" value="warning"/>
<property name="scope" value="public"/>
</module>
</module>
2 changes: 1 addition & 1 deletion buildSrc/version.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ lucene = 8.8.0-snapshot-737cb9c49b0
bundled_jdk_vendor = adoptopenjdk
bundled_jdk = 15.0.1+9

checkstyle = 8.20
checkstyle = 8.39

# optional dependencies
spatial4j = 0.7
Expand Down
41 changes: 27 additions & 14 deletions gradle/ide.gradle
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import org.elasticsearch.gradle.info.BuildParams
import org.jetbrains.gradle.ext.Remote
import org.jetbrains.gradle.ext.JUnit

import java.nio.file.Files
import java.nio.file.StandardCopyOption
import java.nio.file.Paths
import java.nio.file.StandardCopyOption

buildscript {
repositories {
Expand All @@ -29,9 +29,11 @@ tasks.register('configureIdeCheckstyle') {
description = 'Generated a suitable checkstyle config for IDEs'

String checkstyleConfig = 'buildSrc/src/main/resources/checkstyle.xml'
String checkstyleSuppressions = 'buildSrc/src/main/resources/checkstyle_suppressions.xml'
String checkstyleIdeFragment = 'buildSrc/src/main/resources/checkstyle_ide_fragment.xml'
String checkstyleIdeConfig = "$rootDir/checkstyle_ide.xml"

inputs.files(file(checkstyleConfig))
inputs.files(file(checkstyleConfig), file(checkstyleIdeFragment))
outputs.files(file(checkstyleIdeConfig))

doLast {
Expand All @@ -41,28 +43,33 @@ tasks.register('configureIdeCheckstyle') {
Paths.get(file(checkstyleIdeConfig).getPath()),
StandardCopyOption.REPLACE_EXISTING
)

// There are some rules that we only want to enable in an IDE. These
// are extracted to a separate file, and merged into the IDE-specific
// Checkstyle config.
Node xmlFragment = parseXml(checkstyleIdeFragment)

// Edit the copy so that IntelliJ can copy with it
modifyXml(checkstyleIdeConfig, { xml ->
// Remove this module since it is implemented with custom code
xml.module.findAll { it.'@name' == 'org.elasticsearch.gradle.checkstyle.SnippetLengthCheck' }.each { it.parent().remove(it) }

// Move the line length check because the IDE thinks it can't belong under a TreeWalker. Moving the
// configuration in the main file causes the command-line Checkstyle task to fail ¯\_(ツ)_/¯
// Add all the nodes from the fragment file
Node treeWalker = xml.module.find { it.'@name' == 'TreeWalker' }
Node lineLength = treeWalker.module.find { it.'@name' == 'LineLength' }
treeWalker.remove(lineLength)
xml.append(lineLength)
xmlFragment.module.each { treeWalker.append(it) }

// Change the checkstyle config to inline the path to the
// suppressions config. This removes a configuration step when using
// the checkstyle config in an IDE.
Node suppressions = xml.module.find { it.'@name' == 'SuppressionFilter' }
suppressions.property.findAll { it.'@name' == 'file' }.each { it.'@value' = "buildSrc/src/main/resources/checkstyle_suppressions.xml" }
suppressions.property.findAll { it.'@name' == 'file' }.each { it.'@value' = checkstyleSuppressions }
},
"<!DOCTYPE module PUBLIC\n" +
" \"-//Puppy Crawl//DTD Check Configuration 1.3//EN\"\n" +
" \"http://www.puppycrawl.com/dtds/configuration_1_3.dtd\">\n" +
"<!-- Generated automatically from ${checkstyleConfig} - do not edit this file directly. -->\n"
"<!-- Generated automatically from the following - do not edit this file directly. -->\n" +
"<!-- ${checkstyleConfig} -->\n" +
"<!-- ${checkstyleIdeFragment} -->\n"
)
}
}
Expand Down Expand Up @@ -172,12 +179,10 @@ if (System.getProperty('idea.active') == 'true') {
* but before the XML document, e.g. a doctype or comment
*/
void modifyXml(Object path, Action<? super Node> action, String preface = null) {
File xmlFile = project.file(path)
XmlParser xmlParser = new XmlParser(false, true, true)
xmlParser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false)
Node xml = xmlParser.parse(xmlFile)
Node xml = parseXml(path)
action.execute(xml)

File xmlFile = project.file(path)
xmlFile.withPrintWriter { writer ->
def printer = new XmlNodePrinter(writer)
printer.namespaceAware = true
Expand All @@ -190,3 +195,11 @@ void modifyXml(Object path, Action<? super Node> action, String preface = null)
printer.print(xml)
}
}

Node parseXml(Object path) {
File xmlFile = project.file(path)
XmlParser xmlParser = new XmlParser(false, true, true)
xmlParser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false)
Node xml = xmlParser.parse(xmlFile)
return xml
}

0 comments on commit 82fdec1

Please sign in to comment.