Skip to content

Conversation

@xiaoyuyao
Copy link
Contributor

What changes were proposed in this pull request?

CheckStyle: Move LineLength Check parent from TreeWalker to Checker, otherwise fail to import to latest IntelliJ

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4306

How was this patch tested?

Import to IntelliJ checkstyle plugin and verify there is no syntax error.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying to fix this @xiaoyuyao. Unfortunately it requires some more changes to make it work from Maven, too:

  1. Upgrade Checkstyle to 8.24, which introduced this breaking change.
  2. Upgrade maven-checkstyle-plugin to 3.1.1.
  3. Restrict LineLength check to Java files. This is necessary, because the check now applies to all files.
diff --git hadoop-hdds/dev-support/checkstyle/checkstyle.xml hadoop-hdds/dev-support/checkstyle/checkstyle.xml
index 513ed247f..efc50d933 100644
--- hadoop-hdds/dev-support/checkstyle/checkstyle.xml
+++ hadoop-hdds/dev-support/checkstyle/checkstyle.xml
@@ -71,7 +71,9 @@
     <!--<module name="FileLength">-->
     <module name="FileTabCharacter"/>

-    <module name="LineLength"/>
+    <module name="LineLengthCheck">
+      <property name="fileExtensions" value="java" />
+    </module>
     <module name="TreeWalker">

         <module name="SuppressWarningsHolder"/>
diff --git pom.xml pom.xml
index 36cd9dbf0..e58569ded 100644
--- pom.xml
+++ pom.xml
@@ -221,8 +221,8 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
     <exec-maven-plugin.version>1.3.1</exec-maven-plugin.version>
     <make-maven-plugin.version>1.0-beta-1</make-maven-plugin.version>
     <native-maven-plugin.version>1.0-alpha-8</native-maven-plugin.version>
-    <maven-checkstyle-plugin.version>3.0.0</maven-checkstyle-plugin.version>
-    <checkstyle.version>8.19</checkstyle.version>
+    <maven-checkstyle-plugin.version>3.1.1</maven-checkstyle-plugin.version>
+    <checkstyle.version>8.24</checkstyle.version>
     <surefire.fork.timeout>1200</surefire.fork.timeout>
     <aws-java-sdk.version>1.11.615</aws-java-sdk.version>
     <hsqldb.version>2.3.4</hsqldb.version>

@xiaoyuyao
Copy link
Contributor Author

Thanks @adoroszlai for the review. The suggested change LGTM, I will make change in the next commit. One thing I notice that you suggest use LineLengthCheck instead of LineLength module. I think it is typo based on the document [here|https://checkstyle.sourceforge.io/config_sizes.html#LineLength]

@adoroszlai
Copy link
Contributor

One thing I notice that you suggest use LineLengthCheck instead of LineLength module. I think it is typo

Today I learned: both forms work fine.

However, you are correct, I didn't intend to propose changing the module name. Even if both are OK, it's unnecessary for the fix.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xiaoyuyao for updating the patch.

@adoroszlai adoroszlai merged commit 4e1d2ef into apache:master Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants