spark-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From DB Tsai <dbt...@dbtsai.com.INVALID>
Subject Re: Automated formatting
Date Thu, 22 Nov 2018 03:59:51 GMT
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 <srowen@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 <cody@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 <srowen@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 <cody@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 <srowen@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 <
> cody@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-unsubscribe@spark.apache.org
> > > > > >
>
> ---------------------------------------------------------------------
> To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
>
> --
- DB Sent from my iPhone

Mime
View raw message