Bug 35499

Summary: webkitpy: Move webkitpy/mock.py into webkitpy/thirdparty
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cjerdonek, eric, hamaji, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
eric: review-, cjerdonek: commit-queue-
Proposed patch 2 abarth: review+, cjerdonek: commit-queue-

Description Chris Jerdonek 2010-02-28 11:54:22 PST
Move webkitpy/mock.py into webkitpy/thirdparty.

I discovered this was third-party while working on this--

https://bugs.webkit.org/show_bug.cgi?id=35465 (check-webkit-style should check license text)
Comment 1 Chris Jerdonek 2010-02-28 12:19:57 PST
Created attachment 49701 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-02-28 12:20:57 PST
Attachment 49701 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebKitTools/Scripts/webkitpy/mock.py': Can't open for reading
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:59:  whitespace before ')'  [pep8/E202] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:64:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:84:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:88:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:91:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:115:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:127:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:138:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:171:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:174:  line too long (80 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:187:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:188:  line too long (115 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:220:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:246:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:259:  line too long (86 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:263:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:282:  too many blank lines (2)  [pep8/E303] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:298:  line too long (85 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/thirdparty/mock.py:304:  too many blank lines (3)  [pep8/E303] [5]
Total errors found: 19 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Jerdonek 2010-02-28 12:23:59 PST
(In reply to comment #2)
> Attachment 49701 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Skipping input 'WebKitTools/Scripts/webkitpy/mock.py': Can't open for reading
> WebKitTools/Scripts/webkitpy/thirdparty/mock.py:59:  whitespace before ')' 
> [pep8/E202] [5]

I filed a report for this here (to exempt webkitpy/thirdparty from most pep8 checks):

https://bugs.webkit.org/show_bug.cgi?id=35497
Comment 4 Eric Seidel (no email) 2010-03-05 15:11:54 PST
Comment on attachment 49701 [details]
Proposed patch

Why not just fix all the commands?
webkitpy/commands_references.py

Seems cleaner to just fix all the commands instead of using commands_references.  Otherwise this looks fine.

I think ..foo_references is actually quite hard to read.  And it's inconsistent throughout all our files.  I think we should just import these directly, no?
Comment 5 Chris Jerdonek 2010-03-05 19:11:53 PST
(In reply to comment #4)

Thanks a lot for the review and comments, Eric!

> (From update of attachment 49701 [details])
> Why not just fix all the commands?
> webkitpy/commands_references.py

I've explained some of the reasons for this pattern to Adam in the past:

https://bugs.webkit.org/show_bug.cgi?id=33791#c3

I think this pattern is preferable because it helps prevent the number of intra-package references from getting out of control -- especially as webkitpy grows.  This makes it easier to move folders around in the repository since there are fewer files to touch:

Modules in a package don't have to know the location of every intra-package reference.  They just have to know the location of the package's associated reference file.  Similarly, if you want to move the location of a referenced package, you don't have to touch large numbers of files.  You just have to update the references file.

It also helps to understand what packages and functionality a nested package depends on, because the reference file collects them in one place.

> I think ..foo_references is actually quite hard to read.  And it's inconsistent
> throughout all our files.  I think we should just import these directly, no?

Yeah, I agree -- thanks.  (PEP8 does, too, I found out later.)  I actually went through the process of changing all those relative references when I was getting test-webkitpy working with Python 2.4 (since 2.4 doesn't support relative references).  Fortunately we don't have too many right now.
Comment 6 Chris Jerdonek 2010-03-05 19:34:15 PST
Created attachment 50148 [details]
Proposed patch 2

I replaced the relative references with absolute references (e.g. ..command_references with webkitpy.command_references) and left the references files alone after explaining their purpose.  We can continue discussing that though if you want.  Thanks for considering.
Comment 7 Adam Barth 2010-03-13 02:05:42 PST
Comment on attachment 50148 [details]
Proposed patch 2

Nice.  Eventually we need to move more stuff out of the webkitpy root directory.
Comment 8 Chris Jerdonek 2010-03-13 12:58:08 PST
(In reply to comment #7)
> (From update of attachment 50148 [details])
> Nice.  Eventually we need to move more stuff out of the webkitpy root
> directory.

Thanks, Adam!

I'll create a master bug for that mini-project so we can discuss how we'd like to organize things and because the move may be spread over more than one patch.
Comment 9 Adam Barth 2010-03-13 13:04:50 PST
In one of your other patches, you made a "patch" directory.  It might make sense to move webkit-patch specific files there.
Comment 10 Chris Jerdonek 2010-03-13 13:31:44 PST
Manually committed (via svn commit):

http://trac.webkit.org/changeset/55968
Comment 11 Chris Jerdonek 2010-03-13 13:35:55 PST
(In reply to comment #9)
> In one of your other patches, you made a "patch" directory.  It might make
> sense to move webkit-patch specific files there.

Sounds good.  I might need your help in determining which files and folders are webkit-patch specific.
Comment 12 Chris Jerdonek 2010-03-13 17:01:51 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 50148 [details] [details])
> > Nice.  Eventually we need to move more stuff out of the webkitpy root
> > directory.
> 
> Thanks, Adam!
> 
> I'll create a master bug for that mini-project so we can discuss how we'd like
> to organize things and because the move may be spread over more than one patch.

I created the report here:

https://bugs.webkit.org/show_bug.cgi?id=36093