Bug 35499 – webkitpy: Move webkitpy/mock.py into webkitpy/thirdparty

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

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)

[Chris Jerdonek]

Created attachment 49701 [details]
Proposed patch

[WebKit Review Bot]

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.

[Chris Jerdonek]

(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

[Eric Seidel (no email)]

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?

[Chris Jerdonek]

(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.

[Chris Jerdonek]

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.

[Adam Barth]

Comment on attachment 50148 [details]
Proposed patch 2

Nice.  Eventually we need to move more stuff out of the webkitpy root directory.

[Chris Jerdonek]

(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.

[Adam Barth]

In one of your other patches, you made a "patch" directory.  It might make sense to move webkit-patch specific files there.

[Chris Jerdonek]

Manually committed (via svn commit):

http://trac.webkit.org/changeset/55968

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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