google-java-format: how to work with it?

[Saša Živkov]

Now that [1] is submitted how do we proceed?

For contributions, I guess everyone is expected to run the google-java-format over all

changed files. Those already doing that: could you share some details of your local setup?

How do you automate?

For reviewers: does it mean that we no longer comment on nits and, in general, coding style?

This was definitely one of the goals of [1].

Can we trust that everyone will apply google-java-format before pushing? I think we can not.

Shall we have an automated code-style check which basically fetches the change,

checks it out, runs google-java-format over changed files and if there is a diff then

vote CR-1 or V-1 or Code-Style-1?

Shall the code-style-checker upload a new PS automatically if the change wasn't properly

formatted?

Saša

[1] https://gerrit-review.googlesource.com/91444

[Edwin Kempin]

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:

Now that [1] is submitted how do we proceed?

For contributions, I guess everyone is expected to run the google-java-format over all

changed files. Those already doing that: could you share some details of your local setup?

How do you automate?

For now I have a shell alias for 

  git show --diff-filter=AM --pretty="" --name-only HEAD | grep java$ | xargs google-java-format -i && git add . && git commit --amend --no-edit

that I run manually before pushing one commit.

I'm still looking for a command that can reformat uncommited added/modified files in the working tree (e.g. to be used during interactive rebase).

 

For reviewers: does it mean that we no longer comment on nits and, in general, coding style?

Yes, whatever the auto-formatter does is right. You still can comment if it's different from what the auto-formatter does until we have some automatic check for this.

 

This was definitely one of the goals of [1].

Can we trust that everyone will apply google-java-format before pushing? I think we can not.

Shall we have an automated code-style check which basically fetches the change,

checks it out, runs google-java-format over changed files and if there is a diff then

vote CR-1 or V-1 or Code-Style-1?

+1

 

Shall the code-style-checker upload a new PS automatically if the change wasn't properly

formatted?

Not sure about this. But once we have applyable fixes, the code-style-checker can create fixes and the author can accept them from the UI.

 

Saša

[1] https://gerrit-review.googlesource.com/91444

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Saša Živkov]

On Tue, Feb 7, 2017 at 1:37 PM, Edwin Kempin <eke...@google.com> wrote:

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:

Now that [1] is submitted how do we proceed?

For contributions, I guess everyone is expected to run the google-java-format over all

changed files. Those already doing that: could you share some details of your local setup?

How do you automate?

For now I have a shell alias for 

  git show --diff-filter=AM --pretty="" --name-only HEAD | grep java$ | xargs google-java-format -i && git add . && git commit --amend --no-edit

The "git add ." part of the alias will also add those files which are not yet committed (dirty or untracked)?

 

that I run manually before pushing one commit.

I'm still looking for a command that can reformat uncommited added/modified files in the working tree (e.g. to be used during interactive rebase).

 

For reviewers: does it mean that we no longer comment on nits and, in general, coding style?

Yes, whatever the auto-formatter does is right. You still can comment if it's different from what the auto-formatter does until we have some automatic check for this.

 

This was definitely one of the goals of [1].

Can we trust that everyone will apply google-java-format before pushing? I think we can not.

Shall we have an automated code-style check which basically fetches the change,

checks it out, runs google-java-format over changed files and if there is a diff then

vote CR-1 or V-1 or Code-Style-1?

+1

Luca, could you setup a code style checker?

 

 

Shall the code-style-checker upload a new PS automatically if the change wasn't properly

formatted?

Not sure about this. But once we have applyable fixes, the code-style-checker can create fixes and the author can accept them from the UI.

Will the author have any choice?

If the author doesn't apply a code-formatting fix then the next PS will again get a -1 from the code-style checker.

Eventually the author must accept all code-formatting fixes... so why giving a false sense of having a choice?

[Han-Wen Nienhuys]

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:

