You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by Cody Koeninger <co...@koeninger.org> on 2018/11/21 17:39:44 UTC
Automated formatting
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
Re: Automated formatting
Posted by DB Tsai <db...@dbtsai.com.INVALID>.
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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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
Re: Automated formatting
Posted by Cody Koeninger <co...@koeninger.org>.
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 <ma...@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 <co...@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 <mr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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
> >>>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
> >
>
---------------------------------------------------------------------
To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
Re: Automated formatting
Posted by Santiago Saavedra <ss...@openshine.com>.
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 (<ma...@gmail.com>)
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 <co...@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 <mr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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
> >>>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
>
>
Re: Automated formatting
Posted by Matei Zaharia <ma...@gmail.com>.
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 <co...@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 <mr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
>
---------------------------------------------------------------------
To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
Re: Automated formatting
Posted by Cody Koeninger <co...@koeninger.org>.
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 <mr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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
>>
---------------------------------------------------------------------
To unsubscribe e-mail: dev-unsubscribe@spark.apache.org
Re: Automated formatting
Posted by Mridul Muralidharan <mr...@gmail.com>.
Is this handling only scala or java as well ?
Regards,
Mridul
On Thu, Nov 22, 2018 at 9:11 AM Cody Koeninger <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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
>
>
Re: Automated formatting
Posted by Cody Koeninger <co...@koeninger.org>.
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 <sr...@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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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
Re: Automated formatting
Posted by Sean Owen <sr...@gmail.com>.
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 <co...@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 <sr...@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 <co...@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 <sr...@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 <co...@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
Re: Automated formatting
Posted by Cody Koeninger <co...@koeninger.org>.
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 <sr...@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 <co...@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 <sr...@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 <co...@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
Re: Automated formatting
Posted by Sean Owen <sr...@gmail.com>.
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 <co...@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 <sr...@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 <co...@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
Re: Automated formatting
Posted by Cody Koeninger <co...@koeninger.org>.
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 <sr...@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 <co...@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
Re: Automated formatting
Posted by Sean Owen <sr...@gmail.com>.
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 <co...@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