Auto-formatting python

[Andrew Grieve]

"git cl format" recently learned how to auto-format python code (crbug/846432).

You can use it today via:

git cl format --python  # Formats changed lines only

git cl format --python --full # Formats entire file

I'd personally like to have this enforced by our PRESUBMIT, to cut down on format-related code review comments. 

Does anyone have concerns about this?

As for how to enable this, one idea is:

* If a project has a .style.yapf file in their root, then enable auto-formatting without --python flags and include in presubmit check.

* Otherwise, don't enable it by default (but it will still work with --python flag)

Not sure if there are other project-specific configs where it would make sense to add an "enabled_python_git_cl_format = True" flag?

[Nico Weber]

Nice!

How consistent is the autoformatting with the formatting we currently use? Can you run the --full version on ~100 files and upload the diff to gerrit for inspection? I assume it does line wrapping and whatnot? If the requested diff looks good, I'm in favor of checking formatting of python files by default.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1ULXE5-UiQ5HSz0-C%3Don4BNouTecsF7cZZPtMdgKxND9w%40mail.gmail.com.

[Alexei Svitkine]

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

Could the tool's defaults be set up to match surrounding code?

--

[A Olsen]

Does anyone have concerns about this?

I have some potential concerns - not saying don't do it, I just want to make sure it's done well:

- Would the presubmit only enforce that changed lines were misformatted, or any line in the changed file?

- What's the policy if you want to touch a file that currently consistently uses a non-standard formatting? Should you reformat the whole file, or just the lines you touch?

- What's the workaround if a file has to be kept with non-standard formatting?

