Forced "git cl format"?

[Brian White]

I now see try-bot presubmit failures for anything in the files I'm touching that doesn't exactly meet the way "git cl format" would write it.

Is it supposed to do this?  "git cl format" is great and I use it often but shouldn't we leave the final decision to the developers (and reviewers) writing the code?

While "git cl format" is great for fixing indenting and wrapping parameter lists, I find it somewhat narrow-minded in cases where there is correlation across multiple statements.  For example:

• related code:  Sometimes two or more blocks of code will be related and it's useful to highlight those relations by formatting them the same.  Small differences in name lengths can cause auto-formatting do show them completely differently.
• complex constants:  Though mostly in unit-tests, complex structure constants can get mangled in ways that make them more difficult to understand and maintain.

  Brian

  bcw...@google.com
-----------------------------------------------------------------------------------------
Treat someone as they are and they will remain that way.
Treat someone as they can be and they will become that way.

[Colin Blundell]

OWNERS of certain directories in the codebase have run 'git cl format' over all their code and then turned on a PRESUBMIT check that all new code is formatted according to 'git cl format', consciously deciding to welcome their new auto-formatting overlord with its pluses and minuses. Presumably you're in one of these directories?

Best,

Colin

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

As Colin says, OWNERS can opt in to it. Note that chrome-wide automated refactorings usually run `git cl format` on the lines they touch, so even if you decide on a non-clang-formatted formatting for some code in a directory where that's permitted, the next chrome-wide refactoring might change your code anyways. So I'd recommend that you write your code so that it looks fine when auto-formatted, and that you file bugs for cases where clang-format produces something illegible.

Nico

[Brian White]

I'm working in base/.  If this is to be the way it is...  Why even make it a failure case?  Just do the format in the background during upload or commit or whenever.  Automated tools should be automated.

It's not that the auto-format is illegible.  It's just not ideal in all cases.  Couldn't this just be a warning that developers can choose to override if they feel it's worth it?  The reminder to run the tool should be sufficient.  I'd like to think that we're all sufficiently talented that, when presented with a possibly better option, we can pick & choose between styles to produce easily readable code.

If the presubmit automatically ran something like this...

  git cl format && git add -p && git checkout -- . && git ci -a -m "some 'git cl format' changes"

... to automatically and immediately provide the suggestions then developers would generally accept those suggestions unless they had good reasons not to.  You'd get the same consistently formatted code overall while giving a nod to the fact that us meatheads are still, occasionally, smarter than the machine.

-- Brian

--

[Torne (Richard Coles)]

But if you format your code carefully and then I come by and change all your parameter types as part of a refactoring and run "git cl format" then your effort was wasted, and this happens more often than you'd think.

Not having discussions over whether a piece of code is formatted correctly (by having the machine pick the format for you) generally seems like a larger benefit than being able to format occasional cases slightly nicer.

[Colin Blundell]

On Tue, Nov 15, 2016 at 4:33 PM 'Brian White' via Chromium-dev <chromi...@chromium.org> wrote:

I'm working in base/.  If this is to be the way it is...  Why even make it a failure case?  Just do the format in the background during upload or commit or whenever.  Automated tools should be automated.

Presubmit checks aren't allowed to perform mutations (e.g., the check that says there's whitespace at the end of a line could easily just remove the whitespace). I don't recall the precise reasoning for this restriction offhand.

 

It's not that the auto-format is illegible.  It's just not ideal in all cases.  Couldn't this just be a warning that developers can choose to override if they feel it's worth it?  The reminder to run the tool should be sufficient.  I'd like to think that we're all sufficiently talented that, when presented with a possibly better option, we can pick & choose between styles to produce easily readable code. 

 

