Automated formatting

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Automated formatting

Cody Koeninger-2
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: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Sean Owen-2
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 <[hidden email]> 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: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Cody Koeninger-2
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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Sean Owen-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Cody Koeninger-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Sean Owen-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > > > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

DB Tsai-6
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > > > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

--
- DB Sent from my iPhone
Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Cody Koeninger-2
In reply to this post by Sean Owen-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > > > > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Mridul Muralidharan
Is this handling only scala or java as well ?

Regards,
Mridul 

On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> > > > > >

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Cody Koeninger-2
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 <[hidden email]> wrote:

>
> Is this handling only scala or java as well ?
>
> Regards,
> Mridul
>
> On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
>> > > > > > >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe e-mail: [hidden email]
>>

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Matei Zaharia
Administrator
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 <[hidden email]> 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 <[hidden email]> wrote:
>>
>> Is this handling only scala or java as well ?
>>
>> Regards,
>> Mridul
>>
>> On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
>>>>>>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe e-mail: [hidden email]
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

ssaavedra
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, scalafmt works quite well (very few cases where running twice is not idempotent).

El vie., 23 nov. 2018 a las 2:32, Matei Zaharia (<[hidden email]>) escribió:
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 <[hidden email]> 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 <[hidden email]> wrote:
>>
>> Is this handling only scala or java as well ?
>>
>> Regards,
>> Mridul
>>
>> On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
>>>>>>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe e-mail: [hidden email]
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Automated formatting

Cody Koeninger-2
In reply to this post by Matei Zaharia
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
> >>
> >> Is this handling only scala or java as well ?
> >>
> >> Regards,
> >> Mridul
> >>
> >> On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> >>>>>>>>>
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe e-mail: [hidden email]
> >>>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe e-mail: [hidden email]
> >
>

---------------------------------------------------------------------
To unsubscribe e-mail: [hidden email]