[HUDI-296] Explore use of spotless to auto fix formatting errors#945
[HUDI-296] Explore use of spotless to auto fix formatting errors#945vinothchandar merged 3 commits intoapache:masterfrom
Conversation
|
@vinothchandar I can't restart the build in travis page, see |
|
ah. really?.. Then better to run these locally to iterate? It seems to fail from some spotless issue. Also can we make this work with our checkstyle rules that are already in place? |
|
we spent a bunch of time tweaking it for our needs and got it consistent with code styling. So love to make minimal one-time formatting changes to this setup if possible cc @bvaradar |
|
@vinothchandar @bvaradar Please restart the build due to the exception(The forked VM terminated without properly saying goodbye. VM crash or System.exit called), travis log link(https://api.travis-ci.org/v3/job/595435352/log.txt). |
|
This PR uses both the existing checkstyle.xml(used by maven checkstyle plugin) and eclipse-java-google-format.xml(used by spotless plugin) placed in /style folder to check code style. |
|
@bvaradar what do you think? I am okay if this is one time. |
| Option.ofNullable(sortByField.isEmpty() ? null : sortByField), | ||
| Option.ofNullable(isDescending), | ||
| Option.ofNullable(limit <= 0 ? null : limit)).addAllRows(rows).flip(); | ||
| Table table = |
There was a problem hiding this comment.
kind of dislike that we are wasting the first line.. Can we fix it so that we use line 62 as well?
There was a problem hiding this comment.
This is split by spotless plugin, and the line length is defined to 120(org.eclipse.jdt.core.formatter.lineSplit) in eclipse-java-google-format.xml, we need to make org.eclipse.jdt.core.formatter.lineSplit a little larger than 120(such as 200?) to use the above line 62. But IMHO, 120 is a proper length and just let spotless plugin formats the code style, WDYT?
There was a problem hiding this comment.
Okay fair enough. We can leave it as is.
| <properties> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <checkstyle.skip>true</checkstyle.skip> | ||
| <main.basedir>${project.parent.parent.basedir}</main.basedir> |
There was a problem hiding this comment.
please test once if you are able to indepdently run a mvn command from one of the sub modules. i.e cd hudi-cli; mvn clean install should work..
There was a problem hiding this comment.
Good catch.
Before this PR(such as in master branch), we could run mvn clean install -pl hudi-client in project root dir to install specified module but not run mvn clean install in sub modules dirs i.e hudi-client(Unable to find configuration file
at location F:/community/incubator-hudi/hudi-client/style/scalastyle-config.xml). I just updated this PR to make it works and verified mvn clean install -DskipTests in sub modules(hudi-cli, hudi-client, hudi-spark..).
There was a problem hiding this comment.
And maybe we would seperate the commit to another PR(should be the basis of this PR) since it is not so relative to this PR. WDYT?
There was a problem hiding this comment.
ideally yes. but as long as building independent modules work, I am okay for this PR. your call. Let me know how you want to proceed
There was a problem hiding this comment.
I was thinking to open another PR (may also need to define main.basedir as this PR does) to make mvn clean install works in sub modules(just a minor git history improvement). However, it would also be done in this PR. So please merge this PR if there is no other concern.
|
btw spotbugs might be a good thing to explore as well. https://spotbugs.github.io general comment. not related to this PR |
bvaradar
left a comment
bvaradar
left a comment
There was a problem hiding this comment.
@vinothchandar @leesf : Agree with the idea. Good to have command line lint fixing capability.
Looks good overall.
3 participants
see jira: https://jira.apache.org/jira/projects/HUDI/issues/HUDI-296
The main changes are described below:
[1]. Add spotless plugin to the project by reference to the usage of jrtom and avro. We would run
mvn spotless:checkto check style andmvn spotless:applyto fix style errors.[2]. Change the base of eclipse-java-google-style.xml to https://github.com/google/styleguide/blob/gh-pages/eclipse-java-google-style.xml and have some minor modifications.
[3]. Change max property of LineLength in checkstyle.xml for the lack of strict limit of eclipse-java-google-style.xml
[4]. Add
SuppressionFiltermodule to handle violation between checkstyle plugin and eclipse-java-google-style.xml.[5]. Add
AvoidNestedBlocksmodule to checkstyle.xml to handle indentation in case statement.[6]. Add
RealtimeUnmergedRecordReader.javato checkstyle-suppressions.xml to handle indentation violation between checkstyle plugin and eclipse-java-google-style.xml.CC @vinothchandar Please review when you get a chance. Thanks.