> Now that [1] is submitted how do we proceed?
>
> For contributions, I guess everyone is expected to run the
> google-java-format over all
> changed files. Those already doing that: could you share some details of
> your local setup?
> How do you automate?
>
> For reviewers: does it mean that we no longer comment on nits and, in
> general, coding style?
> This was definitely one of the goals of [1].
> Can we trust that everyone will apply google-java-format before pushing? I
> think we can not.
> Shall we have an automated code-style check which basically fetches the
> change,
> checks it out, runs google-java-format over changed files and if there is a
> diff then
> vote CR-1 or V-1 or Code-Style-1?

it would be nice to have an automated check, but the upside of the
automated formatter is that even if an badly formatted file slips
through, it can be fixed automatically in a simple follow-up change
(similar to how people keep fixing unused imports on a regular basis.)

> Shall the code-style-checker upload a new PS automatically if the change
> wasn't properly
> formatted?

-1 - Dealing with changes by others makes the workflow harder to
manage, so I think we should avoid it if we can.

--
Han-Wen Nienhuys
Google Munich
han...@google.com

[Edwin Kempin]

On Tue, Feb 7, 2017 at 2:22 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Tue, Feb 7, 2017 at 1:37 PM, Edwin Kempin <eke...@google.com> wrote:

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:

Now that [1] is submitted how do we proceed?

For contributions, I guess everyone is expected to run the google-java-format over all

changed files. Those already doing that: could you share some details of your local setup?

How do you automate?

For now I have a shell alias for 

  git show --diff-filter=AM --pretty="" --name-only HEAD | grep java$ | xargs google-java-format -i && git add . && git commit --amend --no-edit

The "git add ." part of the alias will also add those files which are not yet committed (dirty or untracked)?

Sure. It's just what I use right now. I'm not saying that this should become the documented best practice :)

[Saša Živkov]

On Tue, Feb 7, 2017 at 2:29 PM, Edwin Kempin <eke...@google.com> wrote:

On Tue, Feb 7, 2017 at 2:22 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Tue, Feb 7, 2017 at 1:37 PM, Edwin Kempin <eke...@google.com> wrote:

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:

Now that [1] is submitted how do we proceed?

For contributions, I guess everyone is expected to run the google-java-format over all

changed files. Those already doing that: could you share some details of your local setup?

How do you automate?

For now I have a shell alias for 

  git show --diff-filter=AM --pretty="" --name-only HEAD | grep java$ | xargs google-java-format -i && git add . && git commit --amend --no-edit

The "git add ." part of the alias will also add those files which are not yet committed (dirty or untracked)?

Sure. It's just what I use right now. I'm not saying that this should become the documented best practice :)

Yes, I understand :-)

Since we may include something like your alias in our best practices I am just posting review comments :-)

[Saša Živkov]

On Tue, Feb 7, 2017 at 2:23 PM, Han-Wen Nienhuys <han...@google.com> wrote:

On Tue, Feb 7, 2017 at 1:07 PM, Saša Živkov <ziv...@gmail.com> wrote:
> Now that [1] is submitted how do we proceed?
>
> For contributions, I guess everyone is expected to run the
> google-java-format over all
> changed files. Those already doing that: could you share some details of
> your local setup?
> How do you automate?
>
> For reviewers: does it mean that we no longer comment on nits and, in
> general, coding style?
> This was definitely one of the goals of [1].
> Can we trust that everyone will apply google-java-format before pushing? I
> think we can not.
> Shall we have an automated code-style check which basically fetches the
> change,
> checks it out, runs google-java-format over changed files and if there is a
> diff then
> vote CR-1 or V-1 or Code-Style-1?

it would be nice to have an automated check, but the upside of the
automated formatter is that even if an badly formatted file slips
through, it can be fixed automatically in a simple follow-up change
(similar to how people keep fixing unused imports on a regular basis.)

Wouldn't this require more attention from each contributor before pushing

as he would have to check if any (unrelated) automated reformatting slipped

into his change and, if yes, split the commit into two: reformatting the existing code + the real change?

[Han-Wen Nienhuys]

Now that we have GJF, formatting becomes irrelevant, so I don't think
it's a big problem if a CL has some unrelated formatting.

We could recommend using

https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py

which will only reformat changed lines.

> --
> --
> To unsubscribe, email repo-discuss...@googlegroups.com

