New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846432 link

Starred by 3 users

Issue metadata

Status: Available
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 890349



Sign in to add a comment

Add pyformat to "git cl format" by default

Project Member Reported by wnwen@chromium.org, May 24 2018

Issue description

Auto-format python code.

https://github.com/myint/pyformat
https://github.com/google/yapf

Once we have auto-formatting, we could disable the formatting-related checks from pylint.

Experimental support for this via yapf was added in 2015:
https://codereview.chromium.org/1156743008

The relevant code is here:
https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?rcl=8bdc1b8a0433e3e0737bab3a31bd6fba39383610&l=5952

You can try it out by:
1) Make a change to any python file
2) Copy .style.yapf from depot_tools to src
3) Run git cl format --python --full


What concrete work needs to be done here:
* Make it use depot_tool's .style.yapf when none exists in the current checkout
* Add support for non --full mode (bug in comments looks to be fixed... probably just need to update version of yapf)
* Enable in git cl format by default (without --python)
 
Description: Show this description
Owner: abenner@google.com
Description: Show this description
Blockedon: 890349
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/c08566e6e27cf671133e2787fd49ca9451535163

commit c08566e6e27cf671133e2787fd49ca9451535163
Author: Aiden Benner <abenner@google.com>
Date: Wed Oct 03 17:52:42 2018

Add non --full support to python git cl format

Parses git diff for changed python files in a method similar to
clang-diff and feeds the resulting line ranges into yapf.

Also sets the default style to the yapf file included in depot tools
by searching parent directories of each changed file to find a yapf
style config (.style.yapf). If none is found the default style
file will be the chromium .style.yapf included in depot tools.

Note: Even if line ranges are specified, yapf will fix indentation
issues for the entire file.
This is intended see https://github.com/google/yapf/issues/499
This may cause some issues if git cl format is run on a file
with lots of indentation issues or on a file or when run on a
third_party file that is formatted with pep8 and does not include
a .style.yapf and may make many more changes then the user expects.

Still undecided on whether this should be turned on by default but
if not I think the non --full support is a positive change anyways.

Bug:846432
Change-Id: Ib85797f4a8e1021870901ff465ec10f7e70deb87
Reviewed-on: https://chromium-review.googlesource.com/1249642
Commit-Queue: Aiden Benner <abenner@google.com>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>

[modify] https://crrev.com/c08566e6e27cf671133e2787fd49ca9451535163/git_cl.py

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89b09b9ca3142d7bf9262de9b4ca7948db7ab7b2

commit 89b09b9ca3142d7bf9262de9b4ca7948db7ab7b2
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Wed Oct 03 20:20:00 2018

Roll src/third_party/depot_tools 6af3aa854956..684313d6a319 (2 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/6af3aa854956..684313d6a319


git log 6af3aa854956..684313d6a319 --date=short --no-merges --format='%ad %ae %s'
2018-10-03 oprypin@webrtc.org gclient setdep doesn't need to update_depot_tools
2018-10-03 abenner@google.com Add non --full support to python git cl format


Created with:
  gclient setdep -r src/third_party/depot_tools@684313d6a319

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:836566,chromium:846432
TBR=agable@chromium.org

Change-Id: I7686bb5d7f4d16aa8badf940b3b9f6bc104f4ee6
Reviewed-on: https://chromium-review.googlesource.com/c/1259489
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#596342}
[modify] https://crrev.com/89b09b9ca3142d7bf9262de9b4ca7948db7ab7b2/DEPS

Very cool that there's been work to add this. Seems to have a could gotchas on Windows. Running out of the box gives an error trying to execute the python script 'yapf'. I tried changing this to target 'yapf.bat' or 'python yapf' but then both seemed to have a bug:

Traceback (most recent call last):
  File "C:\dev\depot_tools\yapf", line 16, in <module>
    from yapf import run_main
ImportError: No module named yapf
Command "python C:\dev\depot_tools\yapf --style C:\dev\depot_tools\.style.yapf scripts/run_code_generation.py -l 22-23 -l 27-27 -l 32-32 -l 35-36 -l 39-45 -l 165-169 -l 172-178 -l 187-189 -l 191-192 -l 194-195 -l 197-199 -l 202-215 -i" failed.

Could be a slight bug with how the parameters are escaped with quotes somehow?

Comment 8 Deleted

Better summary of remaining tasks (Comment 8 was previous draft of this):
1) Get this working on a Windows checkout
2) Add a //docs/yapf.md similar to clang_format.md, and link to it from //styleguide/styleguide.md
3) Add //build/.style.yapf so that this can be tested for a while for //build
4) Add //third_party/blink/tools/blinkpy/.style.yapf so that it stays as pep8
5) Add a root .style.yapf to enable for chromium.

Will enabling it by default be after 5)?
I think the plan is to have py files that have a .style.yapf in an ancestor directory to be formatted by default. The --python flag to run over all py files (and use the chromium default if no style file is found)
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/e47ac15d93b88d76e38e56054a15dfb916b30112

commit e47ac15d93b88d76e38e56054a15dfb916b30112
Author: Aiden Benner <abenner@google.com>
Date: Tue Nov 20 16:44:03 2018

