Skip to content
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.

Sign up for GitHub

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

Add extra checkstyle checks in an IDE #67650

Merged
merged 4 commits into from
Jan 21, 2021
Merged

Add extra checkstyle checks in an IDE #67650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cbda5f
Add extra checkstyle checks in an IDE
pugnascotia Jan 18, 2021
852567a
Customise messages
pugnascotia Jan 18, 2021
336b2af
Re-indent checkstyle file
pugnascotia Jan 18, 2021
9babfaf
Merge branch 'master' into extra-checkstyle-in-ide
elasticmachine Jan 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 40 additions & 31 deletions buildSrc/src/main/resources/checkstyle.xml
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
}