Closed Bug 1468273 Opened 6 years ago Closed 5 years ago

flake8/pep8 more code

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(10 files, 2 obsolete files)

Bug 1468273 - autopep8 on gfx/
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1468273 - Fix flake8/pep8 issues by hand in gfx/
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1468273 - Fix flake8/pep8 in intl/
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1468273 - autopep8 on ipc/
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1468273 - Fix flake8/pep8 issue by hand in ipc/
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1468273 - autopep8 on dom/
59 bytes, text/x-review-board-request
Details
Bug 1468273 - Fix flake8/pep8 issue by hand in dom/
59 bytes, text/x-review-board-request
bzbarsky
: review-
Details
Bug 1468273 - Add intl/, ipc/ and dom/ directories in flake8
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1468273 - Ignore ipc/chromium
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1468273 - Add closure-lib and intl/icu into the flake8 ignore list
59 bytes, text/x-review-board-request
ahal
: review+
Details 
One more flight to SF, more flake8 fixes!
Comment on attachment 8984918 [details]
Bug 1468273 - Add intl/, ipc/ and dom/ directories in flake8

https://reviewboard.mozilla.org/r/250708/#review257086
Attachment #8984918 - Flags: review?(ahal) → review+
Comment on attachment 8985122 [details]
Bug 1468273 - Add closure-lib and intl/icu into the flake8 ignore list

https://reviewboard.mozilla.org/r/250830/#review257088
Attachment #8985122 - Flags: review?(ahal) → review+
Attachment #8984910 - Flags: review?(ted)
Attachment #8984911 - Flags: review?(ted)
Attachment #8984912 - Flags: review?(nfroyd)
Attachment #8984913 - Flags: review?(nfroyd)
Attachment #8984914 - Flags: review?(nfroyd)
Attachment #8984915 - Flags: review?(nfroyd)
Attachment #8984916 - Flags: review?(n.nethercote)
Attachment #8984917 - Flags: review?(n.nethercote)
Attachment #8984919 - Flags: review?(ahal)
Attachment #8985121 - Flags: review?(n.nethercote)
sylvestre: sorry, I'm not a DOM peer so I don't think I should review those patches.
Attachment #8984916 - Flags: review?(n.nethercote)
Attachment #8984917 - Flags: review?(n.nethercote)
Attachment #8985121 - Flags: review?(n.nethercote)
Comment on attachment 8984919 [details]
Bug 1468273 - Ignore ipc/chromium

https://reviewboard.mozilla.org/r/250710/#review257474
Attachment #8984919 - Flags: review?(ahal) → review+
Comment on attachment 8984912 [details]
Bug 1468273 - Fix flake8/pep8 in intl/

https://reviewboard.mozilla.org/r/250696/#review257678
Attachment #8984912 - Flags: review?(nfroyd) → review+
Comment on attachment 8984913 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in intl/

https://reviewboard.mozilla.org/r/250698/#review257680

Please fold this with the previous commit before landing.  Thanks!
Attachment #8984913 - Flags: review?(nfroyd) → review+
Comment on attachment 8984910 [details]
Bug 1468273 - autopep8 on gfx/

https://reviewboard.mozilla.org/r/250692/#review257720

::: tools/lint/flake8.yml:24
(Diff revision 1)
>          - python/mozlint
>          - python/mozterm
>          - python/mozversioncontrol
>          - security/
>          - security/manager
> +        - servo/

This change feels unrelated.
Attachment #8984910 - Flags: review?(ted) → review+
Comment on attachment 8984911 [details]
Bug 1468273 - Fix flake8/pep8 issues by hand in gfx/

https://reviewboard.mozilla.org/r/250694/#review257722

::: gfx/gl/GLParseRegistryXML.py:133
(Diff revision 1)
>  
>      def loadXML(self, path):
>          xmlPath = getXMLDir() + path
>  
>          if not os.path.isfile(xmlPath):
> -            print 'missing file "' + xmlPath + '"'
> +            print('missing file "' + xmlPath + '"')

