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-

Chris Jerdonek
Reported 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)
Attachments
Proposed patch (56.50 KB, patch)
2010-02-28 12:19 PST, Chris Jerdonek
eric: review-
cjerdonek: commit-queue-
Proposed patch 2 (56.57 KB, patch)
2010-03-05 19:34 PST, Chris Jerdonek
abarth: review+
cjerdonek: commit-queue-
Chris Jerdonek
Comment 1 2010-02-28 12:19:57 PST
Created attachment 49701 [details] Proposed patch
WebKit Review Bot
Comment 2 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.
Chris Jerdonek
Comment 3 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
Eric Seidel (no email)
Comment 4 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?
Chris Jerdonek
Comment 5 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.
Chris Jerdonek
Comment 6 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.
Adam Barth
Comment 7 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.
Chris Jerdonek
Comment 8 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.
Adam Barth
Comment 9 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.
Chris Jerdonek
Comment 10 2010-03-13 13:31:44 PST
Manually committed (via svn commit): http://trac.webkit.org/changeset/55968
Chris Jerdonek
Comment 11 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.
Chris Jerdonek
Comment 12 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
Note You need to log in before you can comment on or make changes to this bug.