auto-formatting python code with yapf, part 2, and a proposed change to the style guide

[Dirk Pranke]

Last fall, agrieve@ started a thread about wanting to use YAPF to auto-format Python code, but we kinda deadlocked debating whether to use 2-space or 4-space indentation (and to a lesser extent whether to enforce 80-cols).

(for reference: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZnjbblW-vEY/discussion ).

Current practice in chromium src is supposed to be 2-space indentation, but there is plenty of code that uses 4-space indentation.

PEP-8 style recommends 4-space, as does the current Google external style guide, as does ChromeOS. There is a desire by infra/ops folks to use the same style across both ChromeOS and Chrome.

We will also be doing a large-scale pass over all of the Python code as we migrate to Python 3, too.

Ideally, I (and some others) would like to reformat all of the code to be auto-formatted, and switch to PEP-8 compliance as part of it.

However, some are resistant to changing from 2-space to 4-space just for that reason (feeling that this is needless churn). Some also feel that consistency with internal Google style (which I think still recommend 2-space for legacy reasons) is useful.

There is also a related debate over using lower_with_underscore_names_for_methods (PEP-8) instead of MethodsAsCamelCase (legacy internal Google), but I think there is more agreement that using the former in new code is fine.

I suggest that deadlocking on this isn't a desireable end state, and I'd really like to get agreement (a) enable auto-formatting, 

and (b) move forward somehow.

We hit similar roadblocks when adopting clang-format, and I suggest we adopt a similar approach here as a result:

Proposal: 

First, consistency with surrounding code is the most important thing.

Second, allow various projects to do either 2-space/80 col or 4-space 80 col, and support project-level/directory-level settings for enabling autoformatting and which level of indentation to use.

Third, allow projects to bulk-reformat to 4-space if the OWNERS agree that that is desirable (and to mandate lower_with_underscore).

Fourth, recommend 4-space/80 col/lower_with_underscore for new projects (matching ChromeOS).

If at some point we reach a majority of the codebase using 4-space/80-col, revisit this to see if we want to just move everything over.

Thoughts?

-- Dirk

[Nico Weber]

Generally seems fine to me, some questions/comments below.

What's "project" here? A git repo? A directory in a git repo? 

 

If at some point we reach a majority of the codebase using 4-space/80-col, revisit this to see if we want to just move everything over.

Unless there's a concerted effort to make this happen, it won't happen. If there's a concerted effort, it'll likely take a few months.