> More info at http://groups.google.com/group/repo-discuss?hl=en
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Repo and Gerrit Discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to repo-discuss...@googlegroups.com.

> For more options, visit https://groups.google.com/d/optout.

[Saša Živkov]

On Tue, Feb 7, 2017 at 2:42 PM, Han-Wen Nienhuys <han...@google.com> wrote:

Now that we have GJF, formatting becomes irrelevant, so I don't think
it's a big problem if a CL has some unrelated formatting.

Sounds like a fundamental change in our community attitude, IMHO.

How many times we told to contributors not to mix (unrelated) reformatting with the real code change?

It makes git blame much less usable, makes the diff larger (as diff operates on text and not on Java syntax trees)

and cherry-pick may generate unrelated conflicts.

After all, the whole purpose of the monster reformatting change [1] was to have minimal diff in follow-up

changes, which require GJF. Right?

Or, is it only me still having issues with unrelated reformatting?

We could recommend using

 https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py

which will only reformat changed lines.

Isn't it even more important to reject changes which don't format code according to GJF?

Since GJF is formally defined, checking whether a change conforms to GJF can be fully automated

and I thought that exactly this was meant by the promise to save reviewers time (in the

commit message of [1]).

 

> --
> --
> To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

> More info at http://groups.google.com/group/repo-discuss?hl=en
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Repo and Gerrit Discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to repo-discuss+unsubscribe@googlegroups.com.

> For more options, visit https://groups.google.com/d/optout.

[Han-Wen Nienhuys]

just to be extra clear: I'm completely in favor of checking the
formatting automatically.

I'm just trying to say that formatting is now not very important to
review for or worry about anymore, since we automated it.

>> > To unsubscribe, email repo-discuss...@googlegroups.com

>> > More info at http://groups.google.com/group/repo-discuss?hl=en
>> >
>> > ---
>> > You received this message because you are subscribed to the Google
>> > Groups
>> > "Repo and Gerrit Discussion" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> > an

>> > email to repo-discuss...@googlegroups.com.

>> > For more options, visit https://groups.google.com/d/optout.
>>
>>
>>
>> --
>> Han-Wen Nienhuys
>> Google Munich
>> han...@google.com
>
>

[Edwin Kempin]

On Tue, Feb 7, 2017 at 4:55 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Tue, Feb 7, 2017 at 2:42 PM, Han-Wen Nienhuys <han...@google.com> wrote:

Now that we have GJF, formatting becomes irrelevant, so I don't think
it's a big problem if a CL has some unrelated formatting.

Sounds like a fundamental change in our community attitude, IMHO.

How many times we told to contributors not to mix (unrelated) reformatting with the real code change?

It makes git blame much less usable, makes the diff larger (as diff operates on text and not on Java syntax trees)

and cherry-pick may generate unrelated conflicts.

After all, the whole purpose of the monster reformatting change [1] was to have minimal diff in follow-up

changes, which require GJF. Right?

Or, is it only me still having issues with unrelated reformatting?

No, I also think that changes shouldn't contain unrelated reformattings.

[David Ostrovsky]

Am Dienstag, 7. Februar 2017 17:04:59 UTC+1 schrieb Edwin Kempin:

On Tue, Feb 7, 2017 at 4:55 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Tue, Feb 7, 2017 at 2:42 PM, Han-Wen Nienhuys <han...@google.com> wrote:

Now that we have GJF, formatting becomes irrelevant, so I don't think
it's a big problem if a CL has some unrelated formatting.

Sounds like a fundamental change in our community attitude, IMHO.

How many times we told to contributors not to mix (unrelated) reformatting with the real code change?

It makes git blame much less usable, makes the diff larger (as diff operates on text and not on Java syntax trees)

and cherry-pick may generate unrelated conflicts.

After all, the whole purpose of the monster reformatting change [1] was to have minimal diff in follow-up

changes, which require GJF. Right?

Or, is it only me still having issues with unrelated reformatting?

No, I also think that changes shouldn't contain unrelated reformattings.

+1. I will always reject such a change. 

[Luca Milanesio]