Fix git cl format --python on windows

git cl format --python currently breaks on windows
because FindExecutable('yapf') returns .../depot_tools/yapf
(a py file) instead of .../depot_tools/yapf.bat. Also
yapf.bat tries to run the yapf py file without vpython
which breaks the yapf dependency.

This CL fixes these two issues.


Bug:846432
Change-Id: I551a4c1e6367074fa76767851bd34feb2dcfb6a2
Reviewed-on: https://chromium-review.googlesource.com/c/1341236
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Aiden Benner <abenner@google.com>

[modify] https://crrev.com/e47ac15d93b88d76e38e56054a15dfb916b30112/yapf.bat
[modify] https://crrev.com/e47ac15d93b88d76e38e56054a15dfb916b30112/git_cl.py

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42999ec41a129ac385fbd8007e883bce9ae4f0f7

commit 42999ec41a129ac385fbd8007e883bce9ae4f0f7
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Tue Nov 20 19:48:34 2018

Roll src/third_party/depot_tools 7da982abf99f..e47ac15d93b8 (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/7da982abf99f..e47ac15d93b8


git log 7da982abf99f..e47ac15d93b8 --date=short --no-merges --format='%ad %ae %s'
2018-11-20 abenner@google.com Fix git cl format --python on windows


Created with:
  gclient setdep -r src/third_party/depot_tools@e47ac15d93b8

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:846432
TBR=agable@chromium.org

Change-Id: I2915800b137f3679f918b5cfa488a70469505ffe
Reviewed-on: https://chromium-review.googlesource.com/c/1344271
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#609777}
[modify] https://crrev.com/42999ec41a129ac385fbd8007e883bce9ae4f0f7/DEPS

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/99b0ccb45a0f5b976040c93811818ecdbe83471f

commit 99b0ccb45a0f5b976040c93811818ecdbe83471f
Author: Aiden Benner <abenner@google.com>
Date: Tue Nov 20 19:53:31 2018

Enable python formatting by default if yapf file provided

Enables python formatting in git cl format by default
for all files that have a .style.yapf file in a parent
directory up to the repository root. If no .style.yapf
file is found the --python flag still needs to be
set explicitly for python formatting to be enabled.
Also adds a --no-python to explicitly disable python
formatting on any file even if a .style.yapf file is found.

This allows default formatting to be enabled on a per
project/directory basis by adding a .style.yapf file.

Bug:846432
Change-Id: I40d899ec1a3e0dfca445e04b91befab113416175
Reviewed-on: https://chromium-review.googlesource.com/c/1316415
Commit-Queue: Aiden Benner <abenner@google.com>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>

[modify] https://crrev.com/99b0ccb45a0f5b976040c93811818ecdbe83471f/presubmit_canned_checks.py
[modify] https://crrev.com/99b0ccb45a0f5b976040c93811818ecdbe83471f/git_cl.py

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/892936ea1d80b822d437a4590cc3bfd0ac10eb73

commit 892936ea1d80b822d437a4590cc3bfd0ac10eb73
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Wed Nov 21 09:40:22 2018

Roll src/third_party/depot_tools e47ac15d93b8..a85a4b01ee72 (9 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/e47ac15d93b8..a85a4b01ee72


git log e47ac15d93b8..a85a4b01ee72 --date=short --no-merges --format='%ad %ae %s'
2018-11-21 jbudorick@chromium.org Don't fetch flash when fetching android_internal. (reland)
2018-11-21 tandrii@chromium.org git cl: remove Rietveld implementation entirely.
2018-11-21 tandrii@chromium.org git cl: remove Rietveld import and things it depends on.
2018-11-21 tandrii@chromium.org git cl: get rid of Rietveld's upload.py import.
2018-11-20 vadimsh@chromium.org Revert "[vpython] Roll to pick up user differentiation of cached envs."
2018-11-20 iannucci@chromium.org [vpython] Roll to pick up user differentiation of cached envs.
2018-11-20 jbudorick@chromium.org Revert "Don't fetch flash when fetching android_internal."
2018-11-20 abenner@google.com Enable python formatting by default if yapf file provided
2018-11-20 jbudorick@chromium.org Don't fetch flash when fetching android_internal.


Created with:
  gclient setdep -r src/third_party/depot_tools@a85a4b01ee72

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:856278,chromium:869227,chromium:869227,chromium:856278,chromium:846432,chromium:856278
TBR=agable@chromium.org

Change-Id: Ifc45e8c4dda5ca7c283310fc840910250a778ffd
Reviewed-on: https://chromium-review.googlesource.com/c/1345394
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#609969}
[modify] https://crrev.com/892936ea1d80b822d437a4590cc3bfd0ac10eb73/DEPS

I get crashes when trying to run this in a standalone V8 checkout:

$ git cl format --python
Traceback (most recent call last):
  File "/usr/local/google/home/leszeks/depot_tools/metrics.py", line 262, in print_notice_and_exit
    yield
  File "/usr/local/google/home/leszeks/depot_tools/git_cl.py", line 5728, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/usr/local/google/home/leszeks/depot_tools/git_cl.py", line 5710, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/usr/local/google/home/leszeks/depot_tools/subcommand.py", line 252, in execute
    return command(parser, args[1:])
  File "/usr/local/google/home/leszeks/depot_tools/metrics.py", line 247, in _inner
    return self._collect_metrics(func, command_name, *args, **kwargs)
  File "/usr/local/google/home/leszeks/depot_tools/metrics.py", line 200, in _collect_metrics
    result = func(*args, **kwargs)
  File "/usr/local/google/home/leszeks/depot_tools/git_cl.py", line 5494, in CMDformat
    py_line_diffs = _ComputeDiffLineRanges(filtered_py_files, upstream_commit)
  File "/usr/local/google/home/leszeks/depot_tools/git_cl.py", line 721, in _ComputeDiffLineRanges
    line_diffs[curr_file].append((diff_start, diff_count))
KeyError: None
Hi leszeks.
I was able to reproduce an identical crash by adding diff.noprefix to my git config. This was breaking the regex that was parsing the git diff. I will work on a fix.

Project Member

Comment 18 by bugdroid1@chromium.org, Nov 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/6c18a1afa1a7a30421775c8d6c1cfb7edb3f272a

commit 6c18a1afa1a7a30421775c8d6c1cfb7edb3f272a
Author: Aiden Benner <abenner@google.com>
Date: Fri Nov 23 20:18:23 2018

Fix git cl format --python when diff.noprefix flag is set

Adding the diff.noprefix flag to .gitconfig broke python
formatting because git diffs were being parsed with regex
that relied on the default git diff prefixs.

This CL fix's this issue by always specifying prefix flags
for git diffs when formatting python.

Bug:846432
Change-Id: Ifde305da9574e6b46455a0237703b7364fac77e4
Reviewed-on: https://chromium-review.googlesource.com/c/1349809
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/6c18a1afa1a7a30421775c8d6c1cfb7edb3f272a/git_cl.py

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0cee69040efa1d80e27b0f41a55e431587e11a2

commit f0cee69040efa1d80e27b0f41a55e431587e11a2
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Nov 23 21:54:09 2018

Roll src/third_party/depot_tools 25c4fce2cebb..6c18a1afa1a7 (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/25c4fce2cebb..6c18a1afa1a7


git log 25c4fce2cebb..6c18a1afa1a7 --date=short --no-merges --format='%ad %ae %s'
2018-11-23 abenner@google.com Fix git cl format --python when diff.noprefix flag is set


Created with:
  gclient setdep -r src/third_party/depot_tools@6c18a1afa1a7

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:846432
TBR=agable@chromium.org

Change-Id: I72da7b6f97e3b3026d79ae8ddddee692a11896d8
Reviewed-on: https://chromium-review.googlesource.com/c/1349444
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#610674}
[modify] https://crrev.com/f0cee69040efa1d80e27b0f41a55e431587e11a2/DEPS

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6dad4a95a96690e5f5b40460537b94e79143687

commit a6dad4a95a96690e5f5b40460537b94e79143687
Author: Andrew Grieve <agrieve@chromium.org>
Date: Thu Nov 29 13:57:32 2018

python.md: Add info about YAPF and elaborate on pylint

Bug: 846432
Change-Id: Iac715f99198a10409018c208588211252aa53e53
Reviewed-on: https://chromium-review.googlesource.com/c/1352819
Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Peter Wen <wnwen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612178}
[modify] https://crrev.com/a6dad4a95a96690e5f5b40460537b94e79143687/styleguide/python/python.md

Project Member

Comment 21 by bugdroid1@chromium.org, Nov 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a1b9066b334be375ff937378b0670cb7941b92c

commit 1a1b9066b334be375ff937378b0670cb7941b92c
Author: agrieve <agrieve@chromium.org>
Date: Fri Nov 30 15:47:22 2018

python.md: Add info about YAPF bugs and editor integrations

Bug: 846432
Change-Id: Ie618f4e8f978bec4134b1b163e46f087d3101967
Reviewed-on: https://chromium-review.googlesource.com/c/1356238
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612658}
[modify] https://crrev.com/1a1b9066b334be375ff937378b0670cb7941b92c/styleguide/python/python.md

Project Member

Comment 22 by bugdroid1@chromium.org, Dec 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5caba8d74f3d888c65d18d45f3608ae2118c71fd

commit 5caba8d74f3d888c65d18d45f3608ae2118c71fd
Author: Andrew Grieve <agrieve@chromium.org>
Date: Thu Dec 06 02:23:36 2018

Enable Python auto-formatting for //build/android

This will cause presubmit to fail for deltas that don't match
YAPF rules, and also enable "git cl format" to fix any such
violations.

Bug: 846432
Change-Id: I3602e41811474a068f4780628601a14565320db9
Reviewed-on: https://chromium-review.googlesource.com/c/1364338
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614237}
[add] https://crrev.com/5caba8d74f3d888c65d18d45f3608ae2118c71fd/build/android/.style.yapf

Cc: -agrieve@chromium.org
Owner: agrieve@chromium.org
Summary: Add pyformat to "git cl format" by default (was: Add pyformat to "git cl format")

Sign in to add a comment