For who knows what strange files are in the codebase. For instance, policy_templates.json is actually a python file (so, that's weird) - I don't know if it its format matches the python autoformatter (or the JSON autoformatter), but I do know that reformatting it could trigger lots of unnecessary retranslations, since it contains lots of i18n text that is indented and changing the indents will retrigger the translation.

As an example of possible bad side effects:

I was on a non-chrome project before, where the PRESUBMIT wouldn't let you touch a file with a TODO(username) in it unless you also changed it to TODO(buganizer/entry). Or if you felt that this was a waste of time, you could be sneaky and change it to NOTE(username) which wasn't policed.

[Torne (Richard Coles)]

On Fri, 26 Oct 2018 at 10:21 Alexei Svitkine <asvi...@chromium.org> wrote:

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

Google internal style predated Python's PEP 8 declaring that the global standard is 4 spaces. We should be using 4; other Google open source projects generally do for consistency with the rest of the python world.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SC%3DjqaFQiH9rDJKWwNaoHNpuk5_MKhbG8cikDX4%2BnuDew%40mail.gmail.com.

[Andrew Grieve]

On Fri, Oct 26, 2018 at 12:47 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

On Fri, 26 Oct 2018 at 10:21 Alexei Svitkine <asvi...@chromium.org> wrote:

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

Google internal style predated Python's PEP 8 declaring that the global standard is 4 spaces. We should be using 4; other Google open source projects generally do for consistency with the rest of the python world.

This might be good to propose as a change, but right now our chromium-specific style guide says 2 as well.

[Nico Weber]

On Fri, Oct 26, 2018 at 12:47 PM Torne (Richard Coles) <to...@chromium.org> wrote:

On Fri, 26 Oct 2018 at 10:21 Alexei Svitkine <asvi...@chromium.org> wrote:

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

Google internal style predated Python's PEP 8 declaring that the global standard is 4 spaces. We should be using 4; other Google open source projects generally do for consistency with the rest of the python world.

This is not true. Chromium python code for historical reasons has been 2 spaces, and for self-consistency and not needing to rewrite tons of code, we decided on PEP-8, but with 2-space indent: https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md#python

The autoformatter should definitely match our style guide -- if it doesn't, nobody should use it until it does.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEV-rjecYyhBABB7Vq6%2BPtiUksxd5XckY%3D_7_CjSR8HdB9Q4SA%40mail.gmail.com.

[Torne (Richard Coles)]

oh.. sadface :(

[Andrew Grieve]

Here we are:

https://chromium-review.googlesource.com/c/chromium/src/+/1302313

I spot checked ~20 files. Changes all looked fine to me with the exception of one file:

instrumentation_test_instance_test.py

Probably file-a-bug-worthy formatting on that one...

[Andrew Grieve]

On Fri, Oct 26, 2018 at 10:45 AM, A Olsen <ol...@chromium.org> wrote:

Does anyone have concerns about this?

I have some potential concerns - not saying don't do it, I just want to make sure it's done well:

- Would the presubmit only enforce that changed lines were misformatted, or any line in the changed file?

It checks only changed lines (same as C++ and Java), with one exception:

* Lines that are mis-indented are fixed no matter where they occur (excludes line continuations).

The rationale for this is that line indentation carries semantics in python, so restricting that to changed lines isn't a good idea.

This really shouldn't come up much for us.

 

- What's the policy if you want to touch a file that currently consistently uses a non-standard formatting? Should you reformat the whole file, or just the lines you touch?

I think the general style rules apply - which is that you should match the formatting of the existing file.

 

- What's the workaround if a file has to be kept with non-standard formatting?

Ignore the presubmit warning :)

 

For who knows what strange files are in the codebase. For instance, policy_templates.json is actually a python file (so, that's weird) - I don't know if it its format matches the python autoformatter (or the JSON autoformatter), but I do know that reformatting it could trigger lots of unnecessary retranslations, since it contains lots of i18n text that is indented and changing the indents will retrigger the translation.

The formatter just looks at .py files, so at least this odd-ball will be safe.

[Nico Weber]

https://chromium-review.googlesource.com/c/chromium/src/+/1302313 looks good enough for me to try running it by default (and then seeing how it does in practice). Looking forward to it :-)

What's the envisioned process for reporting bugs?

Is there an easy way to get this integrated into my editor?

Are you going to have an md file that answers these questions, similar to https://chromium.googlesource.com/chromium/src/+/lkgr/docs/clang_format.md ?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1Vtsw2rbry7hSS4%2BDQzLJgYDYq0ibjZRGghuO_D7agymw%40mail.gmail.com.

[abe...@google.com]

The new git cl format --python now defaults to chromium style instead of pep8 when no config file is in a parent directory (instead of using the yapf default which is pep8). 

I meant to send this thread to all but this thread bounced back because I wasn't apart of chromium-dev, but with respect to Alexei's case, it looks like the CL author likely wasn't using the latest version of depot tools that contains this change since I was unable to reproduce the incorrect formatting.

On Fri, Oct 26, 2018, 11:27 Alexei Svitkine <asvi...@chromium.org wrote:

Ah ok, that might be it. It was last week.

On Fri, Oct 26, 2018 at 11:22 AM, Aiden Benner <abe...@google.com> wrote:

I took a look and I was not able to reproduce the incorrect formatting (file gets correctly formatted with 2 spaces using git cl format --python and git cl format --python --full). However git cl format changing the entire file to use 4 spaces would be consistent with the behavior before this change https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1249642. 
Perhaps the CL author didn't have their depot tools up to date?

On Fri, Oct 26, 2018 at 10:52 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

Thanks for clarifying. Here's the CL where it happened - perhaps you can check what happened there?

https://chromium-review.googlesource.com/c/chromium/src/+/1279228

(In the end I advised the CL author to bypass the presubmit that was complaining about the format.)

On Fri, Oct 26, 2018 at 10:44 AM, Aiden Benner <abe...@google.com> wrote:

The new python formatting defaults to chromium style (which uses 2 spaces) unless a .style.yapf file is specified in the parent directories. Could there possibly be a style file specified already? Note the old git cl format with the --full flag would use the yapf default (pep8) unless a style file was specified.

On Fri, Oct 26, 2018, 10:21 Alexei Svitkine <asvi...@chromium.org wrote:

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

[Dirk Pranke]

I'm a bit late to this discussion but ...

a) As Nico says, our style guide doesn't quite match PEP-8.

b) Different parts of the code base actually follow different conventions. Most notably, //third_party/blink/tools/blinkpy follows PEP-8 except for the 80-col limit.