We have ~750KLoC of Python in src.git alone [1]. (We had 100KLoC of gyp code and converting it all to gn took ~3 years – that manual rewriting of the build files so it was a harder problem, but it's the best comparable data point I know of.) I didn't check, but I'd guess that at least 30% of our Python code isn't touched a lot, so it's basically code at rest – meaning presubmits won't see it.

I don't think that's really a problem since we already have a mix of PEP-8 and our artisanal style, so things don't really get worse. And we don't share a lot of google-internal Python code.

1:

$ git ls-files '*.py' | xargs wc -l | grep total

  657200 total

  331992 total

$ git ls-files 'third_party/blink/web_tests/external*.py' | xargs wc -l | grep total

  225272 total

 

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/CAEoffTARK-kx25HCU8dc1GSb-trGiWEqkaL%2BvuCo7zuywTPRCA%40mail.gmail.com.

[Andrew Grieve]

Thanks for bumping this!

We've been running with a //build/android/.style.yapf for a while now, with settings that I think are pretty minimal. I hope to update YAPF's style=chromium to match what's in there (except with 4 space indent).

Drawbacks that I've noticed so far:

* It doesn't check for quotes (filed a crbug)

* It doesn't enforce any naming rules (although looks like pylint can be configured for this)

* It sometimes produces questionably formatted code

   * In this case, the answer is generally to rewrite the code to use more statements, or inject parenthesis.

On the whole though, it's been working very well.

What you suggest sounds good to me. I'm not sure that we should enable YAPF globally by default (e.g. putting a .style.yapf at the root of the repository), as doing so would enforce that style everywhere that didn't override it with another .style.yapf file. Instead, maybe we should just add it to a few high-traffic places (e.g. //build), and document that it's encouraged via the style guide?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiE%2BdR-E3qiRnt0_Xh5n6h3P5RUCE4b1QmG%2BtECJMje%2BMQ%40mail.gmail.com.

[Dirk Pranke]

On Fri, Mar 22, 2019 at 4:50 AM Nico Weber <tha...@chromium.org> wrote:

Generally seems fine to me, some questions/comments below.

On Thu, Mar 21, 2019 at 10:29 PM 'Dirk Pranke' via Chromium-dev <chromi...@chromium.org> wrote:

Last fall, agrieve@ started a thread about wanting to use YAPF to auto-format Python code, but we kinda deadlocked debating whether to use 2-space or 4-space indentation (and to a lesser extent whether to enforce 80-cols).

(for reference: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZnjbblW-vEY/discussion ).

Current practice in chromium src is supposed to be 2-space indentation, but there is plenty of code that uses 4-space indentation.

PEP-8 style recommends 4-space, as does the current Google external style guide, as does ChromeOS. There is a desire by infra/ops folks to use the same style across both ChromeOS and Chrome.

We will also be doing a large-scale pass over all of the Python code as we migrate to Python 3, too.

Ideally, I (and some others) would like to reformat all of the code to be auto-formatted, and switch to PEP-8 compliance as part of it.

However, some are resistant to changing from 2-space to 4-space just for that reason (feeling that this is needless churn). Some also feel that consistency with internal Google style (which I think still recommend 2-space for legacy reasons) is useful.

There is also a related debate over using lower_with_underscore_names_for_methods (PEP-8) instead of MethodsAsCamelCase (legacy internal Google), but I think there is more agreement that using the former in new code is fine.

I suggest that deadlocking on this isn't a desireable end state, and I'd really like to get agreement (a) enable auto-formatting, 

and (b) move forward somehow.

We hit similar roadblocks when adopting clang-format, and I suggest we adopt a similar approach here as a result:

Proposal: 

First, consistency with surrounding code is the most important thing.

Second, allow various projects to do either 2-space/80 col or 4-space 80 col, and support project-level/directory-level settings for enabling autoformatting and which level of indentation to use.

Third, allow projects to bulk-reformat to 4-space if the OWNERS agree that that is desirable (and to mandate lower_with_underscore).

Fourth, recommend 4-space/80 col/lower_with_underscore for new projects (matching ChromeOS).

What's "project" here? A git repo? A directory in a git repo? 

I was thinking of a directory in a git repo, though that might rapidly be "the whole repo" in some repos. But, not src, which is much too large.

 

If at some point we reach a majority of the codebase using 4-space/80-col, revisit this to see if we want to just move everything over.

Unless there's a concerted effort to make this happen, it won't happen. If there's a concerted effort, it'll likely take a few months.

I expect a lot of this to come up as part of the migration to Python 3 compatibility, which will be a big concerted effort that we're planning to track as a program with project mgmt help and everything :).

There is the usual concern about doing one thing at a time, i.e., don't reformat and rewrite at the same time, so we'll have to balance things and figure out what the right thing to do is, of course.

-- Dirk

[Dirk Pranke]

On Fri, Mar 22, 2019 at 10:34 AM Andrew Grieve <agr...@chromium.org> wrote:

Thanks for bumping this!

We've been running with a //build/android/.style.yapf for a while now, with settings that I think are pretty minimal. I hope to update YAPF's style=chromium to match what's in there (except with 4 space indent).

Drawbacks that I've noticed so far:

* It doesn't check for quotes (filed a crbug)

* It doesn't enforce any naming rules (although looks like pylint can be configured for this)

* It sometimes produces questionably formatted code

   * In this case, the answer is generally to rewrite the code to use more statements, or inject parenthesis.

On the whole though, it's been working very well.

What you suggest sounds good to me. I'm not sure that we should enable YAPF globally by default (e.g. putting a .style.yapf at the root of the repository), as doing so would enforce that style everywhere that didn't override it with another .style.yapf file. Instead, maybe we should just add it to a few high-traffic places (e.g. //build), and document that it's encouraged via the style guide?

I agree that we shouldn't enable yapf globally by default; put differently, we need to start opt-in, not opt-out.

-- Dirk

[Robert Ma]

I very much like the idea of eventually enforcing auto-formatting in Python code!

On Thursday, March 21, 2019 at 10:30:42 PM UTC-4, Dirk Pranke wrote:

Proposal: 

First, consistency with surrounding code is the most important thing.

Second, allow various projects to do either 2-space/80 col or 4-space 80 col, and support project-level/directory-level settings for enabling autoformatting and which level of indentation to use.

 

So one thought regarding the line length limit: for example blinkpy tends to have very long lines, and wrapping is probably as large-scale a change as reindenting. In the same vein as optionally allowing 2-space indentation initially to make the transition easier, shall we also give the option to allow >80 col and batch convert later?

[Dirk Pranke]

Yes, that seems reasonable. It would be kinda dumb to re-wrap lines but not re-indent them at the same time (or vice versa).

-- 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/a75cfab9-ec23-4571-9045-dc4fc60d7d13%40chromium.org.

[Dirk Pranke]

It's been almost two weeks since I posted this. 

I've seen no real objections, so I'm going to take this as consent.

We still need to update the Python style guide to reflect this and work out (any remaining) impact on the tooling. Following along with crbug.com/846432 may be your best bet for updates on this for now.

-- Dirk