Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 )
Change subject: Scala code formatting with Scalafmt ...................................................................... Patch Set 2: (9 comments) Even though the Maven build is going away, I think we should setup the Maven plugin too. While we still have both builds it would be unfortunate if things diverged based on what build you were using. http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG@9 PS2, Line 9: Scalafmt is added using the Gradle Scalafmt plugin: Have you considered using the spotless plugin? https://github.com/diffplug/spotless/tree/master/plugin-gradle#applying-scalafmt-to-scala-files It looks like that could support all types of formatting needs. http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf File java/.scalafmt.conf: http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@1 PS2, Line 1: rewrite.rules = [SortImports, sortModifiers, prefercurlyfors, AvoidInfix] I like `expandimportselectors` because it results in less change collisions for backports http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@2 PS2, Line 2: align = more Given basically all of our cases statements aren't aligned today, could we use align.none here so that case statements aren't aligned? Your below alignment settings will still stay true. http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@9 PS2, Line 9: newlines.alwaysBeforeTopLevelStatements = true Any thoughts on setting newlines.penalizeSingleSelectMultiArgList = false? http://gerrit.cloudera.org:8080/#/c/11030/2/java/gradle/quality.gradle File java/gradle/quality.gradle: http://gerrit.cloudera.org:8080/#/c/11030/2/java/gradle/quality.gradle@25 PS2, Line 25: apply plugin: "scalafmt" nit: newline Even though we use the default path ".scalafmt.conf" can you add the configuration here so it's explicit? ``` scalafmt { configFilePath = ".scalafmt.conf" } ``` Adding something like below should allow you to always run the formatting task on compile: ``` tasks.withType(ScalaCompile) { if (!propertyExists("skipFormat")) { dependsOn("scalafmtAll") } } ``` http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-backup/out/test/resources/log4j.properties File java/kudu-backup/out/test/resources/log4j.properties: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-backup/out/test/resources/log4j.properties@1 PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one I don't think this file is supposed to be here. Older version of intellij had a bug that generated the /out directories when working with Gradle. Are you on a current version of Gradle and IntelliJ? I have a global .gitignore file which ignores the /out directory from back when it was an issue. That's an option too. You can see my global gitignore here: https://gist.github.com/granthenke/3b5e688e57dcadb00c4fc9814569018b http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark-tools/out/test/resources/log4j.properties File java/kudu-spark-tools/out/test/resources/log4j.properties: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark-tools/out/test/resources/log4j.properties@1 PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one I don't think this file is supposed to be here. http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala@410 PS2, Line 410: case EqualTo(_, _) | GreaterThan(_, _) | GreaterThanOrEqual(_, _) | Maybe add // format: off, // format: on around this. I prefer the stacked formatting here. http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala@24 PS2, Line 24: * Adds a method, `kudu`, to DataFrameReader that allows you to read Kudu tables using Why is this using the Scala style comment? -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 2 Gerrit-Owner: Tony Foerster <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 15:08:34 +0000 Gerrit-HasComments: Yes