This is going to need a `from __future__ import print_function` to work.
Attachment #8984911 - Flags: review?(ted) → review+
Comment on attachment 8984914 [details]
Bug 1468273 - autopep8 on ipc/

https://reviewboard.mozilla.org/r/250700/#review258028

Not super excited about a patch that adds 400 lines, many of them whitespace, but pep8, whatcha gonna do?
Attachment #8984914 - Flags: review?(nfroyd) → review+
Comment on attachment 8984915 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in ipc/

https://reviewboard.mozilla.org/r/250702/#review258030

I am skeptical that some of these things are correct, or wanted.

::: ipc/ipdl/ipdl.py:16
(Diff revision 1)
>  import ipdl
>  
>  
>  def log(minv, fmt, *args):
>      if _verbosity >= minv:
> -        print fmt % args
> +        print(fmt % args)

I think Ted has already mentioned that this needs appropriate `from __future__` magic.

::: ipc/ipdl/ipdl/cgen.py:45
(Diff revision 1)
>          self.printed = printed
>  
>      def visitTranslationUnit(self, tu):
>          self.printed.add(tu.filename)
>          self.println('//\n// Automatically generated by ipdlc\n//')
> -        CodeGen.visitTranslationUnit(self, tu)
> +        self.visitTranslationUnit(self, tu)

Uh.  We're calling the superclass's method here, why did this get converted to call `self` instead?

::: ipc/ipdl/ipdl/lower.py:11
(Diff revision 1)
> -from ipdl.cxx.ast import *
> +from ipdl.cxx.ast import (File, Block, CaseLabel, Class, ConstructorDecl, ConstructorDefn,
> +                          CppDirective, Decl, DefaultLabel, DestructorDecl, DestructorDefn,
> +                          ExprAddrOf, ExprAssn, ExprBinary, ExprCall, ExprCast, ExprConditional,
> +                          ExprDelete, ExprDeref, ExprLambda, ExprLiteral, ExprMemberInit, ExprMove,
> +                          ExprNew, ExprNot, ExprPrefixUnop, ExprSelect, ExprVar, ForwardDecl,
> +                          FriendClassDecl, FunctionDecl, FunctionDefn, Inherit, Label, MethodDecl,
> +                          MethodDefn, MethodSpec, Namespace, Param, StmtBlock, StmtBreak, StmtDecl,
> +                          StmtExpr, StmtFor, StmtIf, StmtRangedFor, StmtReturn, StmtSwitch, Type,
> +                          Typedef, TypeEnum, TypeFunction, TypeUnion, Visitor, Whitespace)

I can't see that this is an improvement.

::: ipc/ipdl/ipdl/parser.py:8
(Diff revision 1)
>  
>  import os
> -import sys
>  from ply import lex, yacc
>  
> -from ipdl.ast import *
> +from ipdl.ast import ASYNC, CxxInclude, IN, Include, INOUT, INTR, Loc, Manager, ManagesStmt, MessageDecl, Namespace, NORMAL_PRIORITY, NOT_NESTED, OUT, Param, Protocol, QualifiedId, StructDecl, StructField, SYNC, TranslationUnit, TypeSpec, UnionDecl, UsingStmt  # NOQA: E501

Please no.

