Summary: | new-run-webkit-tests --platform=mac-leopard diffs are backwards | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dpranke | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 35365, 34984 | ||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2010-02-22 15:24:58 PST
Created attachment 49441 [details]
Patch
change looks good but we should make this consistent across the board. I've filed bug 35365 about fixing this sort of issue globally. Comment on attachment 49441 [details] Patch > diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog > +2010-02-24 Eric Seidel <eric@webkit.org> > + > + Reviewed by NOBODY (OOPS!). > + > + run-chromium-webkit-tests --platform=mac-leopard diffs are backwards > + https://bugs.webkit.org/show_bug.cgi?id=35265 > + > + Some parts of the code passed arguments as > + "actual, expected" and some passed as "expected, actual". > + As you might imagine, this lead to great confusion and wrongness. > + Standardize on "expected, actual" as that't the order which is typo: that't > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py > - def diff_text(self, actual_text, expected_text, > - actual_filename, expected_filename): > + def diff_text(self, expected_text, actual_text, > + expected_filename, actual_filename): It would be nice to align to the ( of the previous line (as was done before). (In reply to comment #4) > > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py > > - def diff_text(self, actual_text, expected_text, > > - actual_filename, expected_filename): > > + def diff_text(self, expected_text, actual_text, > > + expected_filename, actual_filename): > It would be nice to align to the ( of the previous line (as was done before). I thought "we" as webkit avoided that because you end up having to re-indent all the time. I went with the "just indent one level" model. Does PEP8 say anything on this subject? > I thought "we" as webkit avoided that because you end up having to re-indent
> all the time. I went with the "just indent one level" model. Does PEP8 say
> anything on this subject?
PEP 8 says "Make sure to indent the continued line appropriately", but examples show function arguments and parenthesized expressions being aligned. Google's Python style guide says to align vertically and, failing that, to align w/ a hanging indent.
Created attachment 49664 [details]
posting a revised patch since Eric is OOO
Comment on attachment 49664 [details] posting a revised patch since Eric is OOO > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py > + def diff_text(self, expected_text, actual_text, > + expected_filename, actual_filename): > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py > + def diff_text(self, expected_text, actual_text, > + expected_filename, actual_filename): fwiw, this indent doesn't align with the (. Note that there are many examples even within this patch of the alignment happening. Committed r55372: <http://trac.webkit.org/changeset/55372> |