You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "ASF GitHub Bot (JIRA)" <ji...@apache.org> on 2018/07/23 22:43:00 UTC
[jira] [Commented] (KAFKA-2423) Expand scalafmt coverage to core
[ https://issues.apache.org/jira/browse/KAFKA-2423?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16553498#comment-16553498 ]
ASF GitHub Bot commented on KAFKA-2423:
---------------------------------------
TolerableCoder closed pull request #4545: KAFKA-2423: Introduce Scalastyle
URL: https://github.com/apache/kafka/pull/4545
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/build.gradle b/build.gradle
index 430d9896dcf..d20ea983003 100644
--- a/build.gradle
+++ b/build.gradle
@@ -540,6 +540,7 @@ project(':core') {
println "Building project 'core' with Scala version ${versions.scala}"
apply plugin: 'scala'
+ apply from: file("$rootDir/gradle/scalastyle.gradle")
apply plugin: "org.scoverage"
archivesBaseName = "kafka_${versions.baseScala}"
diff --git a/gradle/buildscript.gradle b/gradle/buildscript.gradle
index 047632b18c9..68c3463b64c 100644
--- a/gradle/buildscript.gradle
+++ b/gradle/buildscript.gradle
@@ -20,4 +20,8 @@ repositories {
url 'http://dl.bintray.com/content/netflixoss/external-gradle-plugins/'
}
}
+
+ dependencies {
+ classpath "org.github.ngbinh.scalastyle:gradle-scalastyle-plugin_2.10:0.9.0"
+ }
}
diff --git a/gradle/resources/scalastyle_config.xml b/gradle/resources/scalastyle_config.xml
new file mode 100644
index 00000000000..caa07d2e084
--- /dev/null
+++ b/gradle/resources/scalastyle_config.xml
@@ -0,0 +1,358 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<!--
+If you wish to turn off checking for a section of code, you can put a comment in the source
+before and after the section, with the following syntax:
+
+ // scalastyle:off
+ ... // stuff that breaks the styles
+ // scalastyle:on
+
+You can also disable only one rule, by specifying its rule id, as specified in:
+ http://www.scalastyle.org/rules-0.7.0.html
+
+ // scalastyle:off no.finalize
+ override def finalize(): Unit = ...
+ // scalastyle:on no.finalize
+
+This file is divided into 3 sections:
+ (1) rules that we enforce.
+ (2) rules that we would like to enforce, but haven't cleaned up the codebase to turn on yet
+ (or we need to make the scalastyle rule more configurable).
+ (3) rules that we don't want to enforce.
+-->
+<scalastyle>
+ <name>Scalastyle standard configuration</name>
+
+ <!-- ================================================================================ -->
+ <!-- rules we enforce -->
+ <!-- ================================================================================ -->
+
+ <!-- Check for the required license header -->
+ <check level="error" class="org.scalastyle.file.HeaderMatchesChecker" enabled="true">
+ <parameters>
+ <parameter name="header">/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */</parameter>
+ </parameters>
+ </check>
+
+ <!-- Tabs or spaces. Pick one. -->
+ <check level="error" class="org.scalastyle.file.FileTabChecker" enabled="true" />
+
+ <!-- Code that is not indented consistently can be hard to read. -->
+ <check level="error" class="org.scalastyle.file.IndentationChecker" enabled="true">
+ <parameters>
+ <parameter name="tabSize">2</parameter>
+ <parameter name="methodParamIndentSize">2</parameter>
+ </parameters>
+ </check>
+
+ <!-- Some version control systems don’t cope well with files which don’t end with a newline character. -->
+ <check level="error" class="org.scalastyle.file.NewLineAtEofChecker" enabled="true"/>
+
+ <!-- Whitespace at the end of a line can cause problems when diffing between files or between versions. -->
+ <check level="error" class="org.scalastyle.file.WhitespaceEndOfLineChecker" enabled="true"/>
+
+ <!-- Scala generic type names are generally single upper case letters. This check checks for classes and traits. -->
+ <check level="error" class="org.scalastyle.scalariform.ClassNamesChecker" enabled="true">
+ <parameters>
+ <parameter name="regex">^[A-Z][A-Za-z]*$</parameter>
+ </parameters>
+ </check>
+
+ <!-- Mistakenly defining a covariant equals() method without overriding method equals(java.lang.Object)
+ can produce unexpected runtime behaviour. -->
+ <check level="error" class="org.scalastyle.scalariform.CovariantEqualsChecker" enabled="true"/>
+
+ <!-- You should be using the Scala @deprecated instead. -->
+ <check level="error" class="org.scalastyle.scalariform.DeprecatedJavaChecker" enabled="true"/>
+
+ <!-- Correct formatting can help readability. -->
+ <check level="error" class="org.scalastyle.scalariform.DisallowSpaceBeforeTokenChecker" enabled="true">
+ <parameters>
+ <parameter name="tokens">COLON, COMMA, RPAREN</parameter>
+ </parameters>
+ </check>
+
+ <!-- Correct formatting can help readability. -->
+ <check level="error" class="org.scalastyle.scalariform.EnsureSingleSpaceBeforeTokenChecker" enabled="true">
+ <parameters>
+ <parameter name="tokens">ARROW, EQUALS, ELSE, TRY, CATCH, FINALLY, LARROW, RARROW</parameter>
+ </parameters>
+ </check>
+
+ <!-- Correct formatting can help readability. -->
+ <check level="error" class="org.scalastyle.scalariform.EnsureSingleSpaceAfterTokenChecker" enabled="true">
+ <parameters>
+ <parameter name="tokens">ARROW, EQUALS, COMMA, COLON, IF, ELSE, DO, WHILE, FOR, MATCH, TRY, CATCH, FINALLY, LARROW, RARROW</parameter>
+ </parameters>
+ </check>
+
+ <!-- A consistent naming convention for field names can make code easier to read and understand. -->
+ <check level="error" class="org.scalastyle.scalariform.FieldNamesChecker" enabled="true">
+ <parameters>
+ <parameter name="regex">^[a-z][A-Za-z0-9]*$</parameter>
+ </parameters>
+ </check>
+
+ <!-- Defining either equals or hashCode in a class without defining the is a known source of bugs.
+ Usually, when you define one, you should also define the other. -->
+ <check level="error" class="org.scalastyle.scalariform.EqualsHashCodeChecker" enabled="true"/>
+
+ <!-- Use of some classes is discouraged for a project. -->
+ <check level="error" class="org.scalastyle.scalariform.IllegalImportsChecker" enabled="true">
+ <parameters>
+ <parameter name="illegalImports">sun._,java.awt._</parameter>
+ </parameters>
+ </check>
+
+ <!-- The clone method is difficult to get right.
+ You can use the copy constructor of case classes rather than implementing clone.
+ For more information on clone(), see Effective Java by Joshua Bloch pages. -->
+ <check level="error" class="org.scalastyle.scalariform.NoCloneChecker" enabled="true"/>
+
+ <!-- finalize() is called when the object is garbage collected, and garbage collection is not guaranteed to happen.
+ It is therefore unwise to rely on code in finalize() method. -->
+ <check level="error" class="org.scalastyle.scalariform.NoFinalizeChecker" enabled="true"/>
+
+ <!-- If there is whitespace after a left bracket, this can be confusing to the reader. -->
+ <check level="error" class="org.scalastyle.scalariform.NoWhitespaceBeforeLeftBracketChecker" enabled="true"/>
+
+ <!-- If there is whitespace before a left bracket, this can be confusing to the reader. -->
+ <check level="error" class="org.scalastyle.scalariform.NoWhitespaceAfterLeftBracketChecker" enabled="true"/>
+
+ <!-- Scala allows unicode characters as operators and some editors misbehave when they see non-ascii character.
+ In a project collaborated by a community of developers. This check can be helpful in such situations. -->
+ <check level="error" class="org.scalastyle.scalariform.NonASCIICharacterChecker" enabled="true"/>
+
+ <!-- ??? usually shouldn't be checked into the code base. -->
+ <check level="error" class="org.scalastyle.scalariform.NotImplementedErrorUsage" enabled="true"/>
+
+ <!-- The Scala style guide recommends that object names conform to certain standards. -->
+ <check level="error" class="org.scalastyle.scalariform.ObjectNamesChecker" enabled="true">
+ <parameters><parameter name="regex">^[A-Z][A-Za-z]*$</parameter></parameters>
+ </check>
+
+ <!-- The Scala style guide recommends that package object names conform to certain standards. -->
+ <check level="error" class="org.scalastyle.scalariform.PackageObjectNamesChecker" enabled="true">
+ <parameters><parameter name="regex">^[a-z][A-Za-z]*$</parameter></parameters>
+ </check>
+
+ <!-- A procedure style declaration can cause confusion -
+ the developer may have simply forgotten to add a ’=’,
+ and now their method returns Unit rather than the inferred type. -->
+ <check level="error" class="org.scalastyle.scalariform.PatternMatchAlignChecker" enabled="true"/>
+
+ <!-- If expressions with boolean constants in both branches can be eliminated without affecting readability. -->
+ <check level="error" class="org.scalastyle.scalariform.RedundantIfChecker" enabled="false"/>
+
+ <!-- An expression with spaces around + can be easier to read. -->
+ <check level="error" class="org.scalastyle.scalariform.SpacesAfterPlusChecker" enabled="true"/>
+
+ <!-- An expression with spaces around + can be easier to read. -->
+ <check level="error" class="org.scalastyle.scalariform.SpacesBeforePlusChecker" enabled="true"/>
+
+ <!-- Structural types in Scala can use reflection - this can have unexpected performance consequences. -->
+ <check level="error" class="org.scalastyle.scalariform.StructuralTypeChecker" enabled="true"/>
+
+ <!-- A lowercase L (l) can look similar to a number 1 with some fonts. -->
+ <check level="error" class="org.scalastyle.scalariform.UppercaseLChecker" enabled="true"/>
+
+ <!-- @VisibleForTesting causes classpath issues. Please note this in the java doc instead. -->
+ <check customId="visiblefortesting" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">
+ <parameters><parameter name="regex">@VisibleForTesting</parameter></parameters>
+ <customMessage>@VisibleForTesting causes classpath issues. Please note this in the java doc instead.</customMessage>
+ </check>
+
+ <!-- ================================================================================ -->
+ <!-- rules we'd like to enforce, but haven't cleaned up the codebase yet -->
+ <!-- ================================================================================ -->
+
+ <!-- Files which are too long can be hard to read and understand. -->
+ <check level="warning" class="org.scalastyle.file.FileLengthChecker" enabled="true">
+ <parameters>
+ <parameter name="maxFileLength">800</parameter>
+ </parameters>
+ </check>
+
+ <!-- Once we refactor this we should change to error -->
+ <!-- Lines that are too long can be hard to read and horizontal scrolling is annoying.-->
+ <check level="warning" class="org.scalastyle.file.FileLineLengthChecker" enabled="true">
+ <parameters>
+ <parameter name="maxLineLength">120</parameter>
+ <parameter name="tabSize">2</parameter>
+ <parameter name="ignoreImports">true</parameter>
+ </parameters>
+ </check>
+
+ <!-- Super long methods should be broken into smaller, more readable, pieces of logic. -->
+ <check level="warning" class="org.scalastyle.scalariform.MethodLengthChecker" enabled="true">
+ <parameters><parameter name="maxLength">100</parameter></parameters>
+ </check>
+
+ <!-- This breaks symbolic method names so we don't turn it on. -->
+ <!-- Maybe we should update it to allow basic symbolic names, and then we are good to go. -->
+ <check level="error" class="org.scalastyle.scalariform.MethodNamesChecker" enabled="false">
+ <parameters>
+ <parameter name="regex">^[a-z][A-Za-z0-9]*$</parameter>
+ </parameters>
+ </check>
+
+ <!-- A method which has more than a certain number of parameters can be hard to understand. -->
+ <check level="warning" class="org.scalastyle.scalariform.ParameterNumberChecker" enabled="true">
+ <parameters><parameter name="maxParameters">10</parameter></parameters>
+ </check>
+
+ <!-- A public method declared on a type is effectively an API declaration.
+ Explicitly declaring a return type means that other code which depends on that type
+ won’t break unexpectedly. -->
+ <check level="warning" class="org.scalastyle.scalariform.PublicMethodsHaveTypeChecker" enabled="true"/>
+
+ <!-- Once we refactor this we should change to error -->
+ <!-- JavaConversions should be replaced with JavaConverters -->
+ <check customId="javaconversions" level="warning" class="org.scalastyle.scalariform.TokenChecker" enabled="true">
+ <parameters><parameter name="regex">JavaConversions</parameter></parameters>
+ <customMessage>Instead of importing implicits in scala.collection.JavaConversions._, import
+ scala.collection.JavaConverters._ and use .asScala / .asJava methods</customMessage>
+ </check>
+
+ <!-- printlns if needed should be to be wrapped in '// scalastyle:off/on println' -->
+ <check customId="println" level="warning" class="org.scalastyle.scalariform.TokenChecker" enabled="true">
+ <parameters><parameter name="regex">^println$</parameter></parameters>
+ <customMessage>Are you sure you want to println? If yes, wrap the code block with
+ // scalastyle:off println
+ println(...)
+ // scalastyle:on println</customMessage>
+ </check>
+
+ <check customId="classforname" level="warning" class="org.scalastyle.file.RegexChecker" enabled="true">
+ <parameters><parameter name="regex">Class\.forName</parameter></parameters>
+ <customMessage>
+ Are you sure that you want to use Class.forName? In most cases, you should use Utils.classForName instead.
+ If you must use Class.forName, wrap the code block with
+ // scalastyle:off classforname
+ Class.forName(...)
+ // scalastyle:on classforname</customMessage>
+ </check>
+
+
+ <!-- ================================================================================ -->
+ <!-- rules we don't want -->
+ <!-- ================================================================================ -->
+
+ <!-- We want the opposite of this: NewLineAtEofChecker -->
+ <check level="error" class="org.scalastyle.file.NoNewLineAtEofChecker" enabled="false"/>
+
+ <!-- Not a big concern. Mostly used to avoid merge errors.-->
+ <check level="error" class="org.scalastyle.scalariform.BlockImportChecker" enabled="false"/>
+
+ <!-- Complexity should be discussed in reviews. -->
+ <check level="error" class="org.scalastyle.scalariform.CyclomaticComplexityChecker" enabled="false">
+ <parameters><parameter name="maximum">10</parameter></parameters>
+ </check>
+
+ <!-- Not a big concern. The braces aren't major. -->
+ <check level="error" class="org.scalastyle.scalariform.EmptyClassChecker" enabled="true"/>
+
+ <!-- Not a big concern. -->
+ <check level="error" class="org.scalastyle.scalariform.ForBraceChecker" enabled="true"/>
+
+
+ <!-- If statement blocks with only a single line should not have braces according to the standard scala style -->
+ <check level="error" class="org.scalastyle.scalariform.IfBraceChecker" enabled="false">
+ <parameters>
+ <parameter name="singleLineAllowed">true</parameter>
+ <parameter name="doubleLineAllowed">true</parameter>
+ </parameters>
+ </check>
+
+ <!-- This is generally done reasonably and otherwise infrequently -->
+ <check level="error" class="org.scalastyle.scalariform.ImportGroupingChecker" enabled="false"/>
+
+ <!-- This is generally more pain than its worth -->
+ <check level="error" class="org.scalastyle.scalariform.ImportOrderChecker" enabled="false"/>
+
+ <!-- Often used purposefully. -->
+ <check level="error" class="org.scalastyle.scalariform.LowercasePatternMatchChecker" enabled="false"/>
+
+ <!-- Fairly sensitive, and we have a lot of magic numbers ... -->
+ <check level="error" class="org.scalastyle.scalariform.MagicNumberChecker" enabled="false">
+ <parameters><parameter name="ignore">-1,0,1,2,3</parameter></parameters>
+ </check>
+
+ <!-- Usefully to enable temporarily but not realistic library wide -->
+ <check level="error" class="org.scalastyle.scalariform.MultipleStringLiteralsChecker" enabled="false">
+ <parameters>
+ <parameter name="allowed">1</parameter>
+ <parameter name="ignoreRegex">^\"\"$</parameter>
+ </parameters>
+ </check>
+
+ <!-- We use null a lot in low level code and to interface with 3rd party code -->
+ <check level="error" class="org.scalastyle.scalariform.NullChecker" enabled="false"/>
+
+ <!-- It is infrequent that somebody introduces a new class with a lot of methods. Should be discussed in reviews. -->
+ <check level="error" class="org.scalastyle.scalariform.NumberOfMethodsInTypeChecker" enabled="false">
+ <parameters><parameter name="maxMethods">30</parameter></parameters>
+ </check>
+
+ <!-- It is infrequent that somebody introduces a new class with a lot of types. Should be discussed in reviews. -->
+ <check level="error" class="org.scalastyle.scalariform.NumberOfTypesChecker" enabled="false">
+ <parameters><parameter name="maxTypes">30</parameter></parameters>
+ </check>
+
+ <!-- Can hurt readability as often as it helps it. -->
+ <check level="error" class="org.scalastyle.scalariform.PatternMatchAlignChecker" enabled="false"/>
+
+ <!-- We use return quite a bit for control flows and guards -->
+ <check level="error" class="org.scalastyle.scalariform.ReturnChecker" enabled="false"/>
+
+ <!-- Not necessarily worth breaking the build -->
+ <check level="error" class="org.scalastyle.scalariform.ScalaDocChecker" enabled="false">
+ <parameters>
+ <parameter name="ignoreRegex">(.*Spec$)|(.*SpecIT$)</parameter>
+ </parameters>
+ </check>
+
+ <!-- Can complain about all kinds of random things. -->
+ <check level="error" class="org.scalastyle.scalariform.SimplifyBooleanExpressionChecker" enabled="false"/>
+
+ <!-- Not a big concern. -->
+ <check level="error" class="org.scalastyle.scalariform.SpaceAfterCommentStartChecker" enabled="false"/>
+
+ <!-- We are not "strictly" purely functional. Reviews should discuss this. -->
+ <check level="error" class="org.scalastyle.scalariform.VarFieldChecker" enabled="false"/>
+ <check level="error" class="org.scalastyle.scalariform.VarLocalChecker" enabled="false"/>
+ <check level="error" class="org.scalastyle.scalariform.WhileChecker" enabled="false"/>
+
+ <!-- Not a big concern. -->
+ <check level="error" class="org.scalastyle.scalariform.XmlLiteralChecker" enabled="false"/>
+</scalastyle>
diff --git a/gradle/scalastyle.gradle b/gradle/scalastyle.gradle
new file mode 100644
index 00000000000..f32ad8e0422
--- /dev/null
+++ b/gradle/scalastyle.gradle
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+apply plugin: 'scalaStyle'
+
+scalaStyle {
+ configLocation = "$rootDir/gradle/resources/scalastyle_config.xml"
+ outputFile = "core/build/scalastyle/scalastyle_report.xml"
+ includeTestSourceDirectory = true
+ failOnViolation = false // Change to "true" when the run is clean
+ failOnWarning = false
+ quiet = true
+ source = "src/main/scala"
+ testSource = "src/test/scala"
+}
+scalaStyle.dependsOn('compileScala', 'compileTestScala')
+check.dependsOn('scalaStyle')
+test.shouldRunAfter('scalaStyle')
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> Expand scalafmt coverage to core
> --------------------------------
>
> Key: KAFKA-2423
> URL: https://issues.apache.org/jira/browse/KAFKA-2423
> Project: Kafka
> Issue Type: Improvement
> Reporter: Ismael Juma
> Assignee: Ray Chiang
> Priority: Major
>
> This is similar to Checkstyle (which we already use), but for Scala:
> http://www.scalastyle.org/
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)