You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Kenneth Knowles <kl...@google.com> on 2018/06/27 04:15:18 UTC
[DISCUSS] Automation for Java code formatting
Hi all,
I like readable code, but I don't like formatting it myself. And I _really_
don't like discussing in code review. "Spotless" [1] can enforce - and
automatically apply - automatic formatting for Java, Groovy, and some
others.
This is not about style or wanting a particular layout. This is about
automation, contributor experience, and streamlining review
- Contributor experience: MUCH better than checkstyle: error message just
says "run ./gradlew :beam-your-module:spotlessApply" instead of telling
them to go in and manually edit.
- Automation: You want to use autoformat so you don't have to format code
by hand. But if you autoformat a file that was in some other format, then
you touch a bunch of unrelated lines. If the file is already autoformatted,
it is much better.
- Review: Never talk about code formatting ever again. A PR also needs
baseline to already be autoformatted or formatting will make it unclear
which lines are really changed.
This is already available via applyJavaNature(enableSpotless: true) and it
is turned on for SQL and our buildSrc gradle plugins. It is very nice.
There is a JIRA [2] to turn it on for the hold code base. Personally, I
think (a) every module could make a different choice if the main
contributors feel strongly and (b) it is objectively better to always
autoformat :-)
WDYT? If we do it, it is trivial to add it module-at-a-time or globally. If
someone conflicts with a massive autoformat commit, they can just keep
their changes and autoformat them and it is done.
Kenn
[1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
[2] https://issues.apache.org/jira/browse/BEAM-4394
Re: [DISCUSS] Automation for Java code formatting
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
+1
I already used it in SQL module and it's great. It makes sense to apply on all modules.
Thanks !
Regards
JB
Le 27 juin 2018 à 12:15, à 12:15, Kenneth Knowles <kl...@google.com> a écrit:
>Hi all,
>
>I like readable code, but I don't like formatting it myself. And I
>_really_
>don't like discussing in code review. "Spotless" [1] can enforce - and
>automatically apply - automatic formatting for Java, Groovy, and some
>others.
>
>This is not about style or wanting a particular layout. This is about
>automation, contributor experience, and streamlining review
>
>- Contributor experience: MUCH better than checkstyle: error message
>just
>says "run ./gradlew :beam-your-module:spotlessApply" instead of telling
>them to go in and manually edit.
>
>- Automation: You want to use autoformat so you don't have to format
>code
>by hand. But if you autoformat a file that was in some other format,
>then
>you touch a bunch of unrelated lines. If the file is already
>autoformatted,
>it is much better.
>
> - Review: Never talk about code formatting ever again. A PR also needs
>baseline to already be autoformatted or formatting will make it unclear
>which lines are really changed.
>
>This is already available via applyJavaNature(enableSpotless: true) and
>it
>is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>There is a JIRA [2] to turn it on for the hold code base. Personally, I
>think (a) every module could make a different choice if the main
>contributors feel strongly and (b) it is objectively better to always
>autoformat :-)
>
>WDYT? If we do it, it is trivial to add it module-at-a-time or
>globally. If
>someone conflicts with a massive autoformat commit, they can just keep
>their changes and autoformat them and it is done.
>
>Kenn
>
>[1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>[2] https://issues.apache.org/jira/browse/BEAM-4394
Re: [DISCUSS] Automation for Java code formatting
Posted by Andrew Pilloud <ap...@google.com>.
I agree that PRs with formatting changes are impossible to review, but I
think that is exactly what this is trying to avoid. If this is run nightly,
some contributors will still autoformat their PRs and have a bunch of
unrelated changes in the diff. If the code is always well formatted (and
that is enforced by a precommit), then you will never have a PR with
unrelated formatting issues.
Andrew
On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
> If spotless is run against a PR that is already well formatted its a
> non-issue as the formatting changes are usually related to the change but I
> have reviewed a few PRs that have 100s of lines of formatting change which
> really obfuscates the work.
> Instead of asking contributors to run spotless, can we have a cron job run
> it across the project like once a day/week/... and cut a PR?
>
> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>
>> Good points, Dan. Checkstyle will still run, but just focused on the
>> things that go beyond format.
>>
>> Kenn
>>
>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>> wrote:
>>
>>> +1 !
>>> It's my custom to avoid reformatting to spare meaningless diff burden to
>>> the reviewer. Now it will be over, thanks.
>>>
>>> Etienne
>>>
>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>
>>> Hi all,
>>>
>>> I like readable code, but I don't like formatting it myself. And I
>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>> others.
>>>
>>> This is not about style or wanting a particular layout. This is about
>>> automation, contributor experience, and streamlining review
>>>
>>> - Contributor experience: MUCH better than checkstyle: error message
>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>> telling them to go in and manually edit.
>>>
>>> - Automation: You want to use autoformat so you don't have to format
>>> code by hand. But if you autoformat a file that was in some other format,
>>> then you touch a bunch of unrelated lines. If the file is already
>>> autoformatted, it is much better.
>>>
>>> - Review: Never talk about code formatting ever again. A PR also needs
>>> baseline to already be autoformatted or formatting will make it unclear
>>> which lines are really changed.
>>>
>>> This is already available via applyJavaNature(enableSpotless: true) and
>>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>>> think (a) every module could make a different choice if the main
>>> contributors feel strongly and (b) it is objectively better to always
>>> autoformat :-)
>>>
>>> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
>>> If someone conflicts with a massive autoformat commit, they can just keep
>>> their changes and autoformat them and it is done.
>>>
>>> Kenn
>>>
>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>
>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Rafael Fernandez <rf...@google.com>.
On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
> Luke: the proposal here solves exactly what you are talking about.
>
> The problem you describe happens when the PR author uses autoformat but
> the baseline is not already autoformatted. What I am proposing is to make
> sure the baseline is already autoformatted, so PRs never have extraneous
> formatting changes.
>
> Rafael: the default setting on GitHub is "allow edits by maintainers" so
> actually a committer can run spotless on behalf of a contributor and push
> the fixup. I have done this. It also lets a committer fix up a good PR
> and merge it even if the contributor is, say, asleep.
>
This is a great practice, review the technical part, use the tool to
address the mechanical part.
>
> Kenn
>
> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
> wrote:
>
>> Luke: Anything that helps contributors and reviewers work better together
>> - +1! :D
>>
>>
>>
>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>
>>> If spotless is run against a PR that is already well formatted its a
>>> non-issue as the formatting changes are usually related to the change but I
>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>> really obfuscates the work.
>>> Instead of asking contributors to run spotless, can we have a cron job
>>> run it across the project like once a day/week/... and cut a PR?
>>>
>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>>> things that go beyond format.
>>>>
>>>> Kenn
>>>>
>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>>>> wrote:
>>>>
>>>>> +1 !
>>>>> It's my custom to avoid reformatting to spare meaningless diff burden
>>>>> to the reviewer. Now it will be over, thanks.
>>>>>
>>>>> Etienne
>>>>>
>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>
>>>>> Hi all,
>>>>>
>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>> others.
>>>>>
>>>>> This is not about style or wanting a particular layout. This is about
>>>>> automation, contributor experience, and streamlining review
>>>>>
>>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>>> telling them to go in and manually edit.
>>>>>
>>>>> - Automation: You want to use autoformat so you don't have to format
>>>>> code by hand. But if you autoformat a file that was in some other format,
>>>>> then you touch a bunch of unrelated lines. If the file is already
>>>>> autoformatted, it is much better.
>>>>>
>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>> unclear which lines are really changed.
>>>>>
>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>> I think (a) every module could make a different choice if the main
>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>> autoformat :-)
>>>>>
>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>> just keep their changes and autoformat them and it is done.
>>>>>
>>>>> Kenn
>>>>>
>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>
>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
It is done. In the same PR I wiped out checkstyle rules that became
redundant. There are still quite a few.
On Thu, Jun 28, 2018 at 10:02 AM Udi Meiri <eh...@google.com> wrote:
> Python already has lint checks: :beam-sdks-python:lint
> There are autoformatting tools, which we don't use AFAIK:
> https://github.com/myint/autoflake
> https://github.com/hhatto/autopep8
>
>
> On Thu, Jun 28, 2018 at 12:31 AM Daniel Kulp <dk...@apache.org> wrote:
>
>>
>>
>> On Jun 28, 2018, at 6:01 AM, Kenneth Knowles <kl...@google.com> wrote:
>>
>> So, there are a few modes of checkstyle failure that can be induced by
>> this:
>>
>> - lines get too long because of wrap & indent logic
>> - a lot of our HTML is actually already broken and doesn't have <p> tags
>> where it should; spotless adds a blank line which makes checkstyle notice
>> the errors
>>
>> I think that autoformat outweighs these. I propose that we comment out
>> these checkstyle rules, turn on autoformat, then gradually restore the
>> rules. I have adjusted my pull request accordingly.
>>
>>
>> Yea… when I was adding all the whitespace rules to checkstyle config, I
>> looked at the spotless stuff and could not find a set of rules for
>> checkstyle that would 100% meet the spotless output. I fully expect to
>> need to disable a few of the rules. However, if spotless is run
>> automatically so the code is always formatted correctly, checkstyle is
>> redundant.
>>
>>
>> --
>> Daniel Kulp
>> dkulp@apache.org - http://dankulp.com/blog
>> Talend Community Coder - http://coders.talend.com
>>
>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Udi Meiri <eh...@google.com>.
Python already has lint checks: :beam-sdks-python:lint
There are autoformatting tools, which we don't use AFAIK:
https://github.com/myint/autoflake
https://github.com/hhatto/autopep8
On Thu, Jun 28, 2018 at 12:31 AM Daniel Kulp <dk...@apache.org> wrote:
>
>
> On Jun 28, 2018, at 6:01 AM, Kenneth Knowles <kl...@google.com> wrote:
>
> So, there are a few modes of checkstyle failure that can be induced by
> this:
>
> - lines get too long because of wrap & indent logic
> - a lot of our HTML is actually already broken and doesn't have <p> tags
> where it should; spotless adds a blank line which makes checkstyle notice
> the errors
>
> I think that autoformat outweighs these. I propose that we comment out
> these checkstyle rules, turn on autoformat, then gradually restore the
> rules. I have adjusted my pull request accordingly.
>
>
> Yea… when I was adding all the whitespace rules to checkstyle config, I
> looked at the spotless stuff and could not find a set of rules for
> checkstyle that would 100% meet the spotless output. I fully expect to
> need to disable a few of the rules. However, if spotless is run
> automatically so the code is always formatted correctly, checkstyle is
> redundant.
>
>
> --
> Daniel Kulp
> dkulp@apache.org - http://dankulp.com/blog
> Talend Community Coder - http://coders.talend.com
>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Reuven Lax <re...@google.com>.
I don't think so. Checkstyle checks for a number of things beyond simple
formatting issues, (i.e. redundant imports).
On Thu, Jun 28, 2018 at 12:31 AM Daniel Kulp <dk...@apache.org> wrote:
>
>
> On Jun 28, 2018, at 6:01 AM, Kenneth Knowles <kl...@google.com> wrote:
>
> So, there are a few modes of checkstyle failure that can be induced by
> this:
>
> - lines get too long because of wrap & indent logic
> - a lot of our HTML is actually already broken and doesn't have <p> tags
> where it should; spotless adds a blank line which makes checkstyle notice
> the errors
>
> I think that autoformat outweighs these. I propose that we comment out
> these checkstyle rules, turn on autoformat, then gradually restore the
> rules. I have adjusted my pull request accordingly.
>
>
> Yea… when I was adding all the whitespace rules to checkstyle config, I
> looked at the spotless stuff and could not find a set of rules for
> checkstyle that would 100% meet the spotless output. I fully expect to
> need to disable a few of the rules. However, if spotless is run
> automatically so the code is always formatted correctly, checkstyle is
> redundant.
>
>
> --
> Daniel Kulp
> dkulp@apache.org - http://dankulp.com/blog
> Talend Community Coder - http://coders.talend.com
>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Daniel Kulp <dk...@apache.org>.
> On Jun 28, 2018, at 6:01 AM, Kenneth Knowles <kl...@google.com> wrote:
>
> So, there are a few modes of checkstyle failure that can be induced by this:
>
> - lines get too long because of wrap & indent logic
> - a lot of our HTML is actually already broken and doesn't have <p> tags where it should; spotless adds a blank line which makes checkstyle notice the errors
>
> I think that autoformat outweighs these. I propose that we comment out these checkstyle rules, turn on autoformat, then gradually restore the rules. I have adjusted my pull request accordingly.
Yea… when I was adding all the whitespace rules to checkstyle config, I looked at the spotless stuff and could not find a set of rules for checkstyle that would 100% meet the spotless output. I fully expect to need to disable a few of the rules. However, if spotless is run automatically so the code is always formatted correctly, checkstyle is redundant.
--
Daniel Kulp
dkulp@apache.org <ma...@apache.org> - http://dankulp.com/blog <http://dankulp.com/blog>
Talend Community Coder - http://coders.talend.com <http://coders.talend.com/>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
So, there are a few modes of checkstyle failure that can be induced by this:
- lines get too long because of wrap & indent logic
- a lot of our HTML is actually already broken and doesn't have <p> tags
where it should; spotless adds a blank line which makes checkstyle notice
the errors
I think that autoformat outweighs these. I propose that we comment out
these checkstyle rules, turn on autoformat, then gradually restore the
rules. I have adjusted my pull request accordingly.
Kenn
On Wed, Jun 27, 2018 at 5:05 PM Robert Burke <ro...@frantil.com> wrote:
> +1!
> Given this is the recommended default for Go projects (with it's own gofmt
> tool) I'm for similar tooling for this in other languages.
>
> I suspect we can add Gradle command to run gofmt over the go code too for
> a consistent hook to run for beam code. It would save folks from needing to
> set up their IDEs for every language independently, any of which they may
> not be used to the idiomatic conventions.
>
> On Wed, Jun 27, 2018, 3:32 PM Kenneth Knowles <kl...@google.com> wrote:
>
>> OK, I opened https://github.com/apache/beam/pull/5797. This thread is
>> still less than a day, so there's no commitment yet. There's no big hurry -
>> I can always discard the autoformat and rerun it. I'm sure there will
>> always be conflicts so I will just have to rerun it and merge at some quiet
>> period for the repo. I will see about pinging existing PRs.
>>
>> On Wed, Jun 27, 2018 at 11:31 AM Daniel Oliveira <da...@google.com>
>> wrote:
>>
>>> +1 I'll throw in my support for auto-formatting, especially if the
>>> entire project is auto-formatted in advance.
>>>
>>> On Wed, Jun 27, 2018 at 10:53 AM Huygaa Batsaikhan <ba...@google.com>
>>> wrote:
>>>
>>>> +1. Global auto-formatting is cool!
>>>>
>>>> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <kl...@google.com>
>>>> wrote:
>>>>
>>>>> I just mean that because of how the tool works. But I guess if there
>>>>> were discretion then two different people could end up with autoformatting
>>>>> that disagrees, so again you get lines in the PR diff that aren't real
>>>>> changes.
>>>>>
>>>>> Kenn
>>>>>
>>>>> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com>
>>>>> wrote:
>>>>>
>>>>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Nope! No discretion allowed :-)
>>>>>>>
>>>>>>
>>>>>> +1. Fair enough!
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> +1.
>>>>>>>>
>>>>>>>> Wondering if it can be configured to reformat only what we care
>>>>>>>> most about (2 space indentation etc), allowing some discretion on the
>>>>>>>> edges. An example of inconsistent formatting that ends up in my code:
>>>>>>>> ---
>>>>>>>> anObject.someLongMethodName(arg_number_1,
>>>>>>>>
>>>>>>>> arg_number_2);
>>>>>>>> --- vs ---
>>>>>>>> anObject.anotherMethodName(
>>>>>>>> arg_number_1,
>>>>>>>> arg_number_2
>>>>>>>> );
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> It wasn't clear to me that the intent was to autoformat all the
>>>>>>>>> code from the proposal initially. If thats the case, then the delta is
>>>>>>>>> quite small typically.
>>>>>>>>>
>>>>>>>>> Also, it would be easier if we recommended to users to run run
>>>>>>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>>>>>>
>>>>>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>>>>>>
>>>>>>>>>> The problem you describe happens when the PR author uses
>>>>>>>>>> autoformat but the baseline is not already autoformatted. What I am
>>>>>>>>>> proposing is to make sure the baseline is already autoformatted, so PRs
>>>>>>>>>> never have extraneous formatting changes.
>>>>>>>>>>
>>>>>>>>>> Rafael: the default setting on GitHub is "allow edits by
>>>>>>>>>> maintainers" so actually a committer can run spotless on behalf of a
>>>>>>>>>> contributor and push the fixup. I have done this. It also lets a
>>>>>>>>>> committer fix up a good PR and merge it even if the contributor is, say,
>>>>>>>>>> asleep.
>>>>>>>>>>
>>>>>>>>>> Kenn
>>>>>>>>>>
>>>>>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <
>>>>>>>>>> rfernand@google.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>>>>>>> together - +1! :D
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> If spotless is run against a PR that is already well formatted
>>>>>>>>>>>> its a non-issue as the formatting changes are usually related to the change
>>>>>>>>>>>> but I have reviewed a few PRs that have 100s of lines of formatting change
>>>>>>>>>>>> which really obfuscates the work.
>>>>>>>>>>>> Instead of asking contributors to run spotless, can we have a
>>>>>>>>>>>> cron job run it across the project like once a day/week/... and cut a PR?
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused
>>>>>>>>>>>>> on the things that go beyond format.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Kenn
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> +1 !
>>>>>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless
>>>>>>>>>>>>>> diff burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Etienne
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I like readable code, but I don't like formatting it myself.
>>>>>>>>>>>>>> And I _really_ don't like discussing in code review. "Spotless" [1] can
>>>>>>>>>>>>>> enforce - and automatically apply - automatic formatting for Java, Groovy,
>>>>>>>>>>>>>> and some others.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is not about style or wanting a particular layout. This
>>>>>>>>>>>>>> is about automation, contributor experience, and streamlining review
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Automation: You want to use autoformat so you don't have
>>>>>>>>>>>>>> to format code by hand. But if you autoformat a file that was in some other
>>>>>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR
>>>>>>>>>>>>>> also needs baseline to already be autoformatted or formatting will make it
>>>>>>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>>>>>>> autoformat :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time
>>>>>>>>>>>>>> or globally. If someone conflicts with a massive autoformat commit, they
>>>>>>>>>>>>>> can just keep their changes and autoformat them and it is done.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Kenn
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Robert Burke <ro...@frantil.com>.
+1!
Given this is the recommended default for Go projects (with it's own gofmt
tool) I'm for similar tooling for this in other languages.
I suspect we can add Gradle command to run gofmt over the go code too for a
consistent hook to run for beam code. It would save folks from needing to
set up their IDEs for every language independently, any of which they may
not be used to the idiomatic conventions.
On Wed, Jun 27, 2018, 3:32 PM Kenneth Knowles <kl...@google.com> wrote:
> OK, I opened https://github.com/apache/beam/pull/5797. This thread is
> still less than a day, so there's no commitment yet. There's no big hurry -
> I can always discard the autoformat and rerun it. I'm sure there will
> always be conflicts so I will just have to rerun it and merge at some quiet
> period for the repo. I will see about pinging existing PRs.
>
> On Wed, Jun 27, 2018 at 11:31 AM Daniel Oliveira <da...@google.com>
> wrote:
>
>> +1 I'll throw in my support for auto-formatting, especially if the entire
>> project is auto-formatted in advance.
>>
>> On Wed, Jun 27, 2018 at 10:53 AM Huygaa Batsaikhan <ba...@google.com>
>> wrote:
>>
>>> +1. Global auto-formatting is cool!
>>>
>>> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> I just mean that because of how the tool works. But I guess if there
>>>> were discretion then two different people could end up with autoformatting
>>>> that disagrees, so again you get lines in the PR diff that aren't real
>>>> changes.
>>>>
>>>> Kenn
>>>>
>>>> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com>
>>>> wrote:
>>>>
>>>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Nope! No discretion allowed :-)
>>>>>>
>>>>>
>>>>> +1. Fair enough!
>>>>>
>>>>>
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> +1.
>>>>>>>
>>>>>>> Wondering if it can be configured to reformat only what we care most
>>>>>>> about (2 space indentation etc), allowing some discretion on the edges. An
>>>>>>> example of inconsistent formatting that ends up in my code:
>>>>>>> ---
>>>>>>> anObject.someLongMethodName(arg_number_1,
>>>>>>> arg_number_2);
>>>>>>> --- vs ---
>>>>>>> anObject.anotherMethodName(
>>>>>>> arg_number_1,
>>>>>>> arg_number_2
>>>>>>> );
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> It wasn't clear to me that the intent was to autoformat all the
>>>>>>>> code from the proposal initially. If thats the case, then the delta is
>>>>>>>> quite small typically.
>>>>>>>>
>>>>>>>> Also, it would be easier if we recommended to users to run run
>>>>>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>>>>>
>>>>>>>>> The problem you describe happens when the PR author uses
>>>>>>>>> autoformat but the baseline is not already autoformatted. What I am
>>>>>>>>> proposing is to make sure the baseline is already autoformatted, so PRs
>>>>>>>>> never have extraneous formatting changes.
>>>>>>>>>
>>>>>>>>> Rafael: the default setting on GitHub is "allow edits by
>>>>>>>>> maintainers" so actually a committer can run spotless on behalf of a
>>>>>>>>> contributor and push the fixup. I have done this. It also lets a
>>>>>>>>> committer fix up a good PR and merge it even if the contributor is, say,
>>>>>>>>> asleep.
>>>>>>>>>
>>>>>>>>> Kenn
>>>>>>>>>
>>>>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <
>>>>>>>>> rfernand@google.com> wrote:
>>>>>>>>>
>>>>>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>>>>>> together - +1! :D
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> If spotless is run against a PR that is already well formatted
>>>>>>>>>>> its a non-issue as the formatting changes are usually related to the change
>>>>>>>>>>> but I have reviewed a few PRs that have 100s of lines of formatting change
>>>>>>>>>>> which really obfuscates the work.
>>>>>>>>>>> Instead of asking contributors to run spotless, can we have a
>>>>>>>>>>> cron job run it across the project like once a day/week/... and cut a PR?
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused
>>>>>>>>>>>> on the things that go beyond format.
>>>>>>>>>>>>
>>>>>>>>>>>> Kenn
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> +1 !
>>>>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Etienne
>>>>>>>>>>>>>
>>>>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I like readable code, but I don't like formatting it myself.
>>>>>>>>>>>>> And I _really_ don't like discussing in code review. "Spotless" [1] can
>>>>>>>>>>>>> enforce - and automatically apply - automatic formatting for Java, Groovy,
>>>>>>>>>>>>> and some others.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is not about style or wanting a particular layout. This
>>>>>>>>>>>>> is about automation, contributor experience, and streamlining review
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR
>>>>>>>>>>>>> also needs baseline to already be autoformatted or formatting will make it
>>>>>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>>>>>> autoformat :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Kenn
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>>>>>
>>>>>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
OK, I opened https://github.com/apache/beam/pull/5797. This thread is still
less than a day, so there's no commitment yet. There's no big hurry - I can
always discard the autoformat and rerun it. I'm sure there will always be
conflicts so I will just have to rerun it and merge at some quiet period
for the repo. I will see about pinging existing PRs.
On Wed, Jun 27, 2018 at 11:31 AM Daniel Oliveira <da...@google.com>
wrote:
> +1 I'll throw in my support for auto-formatting, especially if the entire
> project is auto-formatted in advance.
>
> On Wed, Jun 27, 2018 at 10:53 AM Huygaa Batsaikhan <ba...@google.com>
> wrote:
>
>> +1. Global auto-formatting is cool!
>>
>> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <kl...@google.com> wrote:
>>
>>> I just mean that because of how the tool works. But I guess if there
>>> were discretion then two different people could end up with autoformatting
>>> that disagrees, so again you get lines in the PR diff that aren't real
>>> changes.
>>>
>>> Kenn
>>>
>>> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com>
>>> wrote:
>>>
>>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com>
>>>> wrote:
>>>>
>>>>> Nope! No discretion allowed :-)
>>>>>
>>>>
>>>> +1. Fair enough!
>>>>
>>>>
>>>>>
>>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com>
>>>>> wrote:
>>>>>
>>>>>> +1.
>>>>>>
>>>>>> Wondering if it can be configured to reformat only what we care most
>>>>>> about (2 space indentation etc), allowing some discretion on the edges. An
>>>>>> example of inconsistent formatting that ends up in my code:
>>>>>> ---
>>>>>> anObject.someLongMethodName(arg_number_1,
>>>>>> arg_number_2);
>>>>>> --- vs ---
>>>>>> anObject.anotherMethodName(
>>>>>> arg_number_1,
>>>>>> arg_number_2
>>>>>> );
>>>>>>
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>>>
>>>>>>> It wasn't clear to me that the intent was to autoformat all the code
>>>>>>> from the proposal initially. If thats the case, then the delta is quite
>>>>>>> small typically.
>>>>>>>
>>>>>>> Also, it would be easier if we recommended to users to run run
>>>>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>>>>
>>>>>>>> The problem you describe happens when the PR author uses autoformat
>>>>>>>> but the baseline is not already autoformatted. What I am proposing is to
>>>>>>>> make sure the baseline is already autoformatted, so PRs never have
>>>>>>>> extraneous formatting changes.
>>>>>>>>
>>>>>>>> Rafael: the default setting on GitHub is "allow edits by
>>>>>>>> maintainers" so actually a committer can run spotless on behalf of a
>>>>>>>> contributor and push the fixup. I have done this. It also lets a
>>>>>>>> committer fix up a good PR and merge it even if the contributor is, say,
>>>>>>>> asleep.
>>>>>>>>
>>>>>>>> Kenn
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <
>>>>>>>> rfernand@google.com> wrote:
>>>>>>>>
>>>>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>>>>> together - +1! :D
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> If spotless is run against a PR that is already well formatted
>>>>>>>>>> its a non-issue as the formatting changes are usually related to the change
>>>>>>>>>> but I have reviewed a few PRs that have 100s of lines of formatting change
>>>>>>>>>> which really obfuscates the work.
>>>>>>>>>> Instead of asking contributors to run spotless, can we have a
>>>>>>>>>> cron job run it across the project like once a day/week/... and cut a PR?
>>>>>>>>>>
>>>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused on
>>>>>>>>>>> the things that go beyond format.
>>>>>>>>>>>
>>>>>>>>>>> Kenn
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> +1 !
>>>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>>>>
>>>>>>>>>>>> Etienne
>>>>>>>>>>>>
>>>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>>>>
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> I like readable code, but I don't like formatting it myself.
>>>>>>>>>>>> And I _really_ don't like discussing in code review. "Spotless" [1] can
>>>>>>>>>>>> enforce - and automatically apply - automatic formatting for Java, Groovy,
>>>>>>>>>>>> and some others.
>>>>>>>>>>>>
>>>>>>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>>>>>>
>>>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>>>>
>>>>>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>>>>
>>>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR
>>>>>>>>>>>> also needs baseline to already be autoformatted or formatting will make it
>>>>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>>>>
>>>>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>>>>> autoformat :-)
>>>>>>>>>>>>
>>>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>>>>>
>>>>>>>>>>>> Kenn
>>>>>>>>>>>>
>>>>>>>>>>>> [1]
>>>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>>>>
>>>>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Daniel Oliveira <da...@google.com>.
+1 I'll throw in my support for auto-formatting, especially if the entire
project is auto-formatted in advance.
On Wed, Jun 27, 2018 at 10:53 AM Huygaa Batsaikhan <ba...@google.com>
wrote:
> +1. Global auto-formatting is cool!
>
> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <kl...@google.com> wrote:
>
>> I just mean that because of how the tool works. But I guess if there were
>> discretion then two different people could end up with autoformatting that
>> disagrees, so again you get lines in the PR diff that aren't real changes.
>>
>> Kenn
>>
>> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com> wrote:
>>
>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> Nope! No discretion allowed :-)
>>>>
>>>
>>> +1. Fair enough!
>>>
>>>
>>>>
>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com>
>>>> wrote:
>>>>
>>>>> +1.
>>>>>
>>>>> Wondering if it can be configured to reformat only what we care most
>>>>> about (2 space indentation etc), allowing some discretion on the edges. An
>>>>> example of inconsistent formatting that ends up in my code:
>>>>> ---
>>>>> anObject.someLongMethodName(arg_number_1,
>>>>> arg_number_2);
>>>>> --- vs ---
>>>>> anObject.anotherMethodName(
>>>>> arg_number_1,
>>>>> arg_number_2
>>>>> );
>>>>>
>>>>>
>>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>>
>>>>>> It wasn't clear to me that the intent was to autoformat all the code
>>>>>> from the proposal initially. If thats the case, then the delta is quite
>>>>>> small typically.
>>>>>>
>>>>>> Also, it would be easier if we recommended to users to run run
>>>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>>>
>>>>>>> The problem you describe happens when the PR author uses autoformat
>>>>>>> but the baseline is not already autoformatted. What I am proposing is to
>>>>>>> make sure the baseline is already autoformatted, so PRs never have
>>>>>>> extraneous formatting changes.
>>>>>>>
>>>>>>> Rafael: the default setting on GitHub is "allow edits by
>>>>>>> maintainers" so actually a committer can run spotless on behalf of a
>>>>>>> contributor and push the fixup. I have done this. It also lets a
>>>>>>> committer fix up a good PR and merge it even if the contributor is, say,
>>>>>>> asleep.
>>>>>>>
>>>>>>> Kenn
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <
>>>>>>> rfernand@google.com> wrote:
>>>>>>>
>>>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>>>> together - +1! :D
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> If spotless is run against a PR that is already well formatted its
>>>>>>>>> a non-issue as the formatting changes are usually related to the change but
>>>>>>>>> I have reviewed a few PRs that have 100s of lines of formatting change
>>>>>>>>> which really obfuscates the work.
>>>>>>>>> Instead of asking contributors to run spotless, can we have a cron
>>>>>>>>> job run it across the project like once a day/week/... and cut a PR?
>>>>>>>>>
>>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused on
>>>>>>>>>> the things that go beyond format.
>>>>>>>>>>
>>>>>>>>>> Kenn
>>>>>>>>>>
>>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>>>
>>>>>>>>>>> +1 !
>>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>>>
>>>>>>>>>>> Etienne
>>>>>>>>>>>
>>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> I like readable code, but I don't like formatting it myself. And
>>>>>>>>>>> I _really_ don't like discussing in code review. "Spotless" [1] can enforce
>>>>>>>>>>> - and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>>>>>>> others.
>>>>>>>>>>>
>>>>>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>>>>>
>>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>>>
>>>>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>>>
>>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR
>>>>>>>>>>> also needs baseline to already be autoformatted or formatting will make it
>>>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>>>
>>>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>>>> autoformat :-)
>>>>>>>>>>>
>>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>>>>
>>>>>>>>>>> Kenn
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>>>
>>>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Huygaa Batsaikhan <ba...@google.com>.
+1. Global auto-formatting is cool!
On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <kl...@google.com> wrote:
> I just mean that because of how the tool works. But I guess if there were
> discretion then two different people could end up with autoformatting that
> disagrees, so again you get lines in the PR diff that aren't real changes.
>
> Kenn
>
> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com> wrote:
>
>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com> wrote:
>>
>>> Nope! No discretion allowed :-)
>>>
>>
>> +1. Fair enough!
>>
>>
>>>
>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com> wrote:
>>>
>>>> +1.
>>>>
>>>> Wondering if it can be configured to reformat only what we care most
>>>> about (2 space indentation etc), allowing some discretion on the edges. An
>>>> example of inconsistent formatting that ends up in my code:
>>>> ---
>>>> anObject.someLongMethodName(arg_number_1,
>>>> arg_number_2);
>>>> --- vs ---
>>>> anObject.anotherMethodName(
>>>> arg_number_1,
>>>> arg_number_2
>>>> );
>>>>
>>>>
>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>
>>>>> It wasn't clear to me that the intent was to autoformat all the code
>>>>> from the proposal initially. If thats the case, then the delta is quite
>>>>> small typically.
>>>>>
>>>>> Also, it would be easier if we recommended to users to run run
>>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>>
>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>>
>>>>>> The problem you describe happens when the PR author uses autoformat
>>>>>> but the baseline is not already autoformatted. What I am proposing is to
>>>>>> make sure the baseline is already autoformatted, so PRs never have
>>>>>> extraneous formatting changes.
>>>>>>
>>>>>> Rafael: the default setting on GitHub is "allow edits by maintainers"
>>>>>> so actually a committer can run spotless on behalf of a contributor and
>>>>>> push the fixup. I have done this. It also lets a committer fix up a
>>>>>> good PR and merge it even if the contributor is, say, asleep.
>>>>>>
>>>>>> Kenn
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>>> together - +1! :D
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> If spotless is run against a PR that is already well formatted its
>>>>>>>> a non-issue as the formatting changes are usually related to the change but
>>>>>>>> I have reviewed a few PRs that have 100s of lines of formatting change
>>>>>>>> which really obfuscates the work.
>>>>>>>> Instead of asking contributors to run spotless, can we have a cron
>>>>>>>> job run it across the project like once a day/week/... and cut a PR?
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused on
>>>>>>>>> the things that go beyond format.
>>>>>>>>>
>>>>>>>>> Kenn
>>>>>>>>>
>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>>
>>>>>>>>>> +1 !
>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>>
>>>>>>>>>> Etienne
>>>>>>>>>>
>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> I like readable code, but I don't like formatting it myself. And
>>>>>>>>>> I _really_ don't like discussing in code review. "Spotless" [1] can enforce
>>>>>>>>>> - and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>>>>>> others.
>>>>>>>>>>
>>>>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>>>>
>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>>
>>>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>>
>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>>
>>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>>> autoformat :-)
>>>>>>>>>>
>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>>>
>>>>>>>>>> Kenn
>>>>>>>>>>
>>>>>>>>>> [1]
>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>>
>>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
I just mean that because of how the tool works. But I guess if there were
discretion then two different people could end up with autoformatting that
disagrees, so again you get lines in the PR diff that aren't real changes.
Kenn
On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <ra...@google.com> wrote:
> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com> wrote:
>
>> Nope! No discretion allowed :-)
>>
>
> +1. Fair enough!
>
>
>>
>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com> wrote:
>>
>>> +1.
>>>
>>> Wondering if it can be configured to reformat only what we care most
>>> about (2 space indentation etc), allowing some discretion on the edges. An
>>> example of inconsistent formatting that ends up in my code:
>>> ---
>>> anObject.someLongMethodName(arg_number_1,
>>> arg_number_2);
>>> --- vs ---
>>> anObject.anotherMethodName(
>>> arg_number_1,
>>> arg_number_2
>>> );
>>>
>>>
>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>>>
>>>> It wasn't clear to me that the intent was to autoformat all the code
>>>> from the proposal initially. If thats the case, then the delta is quite
>>>> small typically.
>>>>
>>>> Also, it would be easier if we recommended to users to run run
>>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>>
>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
>>>>
>>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>>
>>>>> The problem you describe happens when the PR author uses autoformat
>>>>> but the baseline is not already autoformatted. What I am proposing is to
>>>>> make sure the baseline is already autoformatted, so PRs never have
>>>>> extraneous formatting changes.
>>>>>
>>>>> Rafael: the default setting on GitHub is "allow edits by maintainers"
>>>>> so actually a committer can run spotless on behalf of a contributor and
>>>>> push the fixup. I have done this. It also lets a committer fix up a
>>>>> good PR and merge it even if the contributor is, say, asleep.
>>>>>
>>>>> Kenn
>>>>>
>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>>> together - +1! :D
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>>>
>>>>>>> If spotless is run against a PR that is already well formatted its a
>>>>>>> non-issue as the formatting changes are usually related to the change but I
>>>>>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>>>>>> really obfuscates the work.
>>>>>>> Instead of asking contributors to run spotless, can we have a cron
>>>>>>> job run it across the project like once a day/week/... and cut a PR?
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused on
>>>>>>>> the things that go beyond format.
>>>>>>>>
>>>>>>>> Kenn
>>>>>>>>
>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>>> echauchot@apache.org> wrote:
>>>>>>>>
>>>>>>>>> +1 !
>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>>
>>>>>>>>> Etienne
>>>>>>>>>
>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>>>>> others.
>>>>>>>>>
>>>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>>>
>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>>
>>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>>> autoformatted, it is much better.
>>>>>>>>>
>>>>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>>>>> unclear which lines are really changed.
>>>>>>>>>
>>>>>>>>> This is already available via applyJavaNature(enableSpotless:
>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle plugins. It is
>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code base.
>>>>>>>>> Personally, I think (a) every module could make a different choice if the
>>>>>>>>> main contributors feel strongly and (b) it is objectively better to always
>>>>>>>>> autoformat :-)
>>>>>>>>>
>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>>
>>>>>>>>> Kenn
>>>>>>>>>
>>>>>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>>
>>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Raghu Angadi <ra...@google.com>.
On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <kl...@google.com> wrote:
> Nope! No discretion allowed :-)
>
+1. Fair enough!
>
> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com> wrote:
>
>> +1.
>>
>> Wondering if it can be configured to reformat only what we care most
>> about (2 space indentation etc), allowing some discretion on the edges. An
>> example of inconsistent formatting that ends up in my code:
>> ---
>> anObject.someLongMethodName(arg_number_1,
>> arg_number_2);
>> --- vs ---
>> anObject.anotherMethodName(
>> arg_number_1,
>> arg_number_2
>> );
>>
>>
>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>>
>>> It wasn't clear to me that the intent was to autoformat all the code
>>> from the proposal initially. If thats the case, then the delta is quite
>>> small typically.
>>>
>>> Also, it would be easier if we recommended to users to run run
>>> "./gradlew spotlessApply" which will run spotless on all modules.
>>>
>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> Luke: the proposal here solves exactly what you are talking about.
>>>>
>>>> The problem you describe happens when the PR author uses autoformat but
>>>> the baseline is not already autoformatted. What I am proposing is to make
>>>> sure the baseline is already autoformatted, so PRs never have extraneous
>>>> formatting changes.
>>>>
>>>> Rafael: the default setting on GitHub is "allow edits by maintainers"
>>>> so actually a committer can run spotless on behalf of a contributor and
>>>> push the fixup. I have done this. It also lets a committer fix up a
>>>> good PR and merge it even if the contributor is, say, asleep.
>>>>
>>>> Kenn
>>>>
>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
>>>> wrote:
>>>>
>>>>> Luke: Anything that helps contributors and reviewers work better
>>>>> together - +1! :D
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>>
>>>>>> If spotless is run against a PR that is already well formatted its a
>>>>>> non-issue as the formatting changes are usually related to the change but I
>>>>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>>>>> really obfuscates the work.
>>>>>> Instead of asking contributors to run spotless, can we have a cron
>>>>>> job run it across the project like once a day/week/... and cut a PR?
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>>>>>> things that go beyond format.
>>>>>>>
>>>>>>> Kenn
>>>>>>>
>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>>> echauchot@apache.org> wrote:
>>>>>>>
>>>>>>>> +1 !
>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>>
>>>>>>>> Etienne
>>>>>>>>
>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>>>> others.
>>>>>>>>
>>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>>
>>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>>> of telling them to go in and manually edit.
>>>>>>>>
>>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>>> autoformatted, it is much better.
>>>>>>>>
>>>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>>>> unclear which lines are really changed.
>>>>>>>>
>>>>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>>>>> I think (a) every module could make a different choice if the main
>>>>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>>>>> autoformat :-)
>>>>>>>>
>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>>
>>>>>>>> Kenn
>>>>>>>>
>>>>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>>
>>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
Nope! No discretion allowed :-)
On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <ra...@google.com> wrote:
> +1.
>
> Wondering if it can be configured to reformat only what we care most about
> (2 space indentation etc), allowing some discretion on the edges. An
> example of inconsistent formatting that ends up in my code:
> ---
> anObject.someLongMethodName(arg_number_1,
> arg_number_2);
> --- vs ---
> anObject.anotherMethodName(
> arg_number_1,
> arg_number_2
> );
>
>
> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
>
>> It wasn't clear to me that the intent was to autoformat all the code from
>> the proposal initially. If thats the case, then the delta is quite small
>> typically.
>>
>> Also, it would be easier if we recommended to users to run run
>> "./gradlew spotlessApply" which will run spotless on all modules.
>>
>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
>>
>>> Luke: the proposal here solves exactly what you are talking about.
>>>
>>> The problem you describe happens when the PR author uses autoformat but
>>> the baseline is not already autoformatted. What I am proposing is to make
>>> sure the baseline is already autoformatted, so PRs never have extraneous
>>> formatting changes.
>>>
>>> Rafael: the default setting on GitHub is "allow edits by maintainers" so
>>> actually a committer can run spotless on behalf of a contributor and push
>>> the fixup. I have done this. It also lets a committer fix up a good PR
>>> and merge it even if the contributor is, say, asleep.
>>>
>>> Kenn
>>>
>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
>>> wrote:
>>>
>>>> Luke: Anything that helps contributors and reviewers work better
>>>> together - +1! :D
>>>>
>>>>
>>>>
>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>>>
>>>>> If spotless is run against a PR that is already well formatted its a
>>>>> non-issue as the formatting changes are usually related to the change but I
>>>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>>>> really obfuscates the work.
>>>>> Instead of asking contributors to run spotless, can we have a cron job
>>>>> run it across the project like once a day/week/... and cut a PR?
>>>>>
>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>>>>> things that go beyond format.
>>>>>>
>>>>>> Kenn
>>>>>>
>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <
>>>>>> echauchot@apache.org> wrote:
>>>>>>
>>>>>>> +1 !
>>>>>>> It's my custom to avoid reformatting to spare meaningless diff
>>>>>>> burden to the reviewer. Now it will be over, thanks.
>>>>>>>
>>>>>>> Etienne
>>>>>>>
>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>>> others.
>>>>>>>
>>>>>>> This is not about style or wanting a particular layout. This is
>>>>>>> about automation, contributor experience, and streamlining review
>>>>>>>
>>>>>>> - Contributor experience: MUCH better than checkstyle: error
>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" instead
>>>>>>> of telling them to go in and manually edit.
>>>>>>>
>>>>>>> - Automation: You want to use autoformat so you don't have to
>>>>>>> format code by hand. But if you autoformat a file that was in some other
>>>>>>> format, then you touch a bunch of unrelated lines. If the file is already
>>>>>>> autoformatted, it is much better.
>>>>>>>
>>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>>> unclear which lines are really changed.
>>>>>>>
>>>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>>>> I think (a) every module could make a different choice if the main
>>>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>>>> autoformat :-)
>>>>>>>
>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>>
>>>>>>> Kenn
>>>>>>>
>>>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>>
>>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Raghu Angadi <ra...@google.com>.
+1.
Wondering if it can be configured to reformat only what we care most about
(2 space indentation etc), allowing some discretion on the edges. An
example of inconsistent formatting that ends up in my code:
---
anObject.someLongMethodName(arg_number_1,
arg_number_2);
--- vs ---
anObject.anotherMethodName(
arg_number_1,
arg_number_2
);
On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> wrote:
> It wasn't clear to me that the intent was to autoformat all the code from
> the proposal initially. If thats the case, then the delta is quite small
> typically.
>
> Also, it would be easier if we recommended to users to run run "./gradlew spotlessApply"
> which will run spotless on all modules.
>
> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
>
>> Luke: the proposal here solves exactly what you are talking about.
>>
>> The problem you describe happens when the PR author uses autoformat but
>> the baseline is not already autoformatted. What I am proposing is to make
>> sure the baseline is already autoformatted, so PRs never have extraneous
>> formatting changes.
>>
>> Rafael: the default setting on GitHub is "allow edits by maintainers" so
>> actually a committer can run spotless on behalf of a contributor and push
>> the fixup. I have done this. It also lets a committer fix up a good PR
>> and merge it even if the contributor is, say, asleep.
>>
>> Kenn
>>
>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
>> wrote:
>>
>>> Luke: Anything that helps contributors and reviewers work better
>>> together - +1! :D
>>>
>>>
>>>
>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>>
>>>> If spotless is run against a PR that is already well formatted its a
>>>> non-issue as the formatting changes are usually related to the change but I
>>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>>> really obfuscates the work.
>>>> Instead of asking contributors to run spotless, can we have a cron job
>>>> run it across the project like once a day/week/... and cut a PR?
>>>>
>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>>>>
>>>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>>>> things that go beyond format.
>>>>>
>>>>> Kenn
>>>>>
>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>>>>> wrote:
>>>>>
>>>>>> +1 !
>>>>>> It's my custom to avoid reformatting to spare meaningless diff burden
>>>>>> to the reviewer. Now it will be over, thanks.
>>>>>>
>>>>>> Etienne
>>>>>>
>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>> others.
>>>>>>
>>>>>> This is not about style or wanting a particular layout. This is about
>>>>>> automation, contributor experience, and streamlining review
>>>>>>
>>>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>>>> telling them to go in and manually edit.
>>>>>>
>>>>>> - Automation: You want to use autoformat so you don't have to format
>>>>>> code by hand. But if you autoformat a file that was in some other format,
>>>>>> then you touch a bunch of unrelated lines. If the file is already
>>>>>> autoformatted, it is much better.
>>>>>>
>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>> unclear which lines are really changed.
>>>>>>
>>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>>> I think (a) every module could make a different choice if the main
>>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>>> autoformat :-)
>>>>>>
>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>
>>>>>> Kenn
>>>>>>
>>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>
>>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Lukasz Cwik <lc...@google.com>.
It wasn't clear to me that the intent was to autoformat all the code from
the proposal initially. If thats the case, then the delta is quite small
typically.
Also, it would be easier if we recommended to users to run run
"./gradlew spotlessApply"
which will run spotless on all modules.
On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <kl...@google.com> wrote:
> Luke: the proposal here solves exactly what you are talking about.
>
> The problem you describe happens when the PR author uses autoformat but
> the baseline is not already autoformatted. What I am proposing is to make
> sure the baseline is already autoformatted, so PRs never have extraneous
> formatting changes.
>
> Rafael: the default setting on GitHub is "allow edits by maintainers" so
> actually a committer can run spotless on behalf of a contributor and push
> the fixup. I have done this. It also lets a committer fix up a good PR
> and merge it even if the contributor is, say, asleep.
>
> Kenn
>
> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
> wrote:
>
>> Luke: Anything that helps contributors and reviewers work better together
>> - +1! :D
>>
>>
>>
>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>>
>>> If spotless is run against a PR that is already well formatted its a
>>> non-issue as the formatting changes are usually related to the change but I
>>> have reviewed a few PRs that have 100s of lines of formatting change which
>>> really obfuscates the work.
>>> Instead of asking contributors to run spotless, can we have a cron job
>>> run it across the project like once a day/week/... and cut a PR?
>>>
>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>>> things that go beyond format.
>>>>
>>>> Kenn
>>>>
>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>>>> wrote:
>>>>
>>>>> +1 !
>>>>> It's my custom to avoid reformatting to spare meaningless diff burden
>>>>> to the reviewer. Now it will be over, thanks.
>>>>>
>>>>> Etienne
>>>>>
>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>>
>>>>> Hi all,
>>>>>
>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>> others.
>>>>>
>>>>> This is not about style or wanting a particular layout. This is about
>>>>> automation, contributor experience, and streamlining review
>>>>>
>>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>>> telling them to go in and manually edit.
>>>>>
>>>>> - Automation: You want to use autoformat so you don't have to format
>>>>> code by hand. But if you autoformat a file that was in some other format,
>>>>> then you touch a bunch of unrelated lines. If the file is already
>>>>> autoformatted, it is much better.
>>>>>
>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>> unclear which lines are really changed.
>>>>>
>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>> I think (a) every module could make a different choice if the main
>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>> autoformat :-)
>>>>>
>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>> just keep their changes and autoformat them and it is done.
>>>>>
>>>>> Kenn
>>>>>
>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>
>>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
Luke: the proposal here solves exactly what you are talking about.
The problem you describe happens when the PR author uses autoformat but the
baseline is not already autoformatted. What I am proposing is to make sure
the baseline is already autoformatted, so PRs never have extraneous
formatting changes.
Rafael: the default setting on GitHub is "allow edits by maintainers" so
actually a committer can run spotless on behalf of a contributor and push
the fixup. I have done this. It also lets a committer fix up a good PR and
merge it even if the contributor is, say, asleep.
Kenn
On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez <rf...@google.com>
wrote:
> Luke: Anything that helps contributors and reviewers work better together
> - +1! :D
>
>
>
> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
>
>> If spotless is run against a PR that is already well formatted its a
>> non-issue as the formatting changes are usually related to the change but I
>> have reviewed a few PRs that have 100s of lines of formatting change which
>> really obfuscates the work.
>> Instead of asking contributors to run spotless, can we have a cron job
>> run it across the project like once a day/week/... and cut a PR?
>>
>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>>
>>> Good points, Dan. Checkstyle will still run, but just focused on the
>>> things that go beyond format.
>>>
>>> Kenn
>>>
>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>>> wrote:
>>>
>>>> +1 !
>>>> It's my custom to avoid reformatting to spare meaningless diff burden
>>>> to the reviewer. Now it will be over, thanks.
>>>>
>>>> Etienne
>>>>
>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>>
>>>> Hi all,
>>>>
>>>> I like readable code, but I don't like formatting it myself. And I
>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>> others.
>>>>
>>>> This is not about style or wanting a particular layout. This is about
>>>> automation, contributor experience, and streamlining review
>>>>
>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>> telling them to go in and manually edit.
>>>>
>>>> - Automation: You want to use autoformat so you don't have to format
>>>> code by hand. But if you autoformat a file that was in some other format,
>>>> then you touch a bunch of unrelated lines. If the file is already
>>>> autoformatted, it is much better.
>>>>
>>>> - Review: Never talk about code formatting ever again. A PR also needs
>>>> baseline to already be autoformatted or formatting will make it unclear
>>>> which lines are really changed.
>>>>
>>>> This is already available via applyJavaNature(enableSpotless: true) and
>>>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>>>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>>>> think (a) every module could make a different choice if the main
>>>> contributors feel strongly and (b) it is objectively better to always
>>>> autoformat :-)
>>>>
>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>> just keep their changes and autoformat them and it is done.
>>>>
>>>> Kenn
>>>>
>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>
>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Rafael Fernandez <rf...@google.com>.
Luke: Anything that helps contributors and reviewers work better together -
+1! :D
On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> wrote:
> If spotless is run against a PR that is already well formatted its a
> non-issue as the formatting changes are usually related to the change but I
> have reviewed a few PRs that have 100s of lines of formatting change which
> really obfuscates the work.
> Instead of asking contributors to run spotless, can we have a cron job run
> it across the project like once a day/week/... and cut a PR?
>
> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
>
>> Good points, Dan. Checkstyle will still run, but just focused on the
>> things that go beyond format.
>>
>> Kenn
>>
>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
>> wrote:
>>
>>> +1 !
>>> It's my custom to avoid reformatting to spare meaningless diff burden to
>>> the reviewer. Now it will be over, thanks.
>>>
>>> Etienne
>>>
>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>>
>>> Hi all,
>>>
>>> I like readable code, but I don't like formatting it myself. And I
>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>> others.
>>>
>>> This is not about style or wanting a particular layout. This is about
>>> automation, contributor experience, and streamlining review
>>>
>>> - Contributor experience: MUCH better than checkstyle: error message
>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>> telling them to go in and manually edit.
>>>
>>> - Automation: You want to use autoformat so you don't have to format
>>> code by hand. But if you autoformat a file that was in some other format,
>>> then you touch a bunch of unrelated lines. If the file is already
>>> autoformatted, it is much better.
>>>
>>> - Review: Never talk about code formatting ever again. A PR also needs
>>> baseline to already be autoformatted or formatting will make it unclear
>>> which lines are really changed.
>>>
>>> This is already available via applyJavaNature(enableSpotless: true) and
>>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>>> think (a) every module could make a different choice if the main
>>> contributors feel strongly and (b) it is objectively better to always
>>> autoformat :-)
>>>
>>> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
>>> If someone conflicts with a massive autoformat commit, they can just keep
>>> their changes and autoformat them and it is done.
>>>
>>> Kenn
>>>
>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>
>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Lukasz Cwik <lc...@google.com>.
If spotless is run against a PR that is already well formatted its a
non-issue as the formatting changes are usually related to the change but I
have reviewed a few PRs that have 100s of lines of formatting change which
really obfuscates the work.
Instead of asking contributors to run spotless, can we have a cron job run
it across the project like once a day/week/... and cut a PR?
On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <kl...@google.com> wrote:
> Good points, Dan. Checkstyle will still run, but just focused on the
> things that go beyond format.
>
> Kenn
>
> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
> wrote:
>
>> +1 !
>> It's my custom to avoid reformatting to spare meaningless diff burden to
>> the reviewer. Now it will be over, thanks.
>>
>> Etienne
>>
>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>>
>> Hi all,
>>
>> I like readable code, but I don't like formatting it myself. And I
>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>> and automatically apply - automatic formatting for Java, Groovy, and some
>> others.
>>
>> This is not about style or wanting a particular layout. This is about
>> automation, contributor experience, and streamlining review
>>
>> - Contributor experience: MUCH better than checkstyle: error message
>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>> telling them to go in and manually edit.
>>
>> - Automation: You want to use autoformat so you don't have to format
>> code by hand. But if you autoformat a file that was in some other format,
>> then you touch a bunch of unrelated lines. If the file is already
>> autoformatted, it is much better.
>>
>> - Review: Never talk about code formatting ever again. A PR also needs
>> baseline to already be autoformatted or formatting will make it unclear
>> which lines are really changed.
>>
>> This is already available via applyJavaNature(enableSpotless: true) and
>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>> think (a) every module could make a different choice if the main
>> contributors feel strongly and (b) it is objectively better to always
>> autoformat :-)
>>
>> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
>> If someone conflicts with a massive autoformat commit, they can just keep
>> their changes and autoformat them and it is done.
>>
>> Kenn
>>
>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>
>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Kenneth Knowles <kl...@google.com>.
Good points, Dan. Checkstyle will still run, but just focused on the things
that go beyond format.
Kenn
On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot <ec...@apache.org>
wrote:
> +1 !
> It's my custom to avoid reformatting to spare meaningless diff burden to
> the reviewer. Now it will be over, thanks.
>
> Etienne
>
> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
>
> Hi all,
>
> I like readable code, but I don't like formatting it myself. And I
> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
> and automatically apply - automatic formatting for Java, Groovy, and some
> others.
>
> This is not about style or wanting a particular layout. This is about
> automation, contributor experience, and streamlining review
>
> - Contributor experience: MUCH better than checkstyle: error message just
> says "run ./gradlew :beam-your-module:spotlessApply" instead of telling
> them to go in and manually edit.
>
> - Automation: You want to use autoformat so you don't have to format code
> by hand. But if you autoformat a file that was in some other format, then
> you touch a bunch of unrelated lines. If the file is already autoformatted,
> it is much better.
>
> - Review: Never talk about code formatting ever again. A PR also needs
> baseline to already be autoformatted or formatting will make it unclear
> which lines are really changed.
>
> This is already available via applyJavaNature(enableSpotless: true) and it
> is turned on for SQL and our buildSrc gradle plugins. It is very nice.
> There is a JIRA [2] to turn it on for the hold code base. Personally, I
> think (a) every module could make a different choice if the main
> contributors feel strongly and (b) it is objectively better to always
> autoformat :-)
>
> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
> If someone conflicts with a massive autoformat commit, they can just keep
> their changes and autoformat them and it is done.
>
> Kenn
>
> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
> [2] https://issues.apache.org/jira/browse/BEAM-4394
>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Etienne Chauchot <ec...@apache.org>.
+1 !It's my custom to avoid reformatting to spare meaningless diff burden to the reviewer. Now it will be over, thanks.
Etienne
Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit :
> Hi all,
> I like readable code, but I don't like formatting it myself. And I _really_ don't like discussing in code review.
> "Spotless" [1] can enforce - and automatically apply - automatic formatting for Java, Groovy, and some others.
>
> This is not about style or wanting a particular layout. This is about automation, contributor experience, and
> streamlining review
>
> - Contributor experience: MUCH better than checkstyle: error message just says "run ./gradlew :beam-your-
> module:spotlessApply" instead of telling them to go in and manually edit.
> - Automation: You want to use autoformat so you don't have to format code by hand. But if you autoformat a file that
> was in some other format, then you touch a bunch of unrelated lines. If the file is already autoformatted, it is much
> better.
>
> - Review: Never talk about code formatting ever again. A PR also needs baseline to already be autoformatted or
> formatting will make it unclear which lines are really changed.
>
> This is already available via applyJavaNature(enableSpotless: true) and it is turned on for SQL and our buildSrc
> gradle plugins. It is very nice. There is a JIRA [2] to turn it on for the hold code base. Personally, I think (a)
> every module could make a different choice if the main contributors feel strongly and (b) it is objectively better to
> always autoformat :-)
>
> WDYT? If we do it, it is trivial to add it module-at-a-time or globally. If someone conflicts with a massive
> autoformat commit, they can just keep their changes and autoformat them and it is done.
>
> Kenn
>
> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
> [2] https://issues.apache.org/jira/browse/BEAM-4394
>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Daniel Kulp <dk...@apache.org>.
I’m +1 to the idea of using something to handle the auto-formatting of code.
HOWEVER, I want to make it clear that this will likely NOT remove checkstyle from the builds as there are things that checkstyle checks that aren’t part of this. Variable names, type names, javadoc things, etc…. Are things that checkstyle currently handles that are not solved by this. Checkstyle can also be used to enforce some other coding issues. For example: checkstyle can mark imports of java.util.logging as a problem to link this to another thread on the list. :)
That said, the checkstyle rules for whitespace and paren padding and brace placement and such can all be turned off. They likely will have to be turned off. Last time I attempted to use spotless, I couldn’t get a set of checkstyle rules that would be a 100% match for what spotless generates.
Dan
> On Jun 27, 2018, at 3:42 PM, Ismaël Mejía <ie...@gmail.com> wrote:
>
> ++1
>
> - This solves the big mismatch of autoformatting using eclipse's google java style and the google-java-format plugin that makes a common source of unneeded diffs part of the reviews.
> - The spotless plugin makes this trivial to do and uniform.
>
> We need to update the instructions on formatting and we need to remove checkstyle and the Eclipse codestyle template from the repo too. So probably worth a JIRA/PR for this too. My preference is to do it all at once because the tool helps to do it quickly and we will have the pain only once and it will be fixed forever. Only thing that bugs me is that we will break probably most of the Java open Pull Requests. From a quicklook there are around 15 PRs from non active contributors that could end up staling if not rebased, so probably worth a message in these PRs remembering to rebase and linking the new autoformatting docs.
>
--
Daniel Kulp
dkulp@apache.org <ma...@apache.org> - http://dankulp.com/blog <http://dankulp.com/blog>
Talend Community Coder - http://coders.talend.com <http://coders.talend.com/>
Re: [DISCUSS] Automation for Java code formatting
Posted by Ismaël Mejía <ie...@gmail.com>.
++1
- This solves the big mismatch of autoformatting using eclipse's google
java style and the google-java-format plugin that makes a common source of
unneeded diffs part of the reviews.
- The spotless plugin makes this trivial to do and uniform.
We need to update the instructions on formatting and we need to remove
checkstyle and the Eclipse codestyle template from the repo too. So
probably worth a JIRA/PR for this too. My preference is to do it all at
once because the tool helps to do it quickly and we will have the pain only
once and it will be fixed forever. Only thing that bugs me is that we will
break probably most of the Java open Pull Requests. From a quicklook there
are around 15 PRs from non active contributors that could end up staling if
not rebased, so probably worth a message in these PRs remembering to rebase
and linking the new autoformatting docs.
Re: [DISCUSS] Automation for Java code formatting
Posted by Łukasz Gajowy <lu...@gmail.com>.
+1 to automating this with Gradle and never have problems with
formatting/forgetting about it again! :)
śr., 27 cze 2018 o 07:40 Tim Robertson <ti...@gmail.com>
napisał(a):
> ++1
>
>
> On Wed, Jun 27, 2018 at 7:36 AM, Ahmet Altay <al...@google.com> wrote:
>
>> +1
>>
>> This is great idea. Does anyone know a similar tool for python? I believe
>> go already has this as part of its tools with go fmt.
>>
>>
>> On Tue, Jun 26, 2018 at 9:55 PM, Ankur Goenka <go...@google.com> wrote:
>>
>>> +1
>>>
>>> Intellij can help but still formatting is an additional thing to keep in
>>> mind. Enabling auto formatting at gradle level will remove this additional
>>> thing to keep in mind.
>>>
>>> On Tue, Jun 26, 2018 at 9:49 PM Eugene Kirpichov <ki...@google.com>
>>> wrote:
>>>
>>>> +1!
>>>>
>>>> In some cases the temptation to format code manually can be quite
>>>> strong, but the ease of just re-running the formatter after any change
>>>> (especially after global changes like class/method renames) overweighs it.
>>>> I lost count of the times when I wasted a precommit because some line
>>>> became >100 characters after a refactoring. I especially love that there's
>>>> a gradle task that does this for you - I used to manually run
>>>> google-java-format-diff.
>>>>
>>>> On Tue, Jun 26, 2018 at 9:38 PM Rafael Fernandez <rf...@google.com>
>>>> wrote:
>>>>
>>>>> +1! Remove guesswork :D
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>>> others.
>>>>>>
>>>>>> This is not about style or wanting a particular layout. This is about
>>>>>> automation, contributor experience, and streamlining review
>>>>>>
>>>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>>>> telling them to go in and manually edit.
>>>>>>
>>>>>> - Automation: You want to use autoformat so you don't have to format
>>>>>> code by hand. But if you autoformat a file that was in some other format,
>>>>>> then you touch a bunch of unrelated lines. If the file is already
>>>>>> autoformatted, it is much better.
>>>>>>
>>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>>> unclear which lines are really changed.
>>>>>>
>>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>>> I think (a) every module could make a different choice if the main
>>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>>> autoformat :-)
>>>>>>
>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>>> just keep their changes and autoformat them and it is done.
>>>>>>
>>>>>> Kenn
>>>>>>
>>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>>
>>>>>>
>>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Tim Robertson <ti...@gmail.com>.
++1
On Wed, Jun 27, 2018 at 7:36 AM, Ahmet Altay <al...@google.com> wrote:
> +1
>
> This is great idea. Does anyone know a similar tool for python? I believe
> go already has this as part of its tools with go fmt.
>
>
> On Tue, Jun 26, 2018 at 9:55 PM, Ankur Goenka <go...@google.com> wrote:
>
>> +1
>>
>> Intellij can help but still formatting is an additional thing to keep in
>> mind. Enabling auto formatting at gradle level will remove this additional
>> thing to keep in mind.
>>
>> On Tue, Jun 26, 2018 at 9:49 PM Eugene Kirpichov <ki...@google.com>
>> wrote:
>>
>>> +1!
>>>
>>> In some cases the temptation to format code manually can be quite
>>> strong, but the ease of just re-running the formatter after any change
>>> (especially after global changes like class/method renames) overweighs it.
>>> I lost count of the times when I wasted a precommit because some line
>>> became >100 characters after a refactoring. I especially love that there's
>>> a gradle task that does this for you - I used to manually run
>>> google-java-format-diff.
>>>
>>> On Tue, Jun 26, 2018 at 9:38 PM Rafael Fernandez <rf...@google.com>
>>> wrote:
>>>
>>>> +1! Remove guesswork :D
>>>>
>>>>
>>>>
>>>> On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com> wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> I like readable code, but I don't like formatting it myself. And I
>>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>>> others.
>>>>>
>>>>> This is not about style or wanting a particular layout. This is about
>>>>> automation, contributor experience, and streamlining review
>>>>>
>>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>>> telling them to go in and manually edit.
>>>>>
>>>>> - Automation: You want to use autoformat so you don't have to format
>>>>> code by hand. But if you autoformat a file that was in some other format,
>>>>> then you touch a bunch of unrelated lines. If the file is already
>>>>> autoformatted, it is much better.
>>>>>
>>>>> - Review: Never talk about code formatting ever again. A PR also
>>>>> needs baseline to already be autoformatted or formatting will make it
>>>>> unclear which lines are really changed.
>>>>>
>>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>>> I think (a) every module could make a different choice if the main
>>>>> contributors feel strongly and (b) it is objectively better to always
>>>>> autoformat :-)
>>>>>
>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>>> just keep their changes and autoformat them and it is done.
>>>>>
>>>>> Kenn
>>>>>
>>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>>
>>>>>
>
Re: [DISCUSS] Automation for Java code formatting
Posted by Ahmet Altay <al...@google.com>.
+1
This is great idea. Does anyone know a similar tool for python? I believe
go already has this as part of its tools with go fmt.
On Tue, Jun 26, 2018 at 9:55 PM, Ankur Goenka <go...@google.com> wrote:
> +1
>
> Intellij can help but still formatting is an additional thing to keep in
> mind. Enabling auto formatting at gradle level will remove this additional
> thing to keep in mind.
>
> On Tue, Jun 26, 2018 at 9:49 PM Eugene Kirpichov <ki...@google.com>
> wrote:
>
>> +1!
>>
>> In some cases the temptation to format code manually can be quite strong,
>> but the ease of just re-running the formatter after any change (especially
>> after global changes like class/method renames) overweighs it. I lost count
>> of the times when I wasted a precommit because some line became >100
>> characters after a refactoring. I especially love that there's a gradle
>> task that does this for you - I used to manually run
>> google-java-format-diff.
>>
>> On Tue, Jun 26, 2018 at 9:38 PM Rafael Fernandez <rf...@google.com>
>> wrote:
>>
>>> +1! Remove guesswork :D
>>>
>>>
>>>
>>> On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> I like readable code, but I don't like formatting it myself. And I
>>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>>> others.
>>>>
>>>> This is not about style or wanting a particular layout. This is about
>>>> automation, contributor experience, and streamlining review
>>>>
>>>> - Contributor experience: MUCH better than checkstyle: error message
>>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>>> telling them to go in and manually edit.
>>>>
>>>> - Automation: You want to use autoformat so you don't have to format
>>>> code by hand. But if you autoformat a file that was in some other format,
>>>> then you touch a bunch of unrelated lines. If the file is already
>>>> autoformatted, it is much better.
>>>>
>>>> - Review: Never talk about code formatting ever again. A PR also needs
>>>> baseline to already be autoformatted or formatting will make it unclear
>>>> which lines are really changed.
>>>>
>>>> This is already available via applyJavaNature(enableSpotless: true)
>>>> and it is turned on for SQL and our buildSrc gradle plugins. It is very
>>>> nice. There is a JIRA [2] to turn it on for the hold code base. Personally,
>>>> I think (a) every module could make a different choice if the main
>>>> contributors feel strongly and (b) it is objectively better to always
>>>> autoformat :-)
>>>>
>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or
>>>> globally. If someone conflicts with a massive autoformat commit, they can
>>>> just keep their changes and autoformat them and it is done.
>>>>
>>>> Kenn
>>>>
>>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>>
>>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Ankur Goenka <go...@google.com>.
+1
Intellij can help but still formatting is an additional thing to keep in
mind. Enabling auto formatting at gradle level will remove this additional
thing to keep in mind.
On Tue, Jun 26, 2018 at 9:49 PM Eugene Kirpichov <ki...@google.com>
wrote:
> +1!
>
> In some cases the temptation to format code manually can be quite strong,
> but the ease of just re-running the formatter after any change (especially
> after global changes like class/method renames) overweighs it. I lost count
> of the times when I wasted a precommit because some line became >100
> characters after a refactoring. I especially love that there's a gradle
> task that does this for you - I used to manually run
> google-java-format-diff.
>
> On Tue, Jun 26, 2018 at 9:38 PM Rafael Fernandez <rf...@google.com>
> wrote:
>
>> +1! Remove guesswork :D
>>
>>
>>
>> On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com> wrote:
>>
>>> Hi all,
>>>
>>> I like readable code, but I don't like formatting it myself. And I
>>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>>> and automatically apply - automatic formatting for Java, Groovy, and some
>>> others.
>>>
>>> This is not about style or wanting a particular layout. This is about
>>> automation, contributor experience, and streamlining review
>>>
>>> - Contributor experience: MUCH better than checkstyle: error message
>>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>>> telling them to go in and manually edit.
>>>
>>> - Automation: You want to use autoformat so you don't have to format
>>> code by hand. But if you autoformat a file that was in some other format,
>>> then you touch a bunch of unrelated lines. If the file is already
>>> autoformatted, it is much better.
>>>
>>> - Review: Never talk about code formatting ever again. A PR also needs
>>> baseline to already be autoformatted or formatting will make it unclear
>>> which lines are really changed.
>>>
>>> This is already available via applyJavaNature(enableSpotless: true) and
>>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>>> think (a) every module could make a different choice if the main
>>> contributors feel strongly and (b) it is objectively better to always
>>> autoformat :-)
>>>
>>> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
>>> If someone conflicts with a massive autoformat commit, they can just keep
>>> their changes and autoformat them and it is done.
>>>
>>> Kenn
>>>
>>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>>
>>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Eugene Kirpichov <ki...@google.com>.
+1!
In some cases the temptation to format code manually can be quite strong,
but the ease of just re-running the formatter after any change (especially
after global changes like class/method renames) overweighs it. I lost count
of the times when I wasted a precommit because some line became >100
characters after a refactoring. I especially love that there's a gradle
task that does this for you - I used to manually run
google-java-format-diff.
On Tue, Jun 26, 2018 at 9:38 PM Rafael Fernandez <rf...@google.com>
wrote:
> +1! Remove guesswork :D
>
>
>
> On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com> wrote:
>
>> Hi all,
>>
>> I like readable code, but I don't like formatting it myself. And I
>> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
>> and automatically apply - automatic formatting for Java, Groovy, and some
>> others.
>>
>> This is not about style or wanting a particular layout. This is about
>> automation, contributor experience, and streamlining review
>>
>> - Contributor experience: MUCH better than checkstyle: error message
>> just says "run ./gradlew :beam-your-module:spotlessApply" instead of
>> telling them to go in and manually edit.
>>
>> - Automation: You want to use autoformat so you don't have to format
>> code by hand. But if you autoformat a file that was in some other format,
>> then you touch a bunch of unrelated lines. If the file is already
>> autoformatted, it is much better.
>>
>> - Review: Never talk about code formatting ever again. A PR also needs
>> baseline to already be autoformatted or formatting will make it unclear
>> which lines are really changed.
>>
>> This is already available via applyJavaNature(enableSpotless: true) and
>> it is turned on for SQL and our buildSrc gradle plugins. It is very nice.
>> There is a JIRA [2] to turn it on for the hold code base. Personally, I
>> think (a) every module could make a different choice if the main
>> contributors feel strongly and (b) it is objectively better to always
>> autoformat :-)
>>
>> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
>> If someone conflicts with a massive autoformat commit, they can just keep
>> their changes and autoformat them and it is done.
>>
>> Kenn
>>
>> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
>> [2] https://issues.apache.org/jira/browse/BEAM-4394
>>
>>
Re: [DISCUSS] Automation for Java code formatting
Posted by Rafael Fernandez <rf...@google.com>.
+1! Remove guesswork :D
On Tue, Jun 26, 2018 at 9:15 PM Kenneth Knowles <kl...@google.com> wrote:
> Hi all,
>
> I like readable code, but I don't like formatting it myself. And I
> _really_ don't like discussing in code review. "Spotless" [1] can enforce -
> and automatically apply - automatic formatting for Java, Groovy, and some
> others.
>
> This is not about style or wanting a particular layout. This is about
> automation, contributor experience, and streamlining review
>
> - Contributor experience: MUCH better than checkstyle: error message just
> says "run ./gradlew :beam-your-module:spotlessApply" instead of telling
> them to go in and manually edit.
>
> - Automation: You want to use autoformat so you don't have to format code
> by hand. But if you autoformat a file that was in some other format, then
> you touch a bunch of unrelated lines. If the file is already autoformatted,
> it is much better.
>
> - Review: Never talk about code formatting ever again. A PR also needs
> baseline to already be autoformatted or formatting will make it unclear
> which lines are really changed.
>
> This is already available via applyJavaNature(enableSpotless: true) and it
> is turned on for SQL and our buildSrc gradle plugins. It is very nice.
> There is a JIRA [2] to turn it on for the hold code base. Personally, I
> think (a) every module could make a different choice if the main
> contributors feel strongly and (b) it is objectively better to always
> autoformat :-)
>
> WDYT? If we do it, it is trivial to add it module-at-a-time or globally.
> If someone conflicts with a massive autoformat commit, they can just keep
> their changes and autoformat them and it is done.
>
> Kenn
>
> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
> [2] https://issues.apache.org/jira/browse/BEAM-4394
>
>