Run auto-formatter on files in webkitpy/layout_tests/.

Run auto-formatter on files in webkitpy/layout_tests/.

Specifically, I ran:

   autopep8 --in-place --max-line-length 1000 *.py */*.py.

Does this look like something reasonable to do?

Note:

check-webkit-style, lint-webkitpy and test-webkitpy pass after this.

Committed: https://crrev.com/62833e8afcb1bd8f3316b0f9dc56615022af79c8
Cr-Commit-Position: refs/heads/master@{#382993}

[qyearsley, 2016-03-11 18:02:39]

Description was changed from

==========
Run autopep8 --in-place --max-line-length 1000 on webkitpy.
==========

to

==========
Run auto-formatter on files in webkitpy/layout_tests/.

Specifically, I ran:

   autopep8 --in-place --max-line-length 1000 *.py */*.py.

Does this look like something reasonable to do?

Note:

check-webkit-style, lint-webkitpy and test-webkitpy pass after this.
==========

[qyearsley, 2016-03-11 18:02:45]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 18:03:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783073002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783073002/1

[commit-bot: I haz the power, 2016-03-11 20:04:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 20:04:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qyearsley, 2016-03-13 23:48:09]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-13 23:48:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783073002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783073002/1

[commit-bot: I haz the power, 2016-03-14 00:55:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 00:55:26]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-03-16 21:03:49]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, ojan@chromium.org

[Dirk Pranke, 2016-03-16 21:54:12]

dpranke@chromium.org changed reviewers:
+ tansell@chromium.org

[Dirk Pranke, 2016-03-16 21:54:13]

Thanks for doing this! I think the concept of reformatting all of our files is a
good one (I just never got around to it).

I would probably prefer that we use yapf instead of autopep8; I think it's
supposed to produce better output these days, though I can't say I've stared at
any differences enough to actually have an opinion.

We don't want to use a line length of 1000; at most it should be 132 (I think
Ojan and I agreed on this at one point).

Can you post another patch with those two changes so we can compare them?

Personally I'd be inclined to switch to use 80 col and a 2-space indent to match
Chromium style, even though I prefer 4-space indents and that is actually what
PEP-8 (and public Google style) recommends.

Eventually it might be nice to use 80 cols and transform lower_case methods to
MixedCase to also match the style, even though the Chromium example is "wrong"
here, too. Or we could see if we could get a consensus to fix Chromium instead
...

[qyearsley, 2016-03-17 17:51:25]

On 2016/03/16 21:54:13, Dirk Pranke wrote:
> Thanks for doing this! I think the concept of reformatting all of our files is
a
> good one (I just never got around to it).
> 
> I would probably prefer that we use yapf instead of autopep8; I think it's
> supposed to produce better output these days, though I can't say I've stared
at
> any differences enough to actually have an opinion.
> 
> We don't want to use a line length of 1000; at most it should be 132 (I think
> Ojan and I agreed on this at one point).
> 
> Can you post another patch with those two changes so we can compare them?
> 
> Personally I'd be inclined to switch to use 80 col and a 2-space indent to
match
> Chromium style, even though I prefer 4-space indents and that is actually what
> PEP-8 (and public Google style) recommends.
> 
> Eventually it might be nice to use 80 cols and transform lower_case methods to
> MixedCase to also match the style, even though the Chromium example is "wrong"
> here, too. Or we could see if we could get a consensus to fix Chromium instead
> ...

The yapf change is larger than the autopep8 change (+2037 lines, -1814 lines,
rather than +405 lines, -288 lines): https://codereview.chromium.org/1806393002

Also, now I see Tools/Scripts/format-webkitpy -- maybe that should be used
instead?

[Dirk Pranke, 2016-03-17 18:17:15]

