| Summary: | check-webkit-style: WebKit needs a python style checker | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
| Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, cjerdonek, hamaji, levin, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Bug Depends on: | 35163 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Eric Seidel (no email)
2010-01-13 16:38:52 PST
We also need to update the WebKit style guide html page to say that we try to follow PEP8. pep8.py seems to be the cannon. I'm not sure what the "expat" license is or if it's compatible with WebKit's repository however. http://pypi.python.org/pypi/pylint is a linter which does some style checking. That's unfortunately GPL though. http://www.python.org/dev/peps/pep-0008/ is the original PEP8 document by the way. (In reply to comment #0) > WebKit needs a python style checker > > We try to follow PEP8. I'm certain there must be a PEP8 checker out there we > could use. We can use webkitpy/autoinstall.py to autoinstall it if it's not > available under a compatible license. Let's also add PEP 257, which covers doc strings. This URL says the Python style guide comprises both PEP 8 and PEP 257: http://www.python.org/doc/essays/styleguide.html Can we use this thread to come to agreement on double vs. single quotes? This came up a day or two ago: https://bugs.webkit.org/show_bug.cgi?id=33454 I propose that we always use double quotes, unless the use of single quotes results in less escaping. To be precise, maybe we can say that single quotes should be used in place of double quotes if and only if the string contains a double quote. I strongly agree to have a style guide for python. It should be PEP8 with a few modifications. The candidates of the modifications I remember are
- (as Chris proposed) use double-quotes for string literals. (We may be able to use single-quoted literals for strings which have double-quotes in the string? E.g., '<a href="webkit.org">webkit</a>')
- don't put (or put) parentheses for if, elif, and while statements.
- remove 80 characters per a line rule to be consistent with other code in WebKit?
- prefer "%(file)s:%(line)d" % {"file": file, "line": line} to "%s:%d" % (file, line) for long string interpolations.
- prefer r"(?P<file>\w+):(?P<line>\d+)" to r"(\w+):(\d+)" for long regexps.
> - remove 80 characters per a line rule to be consistent with other code in > WebKit? Chris informed me we've recently started using this 80 characters rule in Bug 34379. So, we may want some guidelines for line-breaking. (In reply to comment #8) > > - remove 80 characters per a line rule to be consistent with other code in > > WebKit? > > Chris informed me we've recently started using this 80 characters rule in Bug > 34379. So, we may want some guidelines for line-breaking. Eric and Adam started doing this before me in some other check-ins, for example-- http://trac.webkit.org/changeset/53776 I don't recall there being any objections to adopting PEP 8 in its entirety, when the idea was e-mailed to webkit-dev. I guess we can consider it formally adopted once it's on the web site. I'll take this on if no one else is. Looks like it might be fun and not too time-consuming. Added check-webkit-style: to the beginning of the bug title. Created attachment 49439 [details]
Proposed patch
cq- since I would need to rebase the patch after committing the patch this report depends on.
Attachment 49439 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitpy/style/processors/python_unittest_input.py:2: blank line at end of file [pep8/W391] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 49442 [details]
Proposed patch 2
Added code comments to python_unittest_input.py so it is clearer what the style error is.
Attachment 49442 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitpy/style/processors/python_unittest_input.py:2: trailing whitespace [pep8/W291] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #13) > Attachment 49439 [details] did not pass style-queue: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > WebKitTools/Scripts/webkitpy/style/processors/python_unittest_input.py:2: > blank line at end of file [pep8/W391] [5] > Total errors found: 1 in 8 files Well, what do you know -- it works! :) The required pep8.py (or pep8.pyc) file must be left over from the autoinstalling done by the other patch: https://bugs.webkit.org/show_bug.cgi?id=35163 (This error is a deliberate one, by the way, since it is input to a unit test.) I'm super excited about this patch. Thanks Chris! (In reply to comment #17) > I'm super excited about this patch. Thanks Chris! Thanks, guys. And thanks a lot for the review, David. Can one of you do me a favor and review the patch on which this depends? https://bugs.webkit.org/show_bug.cgi?id=35163 pep8 needs to auto-install for this patch to work. (It was sort of an accident that it was able to work on this patch, since I guess the copy of pep8 was left on disk from the other patch getting tried out.) Manually committed (via "git svn dcommit"): http://trac.webkit.org/changeset/55350 Rolled out (via "git svn dcommit"): http://trac.webkit.org/changeset/55361 Because of-- https://bugs.webkit.org/show_bug.cgi?id=35163#c21 (In reply to comment #20) > Rolled out (via "git svn dcommit"): > > http://trac.webkit.org/changeset/55361 > > Because of-- > > https://bugs.webkit.org/show_bug.cgi?id=35163#c21 Fixed up ChangeLog after rollout, after discussing proper protocol with Shinichiro: http://trac.webkit.org/changeset/55362 (This is my first time rolling out. Thanks again for your help, Shinichiro!) The bug report on which this depends has been updated with a new patch: https://bugs.webkit.org/show_bug.cgi?id=35163 Once that gets r+'ed, I can re-land this. Committed: http://trac.webkit.org/changeset/56899 Have I mentioned recently how much I hate 80-col wrap? What a waste of time... (In reply to comment #24) > Have I mentioned recently how much I hate 80-col wrap? What a waste of time... FYI, I tried-- > check-webkit-style --filter -,+pep8/E501 WebKitTools/Scripts/webkitpy and got-- ... WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py:63: line too long (138 characters) [pep8/E501] [5] WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py:64: line too long (146 characters) [pep8/E501] [5] WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer_unittest.py:53: line too long (95 characters) [pep8/E501] [5] Total errors found: 1432 in 475 files > (Note that I had to explicitly add "pep8/e501" to the list of categories to get this to work.) Sorry. My statement wasn't very helpful. I don't understand your comment above? I think we should turn off the 80c check for now, at least until we fix other more useful style violations in the code. Right now we're just assaulting committers with walls of text. :( https://bugs.webkit.org/show_bug.cgi?id=36394#c37 (In reply to comment #26) > Sorry. My statement wasn't very helpful. > > I don't understand your comment above? Oh, I was just running check-webkit-style for fun to see how many line-too-long errors we have in webkitpy (i.e. longer than 79 characters) -- using the filter option to restrict to that pep8 error category. The answer is 1432 lines of code which you can see from the last output line of that comment. (Much higher than I thought it would be, incidentally.) (In reply to comment #27) > I think we should turn off the 80c check for now, at least until we fix other > more useful style violations in the code. Right now we're just assaulting > committers with walls of text. :( > https://bugs.webkit.org/show_bug.cgi?id=36394#c37 Blah. We should just go and fix these. It's not very hard if you turn an 80c guide on in your editor. (In reply to comment #29) > (In reply to comment #27) > > I think we should turn off the 80c check for now, at least until we fix other > > more useful style violations in the code. Right now we're just assaulting > > committers with walls of text. :( > > https://bugs.webkit.org/show_bug.cgi?id=36394#c37 > > Blah. We should just go and fix these. It's not very hard if you turn an 80c > guide on in your editor. Personally I think we should strike this from our style guide. It adds no value to our code. Anyone who wants a fully-PEP8 webkit can run a re-wrapping script. > Personally I think we should strike this from our style guide. It adds no
> value to our code. Anyone who wants a fully-PEP8 webkit can run a re-wrapping
> script.
We had this discussion on webkit-dev. The conclusion was that there's value in match PEP8 fully instead of picking and choosing what rules we care about.
(In reply to comment #31) > > Personally I think we should strike this from our style guide. It adds no > > value to our code. Anyone who wants a fully-PEP8 webkit can run a re-wrapping > > script. > > We had this discussion on webkit-dev. The conclusion was that there's value in > match PEP8 fully instead of picking and choosing what rules we care about. fwiw, I agree with Eric (but I don't do much python coding in wk lately). Seeing this in practice and how it tortures code (and devs who have to squeeze the code like this) pains me. I'm going to turn off the rule. Our official WebKit style can be PEP8 - line wrapping. I'd also be OK with some higher limit that we'd never hit, like 120 or 160 or whatever the longest line in the codebase is. If someone wants to reformat all the code and then turn back on the rule, we could argue about the formatting in that bug. (In reply to comment #32) > (In reply to comment #31) > > > Personally I think we should strike this from our style guide. It adds no > > > value to our code. Anyone who wants a fully-PEP8 webkit can run a re-wrapping > > > script. > > > > We had this discussion on webkit-dev. The conclusion was that there's value in > > match PEP8 fully instead of picking and choosing what rules we care about. > > fwiw, I agree with Eric (but I don't do much python coding in wk lately). > > Seeing this in practice and how it tortures code (and devs who have to squeeze > the code like this) pains me. Personally, I dislike longer lines and am okay with the guideline. Maybe there is some middle ground (like 90 or 100 chars). This reminds me. There is a section of scm_unittests.py which currently needs the extra spaces at end of line, violating W291. We may have to adjust scm_unittests.py or ignore W291 for that file. Created attachment 52345 [details]
Patch
Committed r56942: <http://trac.webkit.org/changeset/56942> (In reply to comment #35) > This reminds me. There is a section of scm_unittests.py which currently needs > the extra spaces at end of line, violating W291. We may have to adjust > scm_unittests.py or ignore W291 for that file. That's not too unusual for unit tests testing patch-related code. There are a few other Perl and Python files I can think of that have that same issue or a similar issue. I think it's okay just to continue ignoring them. Since check-webkit-style only reports errors in changed lines, we'll only be seeing those errors when the actual unit test cases change. (In reply to comment #36) > Created an attachment (id=52345) [details] > Patch @@ -47,6 +47,11 @@ class PythonProcessor(object): pep8_code = text[:4] pep8_message = text[5:] + # We ignore PEP8/E501 -- line limit of 79 characters. Most of our + # python code fails E501 and the rest of WebKit has no wrap limit. + if pep8_code == "E501": + return + category = "pep8/" + pep8_code It probably would have been better to do this in the calling code rather than in the processor itself -- e.g. by adding an additional rule to the _BASE_FILTER_RULES exclusion list: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py#L72 Otherwise, someone can't check for long lines using the custom --filter-rules option even if they wanted to (e.g. as I did above in comment 25). rs=me to fix my hack to be more elegant. :) (In reply to comment #40) > rs=me to fix my hack to be more elegant. :) Rewrote the patch here: http://trac.webkit.org/changeset/56973 Thanks! |