Re: Automated formatting

2018-11-26 Thread Cody Koeninger
That seems like a good first step. Opened a PR / jira ticket with that approach at https://github.com/apache/spark/pull/23148 If anyone tests this and finds a file that doesn't format well (e.g. fails scalastyle afterwards) just let me know, happy to tweak scalafmt config options. On Thu, Nov

Re: Automated formatting

2018-11-23 Thread Santiago Saavedra
As a trial, it could be configured on Jenkins to run the job as a non-critical step, where failure wouldn't constitute a build failure, and assess whether it's useful by trying it out. Scalafmt only works for Scala sources, but as Cody says, there are other utilities for Java. From my experience,

Re: Automated formatting

2018-11-22 Thread Matei Zaharia
Can we start by just recommending to contributors that they do this manually? Then if it seems to work fine, we can try to automate it. > On Nov 22, 2018, at 4:40 PM, Cody Koeninger wrote: > > I believe scalafmt only works on scala sources. There are a few > plugins for formatting java

Re: Automated formatting

2018-11-22 Thread Cody Koeninger
I believe scalafmt only works on scala sources. There are a few plugins for formatting java sources, but I'm less familiar with them. On Thu, Nov 22, 2018 at 11:39 AM Mridul Muralidharan wrote: > > Is this handling only scala or java as well ? > > Regards, > Mridul > > On Thu, Nov 22, 2018 at

Re: Automated formatting

2018-11-22 Thread Mridul Muralidharan
Is this handling only scala or java as well ? Regards, Mridul On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger wrote: > Plugin invocation is ./build/mvn mvn-scalafmt_2.12:format > > It takes about 5 seconds, and errors out on the first different file > that doesn't match formatting. > > I made a

Re: Automated formatting

2018-11-22 Thread Cody Koeninger
Plugin invocation is ./build/mvn mvn-scalafmt_2.12:format It takes about 5 seconds, and errors out on the first different file that doesn't match formatting. I made a shell wrapper so that contributors can just run ./dev/scalafmt to actually format in place the files that have changed (or pass

Re: Automated formatting

2018-11-21 Thread DB Tsai
I like the idea of checking only the diff. Even I am sometimes confused about the right style in Spark since I am working on multiple projects with slightly different coding styles. On Wed, Nov 21, 2018 at 1:36 PM Sean Owen wrote: > I know the PR builder runs SBT, but I presume this would just

Re: Automated formatting

2018-11-21 Thread Sean Owen
I know the PR builder runs SBT, but I presume this would just be a separate mvn job that runs. If it doesn't take long and only checks the right diff, seems worth a shot. What's the invocation that Shane could add (after this change goes in) On Wed, Nov 21, 2018 at 3:27 PM Cody Koeninger wrote: >

Re: Automated formatting

2018-11-21 Thread Cody Koeninger
There's a mvn plugin (sbt as well, but it requires sbt 1.0+) so it should be runnable from the PR builder Super basic example with a minimal config that's close to current style guide here: https://github.com/apache/spark/compare/master...koeninger:scalafmt I imagine tracking down the corner

Re: Automated formatting

2018-11-21 Thread Sean Owen
Yeah fair, maybe mostly consistent in broad strokes but not in the details. Is this something that can be just run in the PR builder? if the rules are simple and not too hard to maintain, seems like a win. On Wed, Nov 21, 2018 at 2:26 PM Cody Koeninger wrote: > > Definitely not suggesting a mass

Re: Automated formatting

2018-11-21 Thread Cody Koeninger
Definitely not suggesting a mass reformat, just on a per-PR basis. scalafmt --diff will reformat only the files that differ from git head scalafmt --test --diff won't modify files, just throw an exception if they don't match format I don't think code is consistently formatted now. I tried

Re: Automated formatting

2018-11-21 Thread Sean Owen
I think reformatting the whole code base might be too much. If there are some more targeted cleanups, sure. We do have some links to style guides buried somewhere in the docs, although the conventions are pretty industry standard. I *think* the code is pretty consistently formatted now, and would

Automated formatting

2018-11-21 Thread Cody Koeninger
Is there any appetite for revisiting automating formatting? I know over the years various people have expressed opposition to it as unnecessary churn in diffs, but having every new contributor greeted with "nit: 4 space indentation for argument lists" isn't very welcoming.