On 2016/03/17 17:51:25, qyearsley wrote:
> On 2016/03/16 21:54:13, Dirk Pranke wrote:
> > Thanks for doing this! I think the concept of reformatting all of our files
is
> a
> > good one (I just never got around to it).
> > 
> > I would probably prefer that we use yapf instead of autopep8; I think it's
> > supposed to produce better output these days, though I can't say I've stared
> at
> > any differences enough to actually have an opinion.
> > 
> > We don't want to use a line length of 1000; at most it should be 132 (I
think
> > Ojan and I agreed on this at one point).
> > 
> > Can you post another patch with those two changes so we can compare them?
> > 
> > Personally I'd be inclined to switch to use 80 col and a 2-space indent to
> match
> > Chromium style, even though I prefer 4-space indents and that is actually
what
> > PEP-8 (and public Google style) recommends.
> > 
> > Eventually it might be nice to use 80 cols and transform lower_case methods
to
> > MixedCase to also match the style, even though the Chromium example is
"wrong"
> > here, too. Or we could see if we could get a consensus to fix Chromium
instead
> > ...
> 
> The yapf change is larger than the autopep8 change (+2037 lines, -1814 lines,
> rather than +405 lines, -288 lines):
https://codereview.chromium.org/1806393002
> 

Interesting. Can you post an update to this patch w/ --max-line-length=132
and then maybe post the yapf change as patchset #3, so we can more 
easily compare the three?

> Also, now I see Tools/Scripts/format-webkitpy -- maybe that should be used
> instead?

format-webkitpy is just a wrapper around autopep8 w/ --maxline-length 132
--aggressive
and a custom formatter for rewriting strings to use single quotes where
possible.

So, it's probably slightly better than what you'd get in the second patchset,
but
not a big difference.

[qyearsley, 2016-03-18 00:33:39]

Alright, after trying yapf, it appears that:

 (1) occasionally yapf reformats some manually-formatted code in ways that makes
it less readable
 (2) occasionally yapf produces output that doesn't pass our linter.

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py
(right):

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py:44:

One of the little differences in default behavior between autopep8 and yapf is
that yapf appears to remove blank lines before the first method in a class by
default, whereas autopep8 adds these blank lines.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py:9:
return isinstance(text, str) and (re.search('"repaintRects": \[$', text,
re.MULTILINE) is not None or
`!= None -> is not None` wasn't done by yapf; this was a manual change
afterwards to make the patch pass lint.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_configuration_unittest.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_configuration_unittest.py:43:
MOCK_MACROS = {'mac': ['snowleopard'], 'win': ['vista', 'win7'], 'linux':
['linux32', 'precise', 'trusty'], }
Sometimes yapf reformats literal dicts and lists so that they're not aligned,
making them harder to read.

This is noted in the README for the project:
https://github.com/google/yapf#why-does-yapf-destroy-my-awesome-formatting

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:742:
assert (self._perf_process is None)
== None -> is None was a manual change done in order to pass the presubmit.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:198:
actual_checksum=None)
Yapf also reformats arguments to functions like this so that there's one per
line (although in this case, there was a readability reason for putting two per
line).

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py:1:
# Copyright (C) 2014 Google Inc. All rights reserved.
This file was re-formatted with autopep8 again after trying out yapf, because
yapf produced somewhat weirdly-formatted code that didn't pass the linter.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py:89:
}}}}
For this statement, after running this code through autopep8 and then yapf, I
got:


        valid_json_data_2 = {u'tests': {u'test_category': {u'test_sub_category':
                                                           {u'test_name_2':
{u'test_2.html': {
                                                               u'expected':
u'PASS',
                                                               u'actual':
u'TEXT',
                                                               u'is_unexpected':
True
                                                           }},
                                                            u'test_name':
{u'test.html': {u'expected': u'TEXT',
                                                                                
         u'actual': u'TEXT',
                                                                                
         u'is_unexpected': True}}}}}}


Which didn't pass check-webkit-style.

However, after this file was reverted back to the master version and yapf -i
--style '{based_on_style: pep8, column_limit: 132}' was run again, the format
produced passed the linter, although it's still not very readable to me:

        valid_json_data_2 = {u'tests': {u'test_category': {u'test_sub_category':
                                                           {u'test_name_2':
{u'test_2.html': {u'expected': u'PASS',
                                                                                
             u'actual': u'TEXT',
                                                                                
             u'is_unexpected': True}},
                                                            u'test_name':
{u'test.html': {u'expected': u'TEXT',
                                                                                
         u'actual': u'TEXT',
                                                                                
         u'is_unexpected': True}}}}}}

[Dirk Pranke, 2016-03-18 02:38:20]

On 2016/03/18 00:33:39, qyearsley wrote:
> Alright, after trying yapf, it appears that:
> 
>  (1) occasionally yapf reformats some manually-formatted code in ways that
makes
> it less readable
>  (2) occasionally yapf produces output that doesn't pass our linter.