Different people feel differently about this (e.g., I'm on the side that never having to worry about formatting as an author or a reviewer is such a huge win that I can tolerate the occasional oddnesses that result). Different OWNERS also feel differently, which is why not every directory has this check enforced :).

[Colin Blundell]

On Tue, Nov 15, 2016 at 4:41 PM Colin Blundell <blun...@chromium.org> wrote:

On Tue, Nov 15, 2016 at 4:33 PM 'Brian White' via Chromium-dev <chromi...@chromium.org> wrote:

I'm working in base/.  If this is to be the way it is...  Why even make it a failure case?  Just do the format in the background during upload or commit or whenever.  Automated tools should be automated.

Presubmit checks aren't allowed to perform mutations (e.g., the check that says there's whitespace at the end of a line could easily just remove the whitespace). I don't recall the precise reasoning for this restriction offhand.

That said, if we reach the point where clang-format is enforced over the whole codebase, perhaps we would work it in automatically somewhere into the workflow.

[Brian White]

But if you format your code carefully and then I come by and change all your parameter types as part of a refactoring and run "git cl format" then your effort was wasted, and this happens more often than you'd think.

You know...  I'm willing to trust you to make an informed decision.  If you decided to change the formatting on parts of code to the auto-formatted version even if you haven't touched it, then okay.  Presumably you have your reasons and I'm willing to give you freedom to make those choices.

As it is, nobody is being trusted to make an informed decision.

-- Brian

[Ian Clelland]

On Tue, Nov 15, 2016 at 8:44 AM, 'Brian White' via Chromium-dev <chromi...@chromium.org> wrote:

• complex constants:  Though mostly in unit-tests, complex structure constants can get mangled in ways that make them more difficult to understand and maintain.

This has been discussed before(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/H8tYKcetScU,

https://groups.google,.com/a/chromium.org/forum/#!topic/chromium-dev/BPXUZCcn9hU) but for this situation, would it make sense to use

// clang-format off

to disable cl format for a block of code?

I'm not going to wade into the discussion about the general case case (or for related code blocks specifically), but for laying out constants in unit tests for readability, it seems perfectly reasonable. It's used fairly frequently in gpu, net, WebKit, v8 -- mostly for tests -- and doesn't seem be abused.

[Nico Weber]

On Tue, Nov 15, 2016 at 10:53 AM, Ian Clelland <icle...@chromium.org> wrote:

On Tue, Nov 15, 2016 at 8:44 AM, 'Brian White' via Chromium-dev <chromi...@chromium.org> wrote:

• complex constants:  Though mostly in unit-tests, complex structure constants can get mangled in ways that make them more difficult to understand and maintain.

This has been discussed before(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/H8tYKcetScU,

https://groups.google,.com/a/chromium.org/forum/#!topic/chromium-dev/BPXUZCcn9hU) but for this situation, would it make sense to use

// clang-format off

to disable cl format for a block of code?

Please use this judiciously. Every time you feel like you want to use this comment, consider filing a clang-format bug instead. Every `clang-format off` block is a block that will usually look worse after a large-scale refactoring, and having too many of them makes large-scale changes difficult. Looking over codesearch results, some of them seem unnecessary too (but to be far, many look like good uses). For reference, in all of chrome's non-third-party non-v8 non-generated code there are currently 42 uses of `clang-format off`.

 

I'm not going to wade into the discussion about the general case case (or for related code blocks specifically), but for laying out constants in unit tests for readability, it seems perfectly reasonable. It's used fairly frequently in gpu, net, WebKit, v8 -- mostly for tests -- and doesn't seem be abused.

[Torne (Richard Coles)]

On Tue, 15 Nov 2016 at 15:45 Brian White <bcw...@google.com> wrote:

But if you format your code carefully and then I come by and change all your parameter types as part of a refactoring and run "git cl format" then your effort was wasted, and this happens more often than you'd think.

You know...  I'm willing to trust you to make an informed decision.  If you decided to change the formatting on parts of code to the auto-formatted version even if you haven't touched it, then okay.  Presumably you have your reasons and I'm willing to give you freedom to make those choices.

I'm explicitly *not* making an informed decision. The cases where I've done this I've run a python script full of regexes over the code to automate a refactoring, and then run "git cl format" to rewrap all the affected lines and realign everything. I expect other large automated refactorings work much the same way, just perhaps with a more sensible way to make the original change.