Bug 37477 – check-webkit-style isn't enforcing 80-column limits in python files for PEP-8 compliance

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Dirk Pranke]

My understanding is that we're trying to stay within 80-column files for strict PEP-8 compliance, but for some reason check-webkit-style isn't enforcing this.

Which is odd, since I think the pep8 script that we're using does this by default. Did we tweak a setting for some reason, or are we not trying to follow this?

[Dirk Pranke]

Created attachment 53202 [details]
Patch

[Chris Jerdonek]

(In reply to comment #0)
> My understanding is that we're trying to stay within 80-column files for strict
> PEP-8 compliance, but for some reason check-webkit-style isn't enforcing this.
> 
> Which is odd, since I think the pep8 script that we're using does this by
> default. Did we tweak a setting for some reason, or are we not trying to follow
> this?

That change is something Eric wanted.  At the moment I can't find the bug report where we discussed it, but the code change is here:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=57467#L94

[David Levin]

Comment on attachment 53202 [details]
Patch

This patch isn't a fix for the bug to which it is attached.

This patch fixes the bug "Python scripts need to be reformatted to fit in 80 columns."

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 53202 [details])
> This patch isn't a fix for the bug to which it is attached.
> 
> This patch fixes the bug "Python scripts need to be reformatted to fit in 80
> columns."

Um, right. I'm not sure why I put this patch on this bug; I'll split it out ...

[Dirk Pranke]

see bug 37586.

[Chris Jerdonek]

(In reply to comment #2)
> (In reply to comment #0)
> > My understanding is that we're trying to stay within 80-column files for strict
> > PEP-8 compliance, but for some reason check-webkit-style isn't enforcing this.
> > 
> > Which is odd, since I think the pep8 script that we're using does this by
> > default. Did we tweak a setting for some reason, or are we not trying to follow
> > this?
> 
> That change is something Eric wanted.  At the moment I can't find the bug
> report where we discussed it, but the code change is here:
> 
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=57467#L94

FYI, the discussion around not enforcing the 79-character limit took place here:

https://bugs.webkit.org/show_bug.cgi?id=33639#c30

I think we are still open to enforcing that or some longer limit, but Eric preferred that existing lines be fixed before we discuss reimposing a limit.

[Dirk Pranke]

The comments in bug 33639 talk about turning this feature off until the rest of the code is brought into compliance. As this bug indicates, that'll never happen - the people who don't use 80 columns by default will do what they're used to and more code will fail to comply, not less.

I also believe that the people who care about there being any real limit on line width only care about 80 columns (i.e., if you make the limit 81 or 82, let alone 120, you'll still piss people off). 

As Adam commented in 33639, the issue of Python style compliance had been discussed before on the broader and more appropriately place (webkit-dev). See https://lists.webkit.org/pipermail/webkit-dev/2010-January/011423.html

It seems inappropriate that would would make this decision and change it in a patch that didn't get wider discussion or review. Can we make this decision once and for all (yeah right) on webkit-dev and then document it in the WebKit style guidelines so that enough people can weigh in and we can get a definitive record? If the majority of people that care think we should change it, then by all means, change it, but don't just do it on the sly.

[Chris Jerdonek]

(In reply to comment #7)
> It seems inappropriate that would would make this decision and change it in a
> patch that didn't get wider discussion or review. Can we make this decision
> once and for all (yeah right) on webkit-dev and then document it in the WebKit
> style guidelines so that enough people can weigh in and we can get a definitive
> record?

I would recommend going ahead and raising this on webkit-dev yourself rather than waiting for someone else to raise it.

> If the majority of people that care think we should change it, then by
> all means, change it, but don't just do it on the sly.

Just to be clear, I hope you're not talking to me. :)  Since I'm the main other/previous commenter in this report, it might appear so.  I actually voiced against the change prior to it being committed:

https://bugs.webkit.org/show_bug.cgi?id=33639#c34

[Dirk Pranke]

> Just to be clear, I hope you're not talking to me. :)  

No, I was not talking to you ;)

-- Dirk

[Dirk Pranke]

Closing this ... doesn't look like I'm gonna convince Eric of the evil of his ways any time soon, and there is a certain logic to not enforcing this if we don't enforce it for c++ code either.