Bug 33639 – check-webkit-style: WebKit needs a python style checker

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

We also need to update the WebKit style guide html page to say that we try to follow PEP8.

[Eric Seidel (no email)]

http://pypi.python.org/pypi/pep8

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

http://www.python.org/dev/peps/pep-0008/ is the original PEP8 document by the way.

[Chris Jerdonek]

(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

[Chris Jerdonek]

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.

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

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

[Chris Jerdonek]

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

[Chris Jerdonek]

I'll take this on if no one else is.  Looks like it might be fun and not too time-consuming.

[Chris Jerdonek]

Added check-webkit-style: to the beginning of the bug title.

[Chris Jerdonek]

Created attachment 49439 [details]
Proposed patch

cq- since I would need to rebase the patch after committing the patch this report depends on.

[WebKit Review Bot]

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.

[Chris Jerdonek]

Created attachment 49442 [details]
Proposed patch 2

Added code comments to python_unittest_input.py so it is clearer what the style error is.

[WebKit Review Bot]

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.

[Chris Jerdonek]

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

[Adam Barth]

I'm super excited about this patch.  Thanks Chris!

[Chris Jerdonek]

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

[Chris Jerdonek]

Manually committed (via "git svn dcommit"):

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

[Chris Jerdonek]

Rolled out (via "git svn dcommit"):

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

Because of--

https://bugs.webkit.org/show_bug.cgi?id=35163#c21

[Chris Jerdonek]

(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!)

[Chris Jerdonek]

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.

[Chris Jerdonek]

Committed:

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

[Eric Seidel (no email)]

Have I mentioned recently how much I hate 80-col wrap?  What a waste of time...

[Chris Jerdonek]

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

[Eric Seidel (no email)]

Sorry.  My statement wasn't very helpful.

I don't understand your comment above?

[Eric Seidel (no email)]

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

[Chris Jerdonek]

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

[Adam Barth]

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

[Eric Seidel (no email)]

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

[Adam Barth]

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

[David Levin]

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

[Eric Seidel (no email)]

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.

[Chris Jerdonek]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 52345 [details]
Patch

[Eric Seidel (no email)]

Committed r56942: <http://trac.webkit.org/changeset/56942>

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Eric Seidel (no email)]

rs=me to fix my hack to be more elegant. :)

[Chris Jerdonek]

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