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 22, 2018 at 7:32 PM Matei Zaharia <matei.zaha...@gmail.com> wrote: > > 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 <c...@koeninger.org> wrote: > > > > 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 <mri...@gmail.com> > > wrote: > >> > >> Is this handling only scala or java as well ? > >> > >> Regards, > >> Mridul > >> > >> On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <c...@koeninger.org> 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 shell wrapper so that contributors can just run > >>> > >>> ./dev/scalafmt > >>> > >>> to actually format in place the files that have changed (or pass > >>> through commandline args if they want to do something different) > >>> > >>> On Wed, Nov 21, 2018 at 3:36 PM Sean Owen <sro...@gmail.com> wrote: > >>>> > >>>> 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 <c...@koeninger.org> > >>>> wrote: > >>>>> > >>>>> 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 cases in the config, especially > >>>>> around interactions with scalastyle, may take a bit of work. Happy to > >>>>> do it, but not if there's significant concern about style related > >>>>> changes in PRs. > >>>>> On Wed, Nov 21, 2018 at 2:42 PM Sean Owen <sro...@gmail.com> wrote: > >>>>>> > >>>>>> 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 <c...@koeninger.org> > >>>>>> wrote: > >>>>>>> > >>>>>>> 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 scalafmt on the most recent PR I looked at, and it caught > >>>>>>> stuff as basic as newlines before curly brace in existing code. > >>>>>>> I've had different reviewers for PRs that were literal backports or > >>>>>>> cut & paste of each other come up with different formatting nits. > >>>>>>> > >>>>>>> > >>>>>>> On Wed, Nov 21, 2018 at 12:03 PM Sean Owen <sro...@gmail.com> wrote: > >>>>>>>> > >>>>>>>> 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 > >>>>>>>> expect contributors to follow formatting they see, so ideally the > >>>>>>>> surrounding code alone is enough to give people guidance. In > >>>>>>>> practice, > >>>>>>>> we're always going to have people format differently no matter what I > >>>>>>>> think so it's inevitable. > >>>>>>>> > >>>>>>>> Is there a way to just check style on PR changes? that's fine. > >>>>>>>> On Wed, Nov 21, 2018 at 11:40 AM Cody Koeninger <c...@koeninger.org> > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> 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. > >>>>>>>>> > >>>>>>>>> --------------------------------------------------------------------- > >>>>>>>>> To unsubscribe e-mail: dev-unsubscr...@spark.apache.org > >>>>>>>>> > >>> > >>> --------------------------------------------------------------------- > >>> To unsubscribe e-mail: dev-unsubscr...@spark.apache.org > >>> > > > > --------------------------------------------------------------------- > > To unsubscribe e-mail: dev-unsubscr...@spark.apache.org > > > --------------------------------------------------------------------- To unsubscribe e-mail: dev-unsubscr...@spark.apache.org