The combination of these two things might actually make a repo-wide standard awkward, but I believe the current implementation supports directory-level overrides for the configuration, so I think that's fine.

Personally, I am very supportive of the following:

1) Enable yapf with directory-specific configs as needed and turn on presubmit checking on changed lines

2) Do some large-scale rewrite CLs to bring everything into compliance

3) Strongly consider doing a large-scale rewrite of everything to a single standard if possible (I'd advocate for just matching PEP-8). I'm not sure how well we can automatically rewrite function and method names, though, so we might need a backwards-compatibility carveout for that.

We'll likely be pushing to move to Python3 over the course of the next 12-14 months; while that doesn't generally require much in the way of re-formatting, it does mean that there'll be other excuses for large-scale changes in the python codebase.

-- Dirk

On Fri, Oct 26, 2018 at 10:10 AM, abenner via Chromium-dev <chromi...@chromium.org> wrote:

The new git cl format --python now defaults to chromium style instead of pep8 when no config file is in a parent directory (instead of using the yapf default which is pep8). 

I meant to send this thread to all but this thread bounced back because I wasn't apart of chromium-dev, but with respect to Alexei's case, it looks like the CL author likely wasn't using the latest version of depot tools that contains this change since I was unable to reproduce the incorrect formatting.

On Fri, Oct 26, 2018, 11:27 Alexei Svitkine <asvi...@chromium.org wrote:

Ah ok, that might be it. It was last week.

On Fri, Oct 26, 2018 at 11:22 AM, Aiden Benner <abe...@google.com> wrote:

I took a look and I was not able to reproduce the incorrect formatting (file gets correctly formatted with 2 spaces using git cl format --python and git cl format --python --full). However git cl format changing the entire file to use 4 spaces would be consistent with the behavior before this change https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1249642. 
Perhaps the CL author didn't have their depot tools up to date?

On Fri, Oct 26, 2018 at 10:52 AM, Alexei Svitkine <asvitkine@chromium.org> wrote:

Thanks for clarifying. Here's the CL where it happened - perhaps you can check what happened there?

https://chromium-review.googlesource.com/c/chromium/src/+/1279228

(In the end I advised the CL author to bypass the presubmit that was complaining about the format.)

On Fri, Oct 26, 2018 at 10:44 AM, Aiden Benner <abe...@google.com> wrote:

The new python formatting defaults to chromium style (which uses 2 spaces) unless a .style.yapf file is specified in the parent directories. Could there possibly be a style file specified already? Note the old git cl format with the --full flag would use the yapf default (pep8) unless a style file was specified.

On Fri, Oct 26, 2018, 10:21 Alexei Svitkine <asvi...@chromium.org wrote:

Thanks for working on this!

On a recent CL, I noticed the CL author was modifying an existing file and "git cl format" changed all the indentation from 2 spaces to 4, which was not how the surrounding files were formatted (the change in question was in grit/) - nor how other Python files I work with in Chromium are formatted....

Also, Google internal Python style guide recommends 2-space indents (go/python-style). Although curiously the external one says 4 spaces (https://github.com/google/styleguide/blob/gh-pages/pyguide.md). :/

Could the tool's defaults be set up to match surrounding code?

On Fri, Oct 26, 2018 at 10:12 AM, 'Andrew Grieve' via Chromium-dev <chromium-dev@chromium.org> wrote:

"git cl format" recently learned how to auto-format python code (crbug/846432).

You can use it today via:

git cl format --python  # Formats changed lines only

git cl format --python --full # Formats entire file

I'd personally like to have this enforced by our PRESUBMIT, to cut down on format-related code review comments. 

Does anyone have concerns about this?

As for how to enable this, one idea is:

* If a project has a .style.yapf file in their root, then enable auto-formatting without --python flags and include in presubmit check.

* Otherwise, don't enable it by default (but it will still work with --python flag)

Not sure if there are other project-specific configs where it would make sense to add an "enabled_python_git_cl_format = True" flag?

-- 
-- 
Chromium Developers mailing list: chromium-dev@chromium.org

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/524468bf-7abe-4c94-bc43-80c994591e79%40chromium.org.

[Alan Cutter]

Awesome! Thanks for pushing to improve our tooling. Looking forward to it being on by default.

[Andrew Grieve]

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

I attempted doing so, and found that it resulted in a lot of >80 char lines.

Here's a snippet of presubmit that resulted:

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  build/android/gyp/aar.py, line 79, 82 chars \
  build/android/gyp/aar.py, line 80, 85 chars \
  build/android/gyp/aar.py, line 145, 83 chars \
  build/android/gyp/aar.py, line 146, 82 chars \
  build/android/gyp/apkbuilder.py, line 204, 86 chars
Pylint (56 files using ['--disable=cyclic-import'] on 36 cores) (2.06s) failed
************* Module finalize_apk
C: 35, 0: Line too long (84/80) (line-too-long)
************* Module assert_static_initializers
C: 33, 0: Line too long (81/80) (line-too-long)
C: 56, 0: Line too long (83/80) (line-too-long)
************* Module extract_unwind_tables_tests
C: 94, 0: Line too long (86/80) (line-too-long)
...

I think we could mitigate by disabling pylint's line-too-long warning, and ignore the long line presumbit (which warns only about changed lines, so it wouldn't show up every time like pylint's does).

Options:

* Go with four spaces to match pep-8 and live with long lines

* Stick with 2 space

* Allow both and let it be decided per-directory (e.g. new directories would be encouraged to use 4)

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/4d45311d-396c-41b2-8d2a-1195087e8914%40chromium.org.

[Nico Weber]

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1WbCXVDTNvrr0AbHk40StwhRnEEJrVyEaTHzRbtWazp%2Bw%40mail.gmail.com.

[Andrew Grieve]

The chromium-os team also wants to enable YAPF, but have a different style guide:

http://dev.chromium.org/chromium-os/python-style-guidelines

I thought it would be nice / less surprising if we could get chromium-os and chromium to have the same style guide.

Theirs suggests using 4 spaces for new projects.

[Alexei Svitkine]

Some of the metrics Python files share logic and code with internal versions server-side. It is useful to have the same indentation and style guide as internal Python code.

(We could in theory try to set up the code such as one of the copies is in a third_party dir or similar and thus doesn't have to follow the codebases' style guide, although currently that's not the case and requires additional work.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1VsGrPuSZynpHG8rUN3HExBNRDCM9WmTFZJbrh5F42iiQ%40mail.gmail.com.

[Dirk Pranke]

On Thu, Dec 13, 2018 at 8:16 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

That was decided before we were using auto-formatters much anywhere, I think.

It's also worth revisiting the decision once in a while, I think, just to see if the thinking has changed. 

-- Dirk

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiHtxp-mL-krSGgcc1QhYaOs2grGy%2Bvz22jZQ2D4w4XO3A%40mail.gmail.com.

[Nico Weber]

On Sat, Dec 15, 2018 at 5:15 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 8:16 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

That was decided before we were using auto-formatters much anywhere, I think.

That's a good point. But agrieve's mail sounds like the autoformatter doesn't make things much easier.

 

It's also worth revisiting the decision once in a while, I think, just to see if the thinking has changed. 

It's worth revisiting decisions if there's new data (which, as you point out, was the case here). Revisiting old decisions where nothing has changed just causes churn.

[Jason Clinton]

On Saturday, December 15, 2018 at 3:17:08 PM UTC-7, Dirk Pranke wrote:

On Thu, Dec 13, 2018 at 8:16 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

That was decided before we were using auto-formatters much anywhere, I think.

It's also worth revisiting the decision once in a while, I think, just to see if the thinking has changed. 

Over on the CrOS side, we've already adopted 4 spaces for new projects. There's no mandate for old projects to transition: projects may or may not adopt the YAPF style settings on a per directory basis. For those directories that are willing to migrate, our strategy is to do a mass reformat to 4 spaces and disable the pylintrc warnings for line length and CamelCase method names with no intention of mass-migrating the method names.

[Dirk Pranke]

This thread kind of trailed off without reaching a conclusion. I think we probably have too much code in too many projects to pick a single formatting style that'll make everyone happy and also not require a bulk-rename.

I think we should adopt the policy we've used in various places, which is that consistency with the surrounding code is most important, but that failing that, we should use 4-spaces and 80 cols for new projects (and otherwise follow the style guide, of course), and opt-in to enforcing whatever guide we have where we can. Bulk-reformatting should be acceptable if the OWNERS for that code agree that that's the desired end goal. If we get to a point where the majority of the code is 4-space/80-col then we can revisit reformatting everything else, much like how we approached the clang-format migration.

Thoughts?

-- Dirk

 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d8939751-c627-4474-a8a2-7da46856a2e1%40chromium.org.

[Nico Weber]

On Thu, Mar 21, 2019 at 8:14 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, Dec 18, 2018 at 10:03 AM 'Jason Clinton' via Chromium-dev <chromi...@chromium.org> wrote:

On Saturday, December 15, 2018 at 3:17:08 PM UTC-7, Dirk Pranke wrote:

On Thu, Dec 13, 2018 at 8:16 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

That was decided before we were using auto-formatters much anywhere, I think.

It's also worth revisiting the decision once in a while, I think, just to see if the thinking has changed. 

Over on the CrOS side, we've already adopted 4 spaces for new projects. There's no mandate for old projects to transition: projects may or may not adopt the YAPF style settings on a per directory basis. For those directories that are willing to migrate, our strategy is to do a mass reformat to 4 spaces and disable the pylintrc warnings for line length and CamelCase method names with no intention of mass-migrating the method names.

This thread kind of trailed off without reaching a conclusion. I think we probably have too much code in too many projects to pick a single formatting style that'll make everyone happy and also not require a bulk-rename.

I think we should adopt the policy we've used in various places, which is that consistency with the surrounding code is most important, but that failing that, we should use 4-spaces and 80 cols for new projects

That's a major change to the current recommendation. If you want to propose that, please start a dedicated thread for that so that people see it.

[Dirk Pranke]

On Thu, Mar 21, 2019 at 5:26 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 8:14 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, Dec 18, 2018 at 10:03 AM 'Jason Clinton' via Chromium-dev <chromi...@chromium.org> wrote:

On Saturday, December 15, 2018 at 3:17:08 PM UTC-7, Dirk Pranke wrote:

On Thu, Dec 13, 2018 at 8:16 PM Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 13, 2018 at 10:46 PM Andrew Grieve <agr...@chromium.org> wrote:

Update!

We've got mention of using "git cl format --python" on the (newly broken out) python style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md#YAPF

I've enabled python format checking for //build/android, with the settings I think make sense.

One big unknown is whether we should attempt to switch to PEP8's 4 space indent. 

Why would we consider this? We decided on PEP8 but with 2 spaces ages ago because "too much existing code", and that's even more true now than many years ago.

That was decided before we were using auto-formatters much anywhere, I think.

It's also worth revisiting the decision once in a while, I think, just to see if the thinking has changed. 

Over on the CrOS side, we've already adopted 4 spaces for new projects. There's no mandate for old projects to transition: projects may or may not adopt the YAPF style settings on a per directory basis. For those directories that are willing to migrate, our strategy is to do a mass reformat to 4 spaces and disable the pylintrc warnings for line length and CamelCase method names with no intention of mass-migrating the method names.

This thread kind of trailed off without reaching a conclusion. I think we probably have too much code in too many projects to pick a single formatting style that'll make everyone happy and also not require a bulk-rename.

I think we should adopt the policy we've used in various places, which is that consistency with the surrounding code is most important, but that failing that, we should use 4-spaces and 80 cols for new projects

That's a major change to the current recommendation. If you want to propose that, please start a dedicated thread for that so that people see it.

Done, though I think this is substantively the same thing as what I posted earlier in this thread, and it's nice to have the context. 

But, anyway, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/RcJgJdkNIdg/discussion and let's have additional discussion there.

-- Dirk