Shall we execute the java formatting check on the CI and provide a specific feedback for that?

Luca.

[Dave Borowitz]

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

This suggests that we should adapt setup_gjf.sh to also set up a wrapper for google-java-diff-format.py[1]. This tool lets you format only changed lines according to "git diff". 

Looks like upstream also needs a way to pass a path to gjf on the command line, that would be a very simple patch.

[1] https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py

[Saša Živkov]

On Tue, Feb 7, 2017 at 10:15 PM, Luca Milanesio <luca.mi...@gmail.com> wrote:

Shall we execute the java formatting check on the CI and provide a specific feedback for that?

+1

Yes, I vote for that.

 

Luca.

To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google
Groups
"Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send
an

email to repo-discuss+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
Han-Wen Nienhuys
Google Munich
han...@google.com

-- 
Han-Wen Nienhuys
Google Munich
han...@google.com

-- 
-- 

To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[David Ostrovsky]

On Tuesday, February 7, 2017 at 10:27:35 PM UTC+1, Dave Borowitz wrote:

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

Thanks, Dave, checked it out, executed ./tools/setup_gjf.sh, added an alias

as it was suggested and now I can run google-java-format.

On Tue, Feb 7, 2017 at 11:29 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Dienstag, 7. Februar 2017 17:04:59 UTC+1 schrieb Edwin Kempin:

On Tue, Feb 7, 2017 at 4:55 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Tue, Feb 7, 2017 at 2:42 PM, Han-Wen Nienhuys <han...@google.com> wrote:

Now that we have GJF, formatting becomes irrelevant, so I don't think
it's a big problem if a CL has some unrelated formatting.

Sounds like a fundamental change in our community attitude, IMHO.

How many times we told to contributors not to mix (unrelated) reformatting with the real code change?

It makes git blame much less usable, makes the diff larger (as diff operates on text and not on Java syntax trees)

and cherry-pick may generate unrelated conflicts.

After all, the whole purpose of the monster reformatting change [1] was to have minimal diff in follow-up

changes, which require GJF. Right?

Or, is it only me still having issues with unrelated reformatting?

No, I also think that changes shouldn't contain unrelated reformattings.

+1. I will always reject such a change. 

This suggests that we should adapt setup_gjf.sh to also set up a wrapper for google-java-diff-format.py[1]. This tool lets you format only changed lines according to "git diff". 

Looks like upstream also needs a way to pass a path to gjf on the command line, that would be a very simple patch.

+1.

 

[1] https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py

--
--
To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

[David Ostrovsky]

On Tuesday, February 7, 2017 at 10:31:01 PM UTC+1, zivkov wrote:

On Tue, Feb 7, 2017 at 10:15 PM, Luca Milanesio <luca.mi...@gmail.com> wrote:

Shall we execute the java formatting check on the CI and provide a specific feedback for that?

+1

Yes, I vote for that.

+1.

I wouldn't expect any magic here and any automated fixes, beside that

(well a bit magic ;), just check the commit and vote on newly introduced

label: "Code-Style". Or alternatively, abuse Verify label for that, run tests,

and vote combined vote on Code-Style + Tests, so that it's clear from

reading CI comment what was wrong. To me it's less sexy as dedicated

label, though.

[Saša Živkov]

On Tue, Feb 7, 2017 at 10:27 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

Doesn't work on Mac (OSX 10.11.3) out of the box, because the readlink command on Mac doesn't support the -f option the same way as Linux:

$ google-java-format

readlink: illegal option -- f

usage: readlink [-n] [file ...] 

Looks like installing coreutils would help [1] but then the command is named greadlink on Mac :-/

Any ideas how to make script work on both Linux & Mac out of the box?

NOTE: google-java-format installed with brew works but it would be nice if google-java-format installed by the tools/setup_gjf.sh works too.

[1] https://www.mac-help.com/threads/equivalent-of-gnus-readlink-f-for-os-x.202979/

[David Ostrovsky]

Am Mittwoch, 8. Februar 2017 10:02:18 UTC+1 schrieb zivkov:

On Tue, Feb 7, 2017 at 10:27 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

Doesn't work on Mac (OSX 10.11.3) out of the box, because the readlink command on Mac doesn't support the -f option the same way as Linux:

$ google-java-format

readlink: illegal option -- f

usage: readlink [-n] [file ...] 

Looks like installing coreutils would help [1] but then the command is named greadlink on Mac :-/

Any ideas how to make script work on both Linux & Mac out of the box?

Similar issue was fixed in context of this bug: [1] with this commit in Buck: [2].

* [1] https://github.com/facebook/buck/issues/1017

* [2] https://github.com/facebook/buck/commit/f97b55c23cf1af6762636f76a9dc89eb47989431 

[Saša Živkov]

On Wed, Feb 8, 2017 at 10:23 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Mittwoch, 8. Februar 2017 10:02:18 UTC+1 schrieb zivkov:

On Tue, Feb 7, 2017 at 10:27 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

Doesn't work on Mac (OSX 10.11.3) out of the box, because the readlink command on Mac doesn't support the -f option the same way as Linux:

$ google-java-format

readlink: illegal option -- f

usage: readlink [-n] [file ...] 

Looks like installing coreutils would help [1] but then the command is named greadlink on Mac :-/

Any ideas how to make script work on both Linux & Mac out of the box?

Similar issue was fixed in context of this bug: [1] with this commit in Buck: [2].

Thanks David. I will try to apply this fix today.

 

* [1] https://github.com/facebook/buck/issues/1017

* [2] https://github.com/facebook/buck/commit/f97b55c23cf1af6762636f76a9dc89eb47989431 

--

[David Ostrovsky]

Am Dienstag, 7. Februar 2017 13:08:15 UTC+1 schrieb zivkov:

Now that [1] is submitted how do we proceed?

Once this PR: [1] is merged and new version of gjf is released,

and the Eclipse plugin is installed, at least for Eclipse IDE users

this should be as easy as pressing Ctrl+Shift+F, done.

* [1] https://github.com/google/google-java-format/pull/118

[Saša Živkov]

On Wed, Feb 8, 2017 at 11:29 AM, Saša Živkov <ziv...@gmail.com> wrote:

On Wed, Feb 8, 2017 at 10:23 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Mittwoch, 8. Februar 2017 10:02:18 UTC+1 schrieb zivkov:

On Tue, Feb 7, 2017 at 10:27 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

This morning I updated dev-contributing.txt with instructions for using google-java-format. Please try them out and see if you like them.

Doesn't work on Mac (OSX 10.11.3) out of the box, because the readlink command on Mac doesn't support the -f option the same way as Linux:

$ google-java-format

readlink: illegal option -- f

usage: readlink [-n] [file ...] 

Looks like installing coreutils would help [1] but then the command is named greadlink on Mac :-/

Any ideas how to make script work on both Linux & Mac out of the box?

Similar issue was fixed in context of this bug: [1] with this commit in Buck: [2].

Thanks David. I will try to apply this fix today.

Ready:

https://gerrit-review.googlesource.com/96695 

[lucamilanesio]

Thanks to DavidO, the CI is now posting proper CodeStyle feedback for every change.

If your change does not need the CodeStyle formatting guidelines (Google Java Format), your change may get a feedback similar to:

GerritForge CI posted comments on this change.

View Change

Patch set 4:Code-Style -1

❌ The following files need formatting:
    gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java
    (https://gerrit-ci.gerritforge.com/job/Gerrit-codestyle/136/console)

Fixing the formatting is mandatory :-)

Kudos to DavidO for making this happen.

Luca.

To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google
Groups
"Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send
an

email to repo-discuss+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
Han-Wen Nienhuys
Google Munich
han...@google.com

-- 
Han-Wen Nienhuys
Google Munich
han...@google.com

-- 
-- 

To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[luca.mi...@gmail.com]

On 6 Jul 2017, at 23:54, lucamilanesio <luca.mi...@gmail.com> wrote:

Thanks to DavidO, the CI is now posting proper CodeStyle feedback for every change.

If your change does not need

/need/meet :-)

To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.