::: ipc/ipdl/ipdl/type.py:691
(Diff revision 1)
>          if not tu.protocol:
>              # header
>              expectedfilename += 'h'
>          if basefilename != expectedfilename:
>              self.error(tu.loc,
> -                       "expected file for translation unit `%s' to be named `%s'; instead it's named `%s'",
> +                       "expected file for translation unit `%s' to be named `%s'; instead it's named `%s'",  # NOQA: E501

What are all these comments doing?
Attachment #8984915 - Flags: review?(nfroyd) → review-
Comment on attachment 8984913 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in intl/

https://reviewboard.mozilla.org/r/250698/#review258032

Whoops, we shouldn't be modifying `intl/icu/`.

::: intl/icu/source/tools/icu-svnprops-check.py:51
(Diff revision 1)
>          if re.match("\s*(#.*)?$", propline):         # Match comment and blank lines
>              continue
>          if re.match("\s*\[auto-props\]", propline):  # Match the [auto-props] line.
>              continue
>          if not re.match("\s*[^\s]+\s*=", propline):  # minimal syntax check for <file-type> =
> -            print "Bad line from autoprops definitions: " + propline
> +            print("Bad line from autoprops definitions: " + propline)

As previously mentioned, this needs `from __future__` magic...but this file is not our code (`intl/icu/` is upstream), so I don't believe these modifications should be done.
Attachment #8984913 - Flags: review+ → review-
Comment on attachment 8984912 [details]
Bug 1468273 - Fix flake8/pep8 in intl/

https://reviewboard.mozilla.org/r/250696/#review258034

As with the manual fixup patch, we shouldn't be touching `intl/icu/`.
Attachment #8984912 - Flags: review+ → review-
Comment on attachment 8984913 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in intl/

https://reviewboard.mozilla.org/r/250698/#review258032

Fixed!
Attachment #8984913 - Attachment is obsolete: true
Attachment #8985121 - Attachment is obsolete: true
Attachment #8984912 - Flags: review?(nfroyd)
Attachment #8984915 - Flags: review?(nfroyd)
Attachment #8984916 - Flags: review?(bzbarsky)
Attachment #8984917 - Flags: review?(bzbarsky)
Assignee: nobody → sledru
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #22)
> > +            print('missing file "' + xmlPath + '"')
> 
> This is going to need a `from __future__ import print_function` to work.
Are you sure? Instead, adding this is breaking the build with:

   File "/home/sylvestre/dev/mozilla/mozilla-inbound/ipc/ipdl/ipdl.py", line 145, in <module>
     print >>ipcmsgstart, """
 TypeError: unsupported operand type(s) for >>: 'builtin_function_or_method' and 'cStringIO.StringO'

While it works without it.
Flags: needinfo?(ted)
I'm sorry for the lag here.  Some of the formatting changes being made aren't making much sense to me; I need to figure out whether they're forced by pep8 or just something the tools are doing.  Specifically:

1) The changes to all dictionaries to not have a space between the '{' and the first key name.  Why is that?
2) The changes to the formatting of if statements when the if condition spans multiple lines.
   I thought we'd already had a discussion about that and decided to not mess with those?

Basically, I'm not sure how much of the "autopep8" patch is changes we actually wanted to make and how much is just the tool making changes.  Doubly so because I'm pretty sure some of the code being touched has already been run through flake8/pep8 stuff before and now we're changing things we didn't change then.

Some context here would be pretty helpful.
Flags: needinfo?(sledru)
The end goal is to enable mozlint for every Python code of Firefox (of course, not thirdparty code).
Once we do that, for free, we will get flake8 executed at review phase and the CI (also at dev phase if needed).
Unlike static analysis, we need to have a clean tree before enabling it. A few of us have been clean the source tree, directory after directory (see bug 1155970)

> 1) The changes to all dictionaries to not have a space between the '{' and the first key name.  Why is that?
This is the pep8 coding style. One of the advantage of Python, it has a normalized coding style.
To be clear, my goal was to have flake8 not complaining. We disable some warnings

> 2) The changes to the formatting of if statements when the if condition spans multiple lines.
>    I thought we'd already had a discussion about that and decided to not mess with those?
Sorry if I forgot about something you told me in the past.
The limit is at 99 chars per line. I don't like it either. When relevant, I am silencing the warning (the "NOQA: E501" are for this).
I can silent the "if too long warning" if you want.

> flake8/pep8 stuff before and now we're changing things we didn't change then.
Maybe, new versions of autopep8 or flake8? These projects are still updated!

Last but not least, flake8 is doing more than coding style. I helps removing deadcode but also finds some errors (use of non existing variables).
Flags: needinfo?(sledru)
Comment on attachment 8984912 [details]
Bug 1468273 - Fix flake8/pep8 in intl/

https://reviewboard.mozilla.org/r/250696/#review264050
Attachment #8984912 - Flags: review?(nfroyd) → review+
Comment on attachment 8984915 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in ipc/

https://reviewboard.mozilla.org/r/250702/#review264052
Attachment #8984915 - Flags: review?(nfroyd) → review+
> The end goal is to enable mozlint for every Python code of Firefox (of course, not thirdparty code).

OK, so the goal is to appease a particular opinionated tool, not just comply with pep8?

> This is the pep8 coding style.

OK.

> The limit is at 99 chars per line.

That's not the issue.  The issue is changes like this:

>         if (self.descriptor.hasUnforgeableMembers and
>-            not self.descriptor.isGlobal()):
>+                not self.descriptor.isGlobal()):
>             slotCount += " + 1 /* slot for the JSObject holding the unforgeable properties */"

pep8 explicitly calls out the before-change version here as acceptable.  This is the thing that has come up before and where I thought the consensus was that if the condition is enclosed in parens we can avoid adding the an indent to the non-first condition lines.

Or is this coming back to flake8 trying to enforce things that pep8 doesn't require?
Flags: needinfo?(sledru)
Oh, and the previous bits here for dom/bindings were bug 1142609 though there may have been some newsgroup discussion too.

As in bug 1142609 it would be somewhat simpler to evaluate the changes to at least dom/bindings/Codegen.py if there were separate patches fixing separate warnings or at least classes of warnings.   I can deal with the megapatch if I have to, but it does make iteration a lot more complicated if that ends up needing to happen.
One other thing.  I just tried running flake8 on dom/bindings/Codegen.py and it doesn't show any warnings at all for the lines I quote in comment 53.  Another reason it would be really helpful to understand which warnings specific changes claim to be fixing, so I can evaluate whether the changes are in fact doing that or not.
(I should note that autopep8 claims it wants to make 59 different E??? fixes while flake8 claims that there are 10 different E??? issues that need addressing... So it sure sounds like autopep8 is a lot more picky than flake8 or something.)
Assuming autopep8 works the same as flake8, it could be that it's given an explicit list of errors to ignore, which actually _replaces_ the default list of errors to ignore, effectively enabling all the ones that aren't listed (found out about this "feature" recently)
Fwiw, I ran autopep8 and flake8 with no errors-to-ignore arguments in my test that led to comment 56.  So they just have different defaults or something...
As always, Mike is right.
If I remove .flake8 in the root directory, and run flak8 again, I am getting
[...]
dom/bindings/Codegen.py:657:13: E129 visually indented line with same indent as next logical line
[...]
With flake8 v3.5.0

Running autopep8 (1.3.4), which doesn't take in account .flake8, is going to reformat to:
         if (self.descriptor.hasUnforgeableMembers and
-            not self.descriptor.isGlobal()):
+                not self.descriptor.isGlobal()):

The pep8 coding style doesn't make a choice:
https://www.python.org/dev/peps/pep-0008/?#indentation
----
This PEP takes no explicit position on how (or whether) to further visually distinguish such conditional lines from the nested suite inside the if-statement.
----


Boris, please let me know what you want me to do here.

anyway, I will create a followup bug for this as all the other patches have been r+.
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #38)
> (In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #22)
> > > +            print('missing file "' + xmlPath + '"')
> > 
> > This is going to need a `from __future__ import print_function` to work.
> Are you sure? Instead, adding this is breaking the build with:
> 
>    File "/home/sylvestre/dev/mozilla/mozilla-inbound/ipc/ipdl/ipdl.py", line
> 145, in <module>
>      print >>ipcmsgstart, """
>  TypeError: unsupported operand type(s) for >>: 'builtin_function_or_method'
> and 'cStringIO.StringO'
> 
> While it works without it.

Oh, ugh. `print('foo')` works in Python 2 by accident, because it's printing the expression `('foo')`. This is super misleading. If you're not going to fix the whole file to use `print_function` then leave the parentheses off of this line to make it clear.
Flags: needinfo?(ted)
> Boris, please let me know what you want me to do here.

I'd really like us to keep things that look like this:

  if (foo and
      bar):
      doStuff()

as they are now, since that's a style we use widely and PEP8 explicitly allows.

Past that, just having separate diffs for separate errors, if practical, would be helpful.
Comment on attachment 8984917 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in dom/

https://reviewboard.mozilla.org/r/250706/#review264814

dom/canvas/test/webgl-conf/checkout/ is third-party code that should be fixed upstream then updated, not changed here here.

Some of these other bits (e.g. the websocket parts) I am not familiar with at all; it would be better to get those reviewed by whever created or reviewed that code if possible.  This will be a bit clearer in a broken-out version of the patches...

::: dom/bindings/parser/tests/test_any_null.py:10
(Diff revision 3)
>              interface DoubleNull {
>                attribute any? foo;
>              };
>          """)
>  
> -        results = parser.finish()
> +        parser.finish()

Why this change here and some places but not other places?  Again, would be good to understand what we're really after here.

Note that this variable is mostly there for debugging purposes, making it easier to examine the parser output.

::: dom/bindings/parser/tests/test_float_types.py:111
(Diff revision 3)
>              interface FloatTypes {
>                [LenientFloat]
>                void m((unrestricted float or FloatTypes) arg);
>              };
>          """)
> -    except Exception, x:
> +    except Exception:

Why is this change being made?

In general, some of the tests have the explicit variable there to make it easier to debug the test as needed...  We could remove it at this point, I guess, at the loss of some ease of debugging.

::: dom/bindings/parser/tests/test_global_extended_attr.py:28
(Diff revision 3)
>              getter any(DOMString name);
>              setter void(DOMString name, any arg);
>            };
>          """)
>          results = parser.finish()
> -    except:
> +    except Exception:

Why this change?  Again, feels like explicit commit messages explaining which problems are being fixed would go a long way here.
Attachment #8984917 - Flags: review?(bzbarsky) → review-
Comment on attachment 8984917 [details]
Bug 1468273 - Fix flake8/pep8 issue by hand in dom/

https://reviewboard.mozilla.org/r/250706/#review264814

ok, thanks

> Why this change here and some places but not other places?  Again, would be good to understand what we're really after here.
> 
> Note that this variable is mostly there for debugging purposes, making it easier to examine the parser output.

Because the variable wasn't used. 
I can silent the warning in that case if you want.

> Why is this change being made?
> 
> In general, some of the tests have the explicit variable there to make it easier to debug the test as needed...  We could remove it at this point, I guess, at the loss of some ease of debugging.

Because this isn't (anymore?) valid python code.
If you want to retrieve the Exception, the correct syntax for exception is:
except Exception as x:

> Why this change?  Again, feels like explicit commit messages explaining which problems are being fixed would go a long way here.

This is part of the python coding style:
http://pycodestyle.pycqa.org/en/latest/intro.html
"do not use bare except, specify exception instead"
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #65)
> ::: dom/bindings/parser/tests/test_global_extended_attr.py:28
> (Diff revision 3)
> >              getter any(DOMString name);
> >              setter void(DOMString name, any arg);
> >            };
> >          """)
> >          results = parser.finish()
> > -    except:
> > +    except Exception:
> 
> Why this change?  Again, feels like explicit commit messages explaining
> which problems are being fixed would go a long way here.

For context, blank excepts are frowned upon in python because they catch things like SystemExit and KeyboardInterrupt. It's probably never the case that you meant to handle those the same way you would handle an error, so it's better to be explicit and catch Exception (which does not include SystemExit or KeyboardInterrupt).
> Because this isn't (anymore?) valid python code.

Ah, I guess this is one of those incompatible Python3 changes.  It's definitely valid Python2; see syntax definition at https://docs.python.org/2/reference/compound_stmts.html#except

> This is part of the python coding style:

OK, thank you for explaining.  Again, this sort of thing should ideally be in commit messages so it's clear why changes are happening.
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.