I apologize; I haven't had the time to look at the actual diffs yet, but I did
see your
comments, and those are intriguing points. We should probably file bugs against
YAPF or the linter as need be, but let me take a closer look first.

[Dirk Pranke, 2016-03-19 02:19:56]

okay, I got about halfway through this before I started getting bored :).

To me neither formatter is producing ideal output, but I might prefer yapf a bit
more. It's possible either or both of them might have knobs that can tweak the
output in a way that's better still.

I don't think I care enough about this, and I would prefer that we have a code
base that was formatted consistently any way at all over what we have now.

So, if ojan@ and mithro@ want to weigh in, great. Otherwise, if you prefer
autopep8, that's good enough by me, especially since we don't have to do any
additional work to make it be installed everywhere.

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py
(right):

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py:44:

On 2016/03/18 00:33:39, qyearsley wrote:
> One of the little differences in default behavior between autopep8 and yapf is
> that yapf appears to remove blank lines before the first method in a class by
> default, whereas autopep8 adds these blank lines.

I think there's a knob in yapf for this.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:219:
# Get the end offset of the slice.
this looks like a bug (the comment shouldn't have been indented).

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:145:
should_add_missing_baselines=(self._options.new_test_results and not
self._test_is_expected_missing(test_file)))
this is an interesting difference; I personally prefer the autopep8 format here.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:168:
time.localtime(self._filesystem.mtime(self._filesystem.join(self._results_directory,
"results.html"))))
I wonder why yapf broke the line here and autopep8 didn't; this kinda looks like
autopep8's version > 132 chars.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:271:
num_workers)
and yet here the formatting is the opposite for what the two formatters did on
_test_input_for_file(), above.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_tests_mover_unittest.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_tests_mover_unittest.py:50:
self._absolute_path('TestExpectations'), """
interesting wrapping choice.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_configuration_unittest.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_configuration_unittest.py:43:
MOCK_MACROS = {'mac': ['snowleopard'], 'win': ['vista', 'win7'], 'linux':
['linux32', 'precise', 'trusty'], }
On 2016/03/18 00:33:39, qyearsley wrote:
> Sometimes yapf reformats literal dicts and lists so that they're not aligned,
> making them harder to read.
> 
> This is noted in the README for the project:
> https://github.com/google/yapf#why-does-yapf-destroy-my-awesome-formatting

Yup, it's clearly trying to fit more stuff on a single line where possible.
There might be knobs we can adjust here.

[qyearsley, 2016-03-19 18:11:44]

Alright -- for now I prefer autopep8 since it's more conservative (doesn't mess
with hand-formatter literal dicts, lists, argument lists as long as they pass
pep8) and I haven't found any cases where it does things I don't want (e.g.
moving comments).

So in the latest patch I just reverted this CL to pack to the `autopep8
--max-line-length 132` version.

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py
(right):

https://codereview.chromium.org/1783073002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/bisect_test_ordering.py:44:

On 2016/03/19 at 02:19:56, Dirk Pranke wrote:
> On 2016/03/18 00:33:39, qyearsley wrote:
> > One of the little differences in default behavior between autopep8 and yapf
is
> > that yapf appears to remove blank lines before the first method in a class
by
> > default, whereas autopep8 adds these blank lines.
> 
> I think there's a knob in yapf for this.

Looks like there is -- the second thing listed at
https://github.com/google/yapf#knobs looks like it would change this.

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py
(right):

https://codereview.chromium.org/1783073002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:219:
# Get the end offset of the slice.
On 2016/03/19 at 02:19:56, Dirk Pranke wrote:
> this looks like a bug (the comment shouldn't have been indented).

That's right -- I just checked, and they have an issue filed for this:
https://github.com/google/yapf/issues/204

[mithro, 2016-03-20 22:21:03]

I think starting with pep8/autopep8 is a good idea. 

If we want to get stricter in the future and play with yapf we can.

[mithro, 2016-03-21 06:01:31]

Couple of quick questions;

 * Why does this CL add two new files?
(third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_tests_mover.py
and friend)

 * A lot of the wrapping could be made nicer by going from;
--------------------------------
        self._print_run_results_entry(run_results, test_expectations.WONTFIX,
                                      "Tests that will only be fixed if they
crash (WONTFIX)")
--------------------------------
to
--------------------------------
        self._print_run_results_entry(
            run_results, test_expectations.WONTFIX,
            "Tests that will only be fixed if they crash (WONTFIX)")
--------------------------------

   I do **not** think you should reformat the code use that style manually. Just
thought it would be worth pointing out and seeing if autopep8 can help at all?

 * Can you make sure the autopep8 output is stable after this CL. IE Running
autopep8 doesn't cause even further changes. I believe this is the default
configuration for autopep8, but it is worth checking.

Otherwise it LGTM.

BTW Can we put a PRESUBMIT.py (or maybe something in the CQ) in to make sure
that the code stays this way? It might be worth opening a bug (and linking this
CL) if you don't plan to do it yourself.

[Dirk Pranke, 2016-03-21 19:09:06]

On 2016/03/21 06:01:31, mithro wrote:
> Couple of quick questions;
> 
>  * Why does this CL add two new files?
> (third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_tests_mover.py
> and friend)

I think this is probably just a merge error. This file was recently deleted and
we
shouldn't re-add it by accident.

[qyearsley, 2016-03-21 22:12:03]

On 2016/03/21 at 06:01:31, tansell wrote:
> Couple of quick questions;
> 
>  * Why does this CL add two new files?
(third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_tests_mover.py
and friend)

Yep, this was a mistake I made when merging. Now removed.

>  * A lot of the wrapping could be made nicer by going from;
> --------------------------------
>         self._print_run_results_entry(run_results, test_expectations.WONTFIX,
>                                       "Tests that will only be fixed if they
crash (WONTFIX)")
> --------------------------------
> to
> --------------------------------
>         self._print_run_results_entry(
>             run_results, test_expectations.WONTFIX,
>             "Tests that will only be fixed if they crash (WONTFIX)")
> --------------------------------
> 
>    I do **not** think you should reformat the code use that style manually.
Just thought it would be worth pointing out and seeing if autopep8 can help at
all?

Both of those styles are acceptable in PEP8, so I'd guess autopep8 probably
doesn't have an option for reformatting prefer line breaks directly after the
opening parenthesis. However, YAPF might be able to do that; there's an option
in yapf called SPLIT_PENALTY_AFTER_OPENING_BRACKET which could be set to zero.


>  * Can you make sure the autopep8 output is stable after this CL. IE Running
autopep8 doesn't cause even further changes. I believe this is the default
configuration for autopep8, but it is worth checking.

Confirmed, it is stable.

> BTW Can we put a PRESUBMIT.py (or maybe something in the CQ) in to make sure
that the code stays this way? It might be worth opening a bug (and linking this
CL) if you don't plan to do it yourself.

There's currently a PRESUBMIT.py in third_party/WebKit that runs
check-webkit-style, which does include pylint and pep8 checks for Python files.
These checks are run on upload but not commit -- which seems fine to me.

[qyearsley, 2016-03-23 21:14:51]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-23 21:15:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783073002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783073002/100001

[commit-bot: I haz the power, 2016-03-23 22:33:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-23 22:33:26]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-03-23 23:34:51]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-03-23 23:34:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tansell@chromium.org
Link to the patchset: https://codereview.chromium.org/1783073002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-23 23:35:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783073002/120001

[commit-bot: I haz the power, 2016-03-24 00:43:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-24 00:44:50]

Description was changed from

==========
Run auto-formatter on files in webkitpy/layout_tests/.

Specifically, I ran:

   autopep8 --in-place --max-line-length 1000 *.py */*.py.

Does this look like something reasonable to do?

Note:

check-webkit-style, lint-webkitpy and test-webkitpy pass after this.
==========

to

==========
Run auto-formatter on files in webkitpy/layout_tests/.

Specifically, I ran:

   autopep8 --in-place --max-line-length 1000 *.py */*.py.

Does this look like something reasonable to do?

Note:

check-webkit-style, lint-webkitpy and test-webkitpy pass after this.

Committed: https://crrev.com/62833e8afcb1bd8f3316b0f9dc56615022af79c8
Cr-Commit-Position: refs/heads/master@{#382993}
==========

[commit-bot: I haz the power, 2016-03-24 00:44:52]

Patchset 7 (id:??) landed as
https://crrev.com/62833e8afcb1bd8f3316b0f9dc56615022af79c8
Cr-Commit-Position: refs/heads/master@{#382993}