Closed Bug 1142609 Opened 7 years ago Closed 7 years ago

Python code in dom/bindings should follow the flake8 convention

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: Sylvestre, Assigned: samy, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=Python])

Attachments

(12 files, 50 obsolete files)

4.46 KB, patch
samy
: review+
Details | Diff | Splinter Review
24.51 KB, patch
samy
: review+
Details | Diff | Splinter Review
2.41 KB, patch
samy
: review+
Details | Diff | Splinter Review
1.69 KB, patch
samy
: review+
Details | Diff | Splinter Review
2.17 KB, patch
samy
: review+
Details | Diff | Splinter Review
18.49 KB, patch
samy
: review+
Details | Diff | Splinter Review
19.84 KB, patch
samy
: review+
Details | Diff | Splinter Review
30.26 KB, patch
samy
: review+
Details | Diff | Splinter Review
31.54 KB, patch
samy
: review+
Details | Diff | Splinter Review
56.44 KB, patch
samy
: review+
Details | Diff | Splinter Review
5.49 KB, patch
samy
: review+
Details | Diff | Splinter Review
25.19 KB, patch
samy
: review+
Details | Diff | Splinter Review
Run on the Firefox source code, the following line shows a bunch of warnings. Nothing critical but it would be nice to remove them:
$ flake8 --max-line-length=220 $(find dom/bindings -iname '*.py'|grep -v tests)

That should be pretty easy to fix for a beginner in Python.
It's not clear to me that we want to fix all of these (e.g. E201/E202).

If someone decides to work on this, I suggest writing separate patches for each separate kind of warning.
Assignee: nobody → samy
Attachment #8577225 - Flags: review?(peterv)
Attachment #8577226 - Flags: review?(peterv)
Attachment #8577227 - Flags: review?(peterv)
Attachment #8577228 - Flags: review?(peterv)
Attachment #8577230 - Flags: review?(peterv)
Attachment #8577229 - Flags: review?(peterv)
Comment on attachment 8577229 [details] [diff] [review]
Fix flake8's E124 warnings in dom/bindings's Python code

This change is clearly wrong, since now the ')' is commented out so you get invalid syntax exceptions when trying to run this code.  Have you actually tried these patches out?
Flags: needinfo?(samy)
(In reply to Not doing reviews right now from comment #8)
> Comment on attachment 8577229 [details] [diff] [review]
> Fix flake8's E124 warnings in dom/bindings's Python code
> 
> This change is clearly wrong, since now the ')' is commented out so you get
> invalid syntax exceptions when trying to run this code.  Have you actually
> tried these patches out?

That was reckless of me, I'm sorry I missed that! Fixing it right now.

I was actually wondering if there are any tests I could run to make sure my patches don't break anything. Are there?
Flags: needinfo?(samy)
Attachment #8577229 - Attachment is obsolete: true
Attachment #8577229 - Flags: review?(peterv)
Attachment #8579377 - Flags: review?(peterv)
Comment on attachment 8579377 [details] [diff] [review]
Fix flake8's E124 warnings in dom/bindings's Python code

The WebIDL.py diff in here doesn't look like a diff from the current tree state.  ;)

As far as tests, yes, there are.  First test is just compiling bindings ("mach build dom/bindings").  Second test is running the Web IDL parser tests ("mach webidl-parser-test").  Third test is doing a try run (see <https://wiki.mozilla.org/Build:TryServer>), which will obviously perform those two tests and many others.  If you don't have try access, attach a squashed patch that contains all these changesets, and I or Sylvestre can do a try push for you.
Comment on attachment 8577225 [details] [diff] [review]
Fix flake8's E127 warnings in dom/bindings's Python code

Review of attachment 8577225 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +293,5 @@
>              return originalObject.addOverload(newObject)
>  
>          # Default to throwing, derived classes can override.
>          conflictdesc = "\n\t%s at %s\n\t%s at %s" % \
> +        (originalObject, originalObject.location, newObject, newObject.location)

I think you should fix this by adding 2 spaces instead of removing 2

@@ +965,5 @@
>              if memberType in specialMembersSeen:
>                  raise WebIDLError("Multiple " + memberType + " on %s" % (self),
> +                                  [self.location,
> +                                  specialMembersSeen[memberType].location,
> +                                  member.location])

These last 2 should be aligned with |self.location|.

@@ +2719,5 @@
>  
>      def isUnrestricted(self):
>          assert self.isFloat()
>          return self._typeTag == IDLBuiltinType.Types.unrestricted_float or \
> +            self._typeTag == IDLBuiltinType.Types.unrestricted_double

I disagree with this, so I don't think we should do this. But I'm also confused why it would complain here, but not 5 lines earlier where we do the exact same thing.

@@ +3764,5 @@
>              overload = self._overloads[0]
>              arguments =  overload.arguments
>              assert len(arguments) == 1
>              assert arguments[0].type == BuiltinTypes[IDLBuiltinType.Types.domstring] or \
> +                arguments[0].type == BuiltinTypes[IDLBuiltinType.Types.unsigned_long]

Same here.

@@ +3773,5 @@
>              assert len(self._overloads) == 1
>              arguments = self._overloads[0].arguments
>              assert len(arguments) == 2
>              assert arguments[0].type == BuiltinTypes[IDLBuiltinType.Types.domstring] or \
> +                arguments[0].type == BuiltinTypes[IDLBuiltinType.Types.unsigned_long]

And here.

@@ +4869,5 @@
>              if arguments[0].optional or arguments[0].variadic:
>                  raise WebIDLError("%s cannot have %s argument" %
>                                    ("getter" if getter else "deleter",
>                                     "optional" if arguments[0].optional else "variadic"),
> +                                [arguments[0].location])

I think you should add 2 spaces here.
Attachment #8577225 - Flags: review?(peterv) → review+
Comment on attachment 8577226 [details] [diff] [review]
Fix flake8's E126 warnings in dom/bindings's Python code

Review of attachment 8577226 [details] [diff] [review]:
-----------------------------------------------------------------

-'ing so I can take another look after you take care of my comments.

::: dom/bindings/Codegen.py
@@ +4185,1 @@
>                  "declName": "slot" + str(nestingLevel),

It looks really wrong that these are not aligned anymore, please fix (here and all the others below).

@@ +15140,5 @@
>                                    "mozilla/ErrorResult.h",
>                                    "mozilla/dom/%sBinding.h" % interfaceName,
>                                    'mozilla/dom/BindingUtils.h',
> +            ],
> +            [

This looks really bad too, I think we should not do this. Is it complaining about the closing bracket alone? If so, we could align it to the line before it?

::: dom/bindings/parser/WebIDL.py
@@ +82,5 @@
>          self._file = filename if filename else "<unknown>"
>  
>      def __eq__(self, other):
>          return self._lexpos == other._lexpos and \
> +            self._file == other._file

Don't think we should do this.

@@ +120,5 @@
>          self.msg = text + "\n"
>  
>      def __eq__(self, other):
>          return isinstance(other, BuiltinLocation) and \
> +            self.msg == other.msg

And this.

@@ +1056,5 @@
>                      if fowardAttr is None:
>                          raise WebIDLError("Attribute %s on %s forwards to "
>                                            "missing attribute %s" %
> +                            (attr.identifier.name, iface, putForwards),
> +                            [attr.location])

Can we align these on the opening quotes?

@@ +2424,5 @@
>  
>      def __eq__(self, other):
>          return isinstance(other, IDLWrapperType) and \
> +            self._identifier == other._identifier and \
> +            self.builtin == other.builtin

Don't think we should do this.

@@ +2461,5 @@
>          return isinstance(self.inner, IDLDictionary)
>  
>      def isInterface(self):
>          return isinstance(self.inner, IDLInterface) or \
> +            isinstance(self.inner, IDLExternalInterface)

And this. I'm going to stop pointing this out, but there's more below.
Attachment #8577226 - Flags: review?(peterv) → review-
Comment on attachment 8577227 [details] [diff] [review]
Fix flake8's E111/E113 warnings in dom/bindings's Python code

Review of attachment 8577227 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3035,2 @@
>                                                 " &&\n"),
>                                          pre="return ", post=";\n", reindent=True)

Please keep things lined up to the parens.
Attachment #8577227 - Flags: review?(peterv) → review+
Comment on attachment 8577228 [details] [diff] [review]
Fix flake8's E122 warnings in dom/bindings's Python code

Review of attachment 8577228 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +14875,5 @@
>                              members += fill(
>                                  """
>                                  e->${varname}.set(${srcname}.Obj());
>                                  """,
> +                                varname=name, srcname=srcname)

Can you move srcname to its own line for consistency?
Attachment #8577228 - Flags: review?(peterv) → review+
Attachment #8577230 - Flags: review?(peterv) → review+
Attachment #8579377 - Flags: review?(peterv) → review+
We could perhaps address the backslash-continuation spacing weirdness by using parens instead of backslash-continuations...
Samy, any news on it?
Flags: needinfo?(samy)
Blocks: flake8
Attachment #8577227 - Attachment is obsolete: true
Flags: needinfo?(samy)
Attachment #8594480 - Flags: review?(peterv)
Attachment #8577228 - Attachment is obsolete: true
Attachment #8594481 - Flags: review?(peterv)
Attachment #8579377 - Attachment is obsolete: true
Attachment #8594482 - Flags: review?(peterv)
Attachment #8577230 - Attachment is obsolete: true
Attachment #8594483 - Flags: review?(peterv)
Attachment #8577226 - Attachment is obsolete: true
Attachment #8594484 - Flags: review?(peterv)
Attachment #8577225 - Attachment is obsolete: true
Attachment #8594485 - Flags: review?(peterv)
Attachment #8594486 - Flags: review?(peterv)
Attachment #8594487 - Flags: review?(peterv)
Attachment #8594488 - Flags: review?(peterv)
Attachment #8594489 - Flags: review?(peterv)
Attachment #8594490 - Flags: review?(peterv)
Attachment #8594491 - Flags: review?(peterv)
Attachment #8594480 - Attachment description: 1142609_E111_E113.patch → Fix flake8's E111/113 warnings in dom/bindings's Python code
Hi everyone,

I'm sorry for the silence.  
I just made a suite of patches that makes all the code flake8 compliant, taking into consideration Peter's comments.

(Peter, by the way, the " \" at the end of lines were there already. The new patches remove those that affect my changes)

Also, I made some lines shorter than 80 columns, forgetting it wasn't necessary. Tell me if I should revert the changes.

Thanks!
> (Peter, by the way, the " \" at the end of lines were there already. The new
> patches remove those that affect my changes)

s/that affect/affected by/
Samy, I think you should look for an other reviewer.
Flags: needinfo?(samy)
(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> Samy, I think you should look for an other reviewer.

Do you have anyone in mind?
Flags: needinfo?(samy)
You should have a look to the hg history of the files.
Attachment #8594480 - Flags: review?(peterv) → review?(jcoppeard)
@jcoppeard, can you please review my patches?
Thank you.
(In reply to Samy Dindane from comment #35)
Hi Samy, I'm not a DOM peer so I'm not really the right person to review this.

Maybe Ms2ger can suggest someone?

BTW I'm happy to review some of the patches if they are just python changes and not DOM-specific.
Flags: needinfo?(Ms2ger)
Peter will do it.
Flags: needinfo?(Ms2ger)
Why are none of the r+'s carried over? Did all of the patches change substantially since I did those reviews?
Comment on attachment 8577227 [details] [diff] [review]
Fix flake8's E111/E113 warnings in dom/bindings's Python code

# HG changeset patch
# User Samy Dindane <samy@dindane.com>
# Parent  8d8df22fe72d7dbbb63ff735afe965497669a465
Fix flake8's E111/E113 warnings in dom/bindings's Python code

diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -1013,17 +1013,17 @@ class CGHeaders(CGWrapper):
 
         # Grab the includes for checking hasInstance
         interfacesImplementingSelf = set()
         for d in descriptors:
             interfacesImplementingSelf |= d.interface.interfacesImplementingSelf
         implementationIncludes |= set(self.getDeclarationFilename(i) for i in
                                       interfacesImplementingSelf)
 
-         # Grab the includes for the things that involve XPCOM interfaces
+        # Grab the includes for the things that involve XPCOM interfaces
         hasInstanceIncludes = set("nsIDOM" + d.interface.identifier.name + ".h" for d
                                   in descriptors if
                                   d.interface.hasInterfaceObject() and
                                   NeedsGeneratedHasInstance(d) and
                                   d.interface.hasInterfacePrototypeObject())
 
         # Now find all the things we'll need as arguments because we
         # need to wrap or unwrap them.
@@ -1995,18 +1995,18 @@ class PropertyDefiner:
 
         # We want to generate a single list of specs, but with specTerminator
         # inserted at every point where the pref name controlling the member
         # changes.  That will make sure the order of the properties as exposed
         # on the interface and interface prototype objects does not change when
         # pref control is added to members while still allowing us to define all
         # the members in the smallest number of JSAPI calls.
         assert len(array) != 0
-        lastCondition = getCondition(array[0], self.descriptor)  # So we won't put a specTerminator
-                                                                 # at the very front of the list.
+        # So we won't put a specTerminator at the very front of the list.
+        lastCondition = getCondition(array[0], self.descriptor)
         specs = []
         prefableSpecs = []
 
         prefableTemplate = '  { true, %s, %s, %s, &%s[%d] }'
         prefCacheTemplate = '&%s[%d].enabled'
 
         def switchToCondition(props, condition):
             # Remember the info about where our pref-controlled
@@ -3104,21 +3104,21 @@ class CGConstructorEnabled(CGAbstractMet
         checkPermissions = self.descriptor.checkPermissionsIndex
         if checkPermissions is not None:
             conditions.append("CheckPermissions(aCx, aObj, permissions_%i)" % checkPermissions)
         # We should really have some conditions
         assert len(body) or len(conditions)
 
         conditionsWrapper = ""
         if len(conditions):
-          conditionsWrapper = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
+            conditionsWrapper = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
                                                " &&\n"),
                                         pre="return ", post=";\n", reindent=True)
         else:
-          conditionsWrapper = CGGeneric("return true;\n")
+            conditionsWrapper = CGGeneric("return true;\n")
 
         body.append(conditionsWrapper)
         return body.define()
 
 
 def CreateBindingJSObject(descriptor, properties):
     objDecl = "BindingJSObjectCreator<%s> creator(aCx);\n" % descriptor.nativeType
 
diff --git a/dom/bindings/parser/WebIDL.py b/dom/bindings/parser/WebIDL.py
--- a/dom/bindings/parser/WebIDL.py
+++ b/dom/bindings/parser/WebIDL.py
@@ -5623,18 +5623,18 @@ class Parser(Tokenizer):
             PrimitiveOrStringType : USVSTRING
         """
         p[0] = IDLBuiltinType.Types.usvstring
 
     def p_UnsignedIntegerTypeUnsigned(self, p):
         """
             UnsignedIntegerType : UNSIGNED IntegerType
         """
-        p[0] = p[2] + 1 # Adding one to a given signed integer type
-                        # gets you the unsigned type.
+        # Adding one to a given signed integer type gets you the unsigned type.
+        p[0] = p[2] + 1
 
     def p_UnsignedIntegerType(self, p):
         """
             UnsignedIntegerType : IntegerType
         """
         p[0] = p[1]
 
     def p_IntegerTypeShort(self, p):
Attachment #8577227 - Attachment is obsolete: false
Attachment #8594480 - Attachment is obsolete: true
Attachment #8594480 - Flags: review?(jcoppeard)
Comment on attachment 8577227 [details] [diff] [review]
Fix flake8's E111/E113 warnings in dom/bindings's Python code

# HG changeset patch
# User Samy Dindane <samy@dindane.com>
# Parent  8d8df22fe72d7dbbb63ff735afe965497669a465
Fix flake8's E111/E113 warnings in dom/bindings's Python code

diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -1013,17 +1013,17 @@ class CGHeaders(CGWrapper):
 
         # Grab the includes for checking hasInstance
         interfacesImplementingSelf = set()
         for d in descriptors:
             interfacesImplementingSelf |= d.interface.interfacesImplementingSelf
         implementationIncludes |= set(self.getDeclarationFilename(i) for i in
                                       interfacesImplementingSelf)
 
-         # Grab the includes for the things that involve XPCOM interfaces
+        # Grab the includes for the things that involve XPCOM interfaces
         hasInstanceIncludes = set("nsIDOM" + d.interface.identifier.name + ".h" for d
                                   in descriptors if
                                   d.interface.hasInterfaceObject() and
                                   NeedsGeneratedHasInstance(d) and
                                   d.interface.hasInterfacePrototypeObject())
 
         # Now find all the things we'll need as arguments because we
         # need to wrap or unwrap them.
@@ -1995,18 +1995,18 @@ class PropertyDefiner:
 
         # We want to generate a single list of specs, but with specTerminator
         # inserted at every point where the pref name controlling the member
         # changes.  That will make sure the order of the properties as exposed
         # on the interface and interface prototype objects does not change when
         # pref control is added to members while still allowing us to define all
         # the members in the smallest number of JSAPI calls.
         assert len(array) != 0
-        lastCondition = getCondition(array[0], self.descriptor)  # So we won't put a specTerminator
-                                                                 # at the very front of the list.
+        # So we won't put a specTerminator at the very front of the list.
+        lastCondition = getCondition(array[0], self.descriptor)
         specs = []
         prefableSpecs = []
 
         prefableTemplate = '  { true, %s, %s, %s, &%s[%d] }'
         prefCacheTemplate = '&%s[%d].enabled'
 
         def switchToCondition(props, condition):
             # Remember the info about where our pref-controlled
@@ -3104,21 +3104,21 @@ class CGConstructorEnabled(CGAbstractMet
         checkPermissions = self.descriptor.checkPermissionsIndex
         if checkPermissions is not None:
             conditions.append("CheckPermissions(aCx, aObj, permissions_%i)" % checkPermissions)
         # We should really have some conditions
         assert len(body) or len(conditions)
 
         conditionsWrapper = ""
         if len(conditions):
-          conditionsWrapper = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
+            conditionsWrapper = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
                                                " &&\n"),
                                         pre="return ", post=";\n", reindent=True)
         else:
-          conditionsWrapper = CGGeneric("return true;\n")
+            conditionsWrapper = CGGeneric("return true;\n")
 
         body.append(conditionsWrapper)
         return body.define()
 
 
 def CreateBindingJSObject(descriptor, properties):
     objDecl = "BindingJSObjectCreator<%s> creator(aCx);\n" % descriptor.nativeType
 
diff --git a/dom/bindings/parser/WebIDL.py b/dom/bindings/parser/WebIDL.py
--- a/dom/bindings/parser/WebIDL.py
+++ b/dom/bindings/parser/WebIDL.py
@@ -5623,18 +5623,18 @@ class Parser(Tokenizer):
             PrimitiveOrStringType : USVSTRING
         """
         p[0] = IDLBuiltinType.Types.usvstring
 
     def p_UnsignedIntegerTypeUnsigned(self, p):
         """
             UnsignedIntegerType : UNSIGNED IntegerType
         """
-        p[0] = p[2] + 1 # Adding one to a given signed integer type
-                        # gets you the unsigned type.
+        # Adding one to a given signed integer type gets you the unsigned type.
+        p[0] = p[2] + 1
 
     def p_UnsignedIntegerType(self, p):
         """
             UnsignedIntegerType : IntegerType
         """
         p[0] = p[1]
 
     def p_IntegerTypeShort(self, p):
Please ignore the last two comments, still learning… Sorry for the noise.
This patch is the same as the previous one (1142609_E111_E113.patch) with the first hunk's position updated.
Attachment #8577227 - Attachment is obsolete: true
Attachment #8608377 - Attachment description: 1142609_E111_E113_2.patch → Fix flake8's E111/E113 warnings in dom/bindings's Python code
Attachment #8608377 - Flags: review?(peterv)
E121 errors recently appeared.
Attachment #8608393 - Flags: review?(peterv)
Attachment #8594481 - Attachment is obsolete: true
Attachment #8594481 - Flags: review?(peterv)
Comment on attachment 8594481 [details] [diff] [review]
Fix flake8's E122 warnings in dom/bindings's Python code

This patch is the same as the previous one (1142609_E122.patch) with the last hunk removed (because it's obsolete) and "srcname=srcname" put in its own line.
Attachment #8594481 - Attachment filename: 1142609_E122.patch → 1142609_E122_2.patch
Attachment #8594481 - Attachment is obsolete: false
Comment on attachment 8594482 [details] [diff] [review]
Fix flake8's E124 warnings in dom/bindings's Python code

The previous r+ is obsolete as there were some "substantial" changes to the code. :)
Comment on attachment 8577230 [details] [diff] [review]
Fix flake8's E125 warnings in dom/bindings's Python code

The difference between this patch and https://bugzilla.mozilla.org/attachment.cgi?id=8594483 is the commit name (no "Bug 1142609 -" prefix in the other one). The changes are the same.
Please feel free to keep the one you think most suitable.
Updated for latest code changes.
Attachment #8594484 - Attachment is obsolete: true
Attachment #8594484 - Flags: review?(peterv)
Attachment #8608414 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594485 - Attachment is obsolete: true
Attachment #8594485 - Flags: review?(peterv)
Attachment #8608422 - Flags: review?(peterv)
Attachment #8594485 - Attachment is obsolete: false
Comment on attachment 8608422 [details] [diff] [review]
Fix flake8's E127 warnings in dom/bindings's Python code (Part 2)

I need to merge 1142609_E127.patch and 1142609_E127_2.patch. I'll do it ASAP.
Attachment #8608422 - Attachment description: Fix flake8's E127 warnings in dom/bindings's Python code → Fix flake8's E127 warnings in dom/bindings's Python code (Part 2)
I need to update E127, E128, E129 and E(2|3|5|7)xx.

Feel free to merge E111/E113, E121, E122, E124 and E125 meanwhile, and in order please, as they're dependent on one another.

Thank you.
Updated for the latest code changes.
Attachment #8594485 - Attachment is obsolete: true
Attachment #8608422 - Attachment is obsolete: true
Attachment #8608422 - Flags: review?(peterv)
Attachment #8608570 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594486 - Attachment is obsolete: true
Attachment #8594486 - Flags: review?(peterv)
Attachment #8608571 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594487 - Attachment is obsolete: true
Attachment #8594487 - Flags: review?(peterv)
Attachment #8608577 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594488 - Attachment is obsolete: true
Attachment #8594488 - Flags: review?(peterv)
Attachment #8608583 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594489 - Attachment is obsolete: true
Attachment #8594489 - Flags: review?(peterv)
Attachment #8608586 - Flags: review?(peterv)
Updated for the latest code changes.
Attachment #8594491 - Attachment is obsolete: true
Attachment #8594491 - Flags: review?(peterv)
Attachment #8608588 - Flags: review?(peterv)
E131 errors recently appeared.
Attachment #8608592 - Flags: review?(peterv)
I think we're good.
I updated all the patches to work with latest changes in dom/bindings.

Peter, can you please review and merge them if you think they're good?
The order is: E111/E113 > E121 > E122 > E124 > E125 > E126 > E127 > E128 > E129 > E131 > E2xx > E3xx > E5xx > E7xx.

To quickly try the changes, run:
flake8 --max-line-length=220 $(find dom/bindings -iname '*.py'|grep -v tests)|grep "E\d\d\d"|wc -l
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608377
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608393
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8594481
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8594482
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8594483
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608414
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608570
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608571
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608577
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608592
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608583
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608586
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8594490
hg import https://bugzilla.mozilla.org/attachment.cgi\?id\=8608588
flake8 --max-line-length=220 $(find dom/bindings -iname '*.py'|grep -v tests)|grep "E\d\d\d"|wc -l # should be 0
./mach build dom/bindings && ./mach webidl-parser-test

Thank you.
Attachment #8594481 - Flags: review?(peterv)
Comment on attachment 8594482 [details] [diff] [review]
Fix flake8's E124 warnings in dom/bindings's Python code

Already reviewed.
Attachment #8594482 - Flags: review?(peterv)
Comment on attachment 8594483 [details] [diff] [review]
Fix flake8's E125 warnings in dom/bindings's Python code

Already reviewed.
Attachment #8594483 - Flags: review?(peterv)
Comment on attachment 8594481 [details] [diff] [review]
Fix flake8's E122 warnings in dom/bindings's Python code

Already reviewed.
Attachment #8594481 - Flags: review?(peterv)
Comment on attachment 8608377 [details] [diff] [review]
Fix flake8's E111/E113 warnings in dom/bindings's Python code

Already reviewed.
Attachment #8608377 - Flags: review?(peterv)
Comment on attachment 8608570 [details] [diff] [review]
Fix flake8's E127 warnings in dom/bindings's Python code

Already reviewed.
Attachment #8608570 - Flags: review?(peterv)
Attachment #8608377 - Flags: review+
Attachment #8594481 - Flags: review+
Attachment #8594482 - Flags: review+
Attachment #8594483 - Flags: review+
Attachment #8608570 - Flags: review+
Attachment #8594490 - Flags: review?(peterv) → review+
Comment on attachment 8608393 [details] [diff] [review]
Fix flake8's E121 warnings in dom/bindings's Python code

Review of attachment 8608393 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +2750,1 @@
>  

I don't think we should do this.
Attachment #8608393 - Flags: review?(peterv) → review+
Comment on attachment 8608414 [details] [diff] [review]
Fix flake8's E126 warnings in dom/bindings's Python code

Review of attachment 8608414 [details] [diff] [review]:
-----------------------------------------------------------------

There's a couple here that I think we shouldn't do.

::: dom/bindings/parser/WebIDL.py
@@ +2767,5 @@
>          return self._typeTag == IDLBuiltinType.Types.ArrayBufferView
>  
>      def isTypedArray(self):
>          return self._typeTag >= IDLBuiltinType.Types.Int8Array and \
> +            self._typeTag <= IDLBuiltinType.Types.Float64Array

I don't think we should do this.

@@ +2774,5 @@
>          # TypedArray things are interface types per the TypedArray spec,
>          # but we handle them as builtins because SpiderMonkey implements
>          # all of it internally.
>          return self.isArrayBuffer() or \
> +            self.isArrayBufferView() or \

Or this.

@@ +2788,1 @@
>                 self._typeTag == IDLBuiltinType.Types.unrestricted_float or \

Definitely not.

@@ +5861,5 @@
>      @ staticmethod
>      def handleModifiers(type, modifiers):
>          for (modifier, modifierLocation) in modifiers:
>              assert modifier == IDLMethod.TypeSuffixModifier.QMark or \
> +                modifier == IDLMethod.TypeSuffixModifier.Brackets

I don't think we should do this either.
Comment on attachment 8608414 [details] [diff] [review]
Fix flake8's E126 warnings in dom/bindings's Python code

Review of attachment 8608414 [details] [diff] [review]:
-----------------------------------------------------------------

There's a couple here that I think we shouldn't do.

::: dom/bindings/parser/WebIDL.py
@@ +2767,5 @@
>          return self._typeTag == IDLBuiltinType.Types.ArrayBufferView
>  
>      def isTypedArray(self):
>          return self._typeTag >= IDLBuiltinType.Types.Int8Array and \
> +            self._typeTag <= IDLBuiltinType.Types.Float64Array

I don't think we should do this.

@@ +2774,5 @@
>          # TypedArray things are interface types per the TypedArray spec,
>          # but we handle them as builtins because SpiderMonkey implements
>          # all of it internally.
>          return self.isArrayBuffer() or \
> +            self.isArrayBufferView() or \

Or this.

@@ +2788,1 @@
>                 self._typeTag == IDLBuiltinType.Types.unrestricted_float or \

Definitely not.

@@ +5861,5 @@
>      @ staticmethod
>      def handleModifiers(type, modifiers):
>          for (modifier, modifierLocation) in modifiers:
>              assert modifier == IDLMethod.TypeSuffixModifier.QMark or \
> +                modifier == IDLMethod.TypeSuffixModifier.Brackets

I don't think we should do this either.
Attachment #8608414 - Flags: review?(peterv) → review+
Comment on attachment 8608571 [details] [diff] [review]
Fix flake8's E128 warnings in dom/bindings's Python code

Review of attachment 8608571 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2818,5 @@
>                        return;
>                      }
>                      """,
> +                                   defineFn=defineFn,
> +                                   prop=prop))

I think you should do:

                    CGGeneric(fill(
                        """
...
                        """,
                        defineFn=defineFn,
                        prop=prop))

It's what we do in most other places.

@@ +2828,5 @@
>                          if (!JS_GetProperty(aCx, proto, \"${prop}\", &aliasedVal)) {
>                            return;
>                          }
>                          """,
> +                                   prop=m.identifier.name))

Same here.

@@ +4212,5 @@
>                                $*{exceptionCode}
>                              }
>                              """,
> +                                sourceDescription=sourceDescription,
> +                                exceptionCode=exceptionCode) + templateBody

Same here.

@@ +5205,5 @@
>                                $*{exceptionCode}
>                              }
>                              """,
> +                                sourceDescription=sourceDescription,
> +                                exceptionCode=exceptionCode) + templateBody

Same here.

@@ +10376,5 @@
>                      } else {
>                        $*{hasOnProto}
>                      }
>                      """,
> +                                        hasOnProto=computeCondition)

Same here.

@@ +10632,5 @@
>                      if (!hasOnProto) {
>                        $*{delete}
>                      }
>                      """,
> +                              delete=delete)

Can you just move the opening """ to the next line instead, lined up with the closing """ and not touch the delete argument?

@@ +10754,5 @@
>                        $*{protoLacksProperty}
>                        return true;
>                      }
>                      """,
> +                             protoLacksProperty=named)

Same here.
Attachment #8608571 - Flags: review?(peterv) → review+
Update of the previous patch with Peter's comments.
Attachment #8608414 - Attachment is obsolete: true
Attachment #8611125 - Flags: review+
Update of the previous patch with Peter's comments.
Attachment #8608393 - Attachment is obsolete: true
Attachment #8611143 - Flags: review+
Update of the previous patch with Peter's comments.
Attachment #8611125 - Attachment is obsolete: true
Attachment #8611147 - Flags: review+
Updated for the latest code changes.
Attachment #8608570 - Attachment is obsolete: true
Attachment #8611152 - Flags: review+
Update of the previous patch with Peter's comments.
Attachment #8608571 - Attachment is obsolete: true
Attachment #8611154 - Flags: review+
Update of the previous patch with Peter's comments.
Attachment #8608577 - Attachment is obsolete: true
Attachment #8608577 - Flags: review?(peterv)
Attachment #8611155 - Flags: review+
Update of the previous patch with Peter's comments.
Attachment #8608583 - Attachment is obsolete: true
Attachment #8608583 - Flags: review?(peterv)
Attachment #8611156 - Flags: review+
Updated for the latest code changes.
Attachment #8608586 - Attachment is obsolete: true
Attachment #8608586 - Flags: review?(peterv)
Attachment #8611159 - Flags: review+
Updated for the latest code changes.
Attachment #8594490 - Attachment is obsolete: true
Attachment #8611160 - Flags: review+
Thanks for your feedback Peter. I made the necessary changes and rebased the other patches.
Attachment #8611160 - Flags: review+ → review?(peterv)
Attachment #8611159 - Flags: review+ → review?(peterv)
Attachment #8611156 - Flags: review+ → review?(peterv)
Attachment #8611155 - Flags: review+ → review?(peterv)
Comment on attachment 8608592 [details] [diff] [review]
Fix flake8's E131 warnings in dom/bindings's Python code

No more E131 errors.
Attachment #8608592 - Attachment is obsolete: true
Attachment #8608592 - Flags: review?(peterv)
Note: the merge order is E111/E113 > E121 > E122 > E124 > E125 > E126 > E127 > E128 > E129 > E2xx > E3xx > E5xx > E7xx.
Comment on attachment 8611156 [details] [diff] [review]
Fix flake8's E2xx warnings in dom/bindings's Python code

Review of attachment 8611156 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +15034,5 @@
>      def defineInit(self, cgClass):
>          iface = self.descriptorProvider.interface
>          members = ""
>          while iface.identifier.name != "Event":
> +            i = 3  # Skip the boilerplate args: type, bubble,s cancelable.

While you're here, please s/bubble,s/bubbles,/

::: dom/bindings/parser/WebIDL.py
@@ +5885,1 @@
>          #    print tok

Please add a space to the second line then.
Attachment #8611156 - Flags: review?(peterv) → review+
Attachment #8611159 - Flags: review?(peterv) → review+
Attachment #8611160 - Flags: review?(peterv) → review+
Comment on attachment 8608588 [details] [diff] [review]
Fix flake8's E7xx warnings in dom/bindings's Python code

Review of attachment 8608588 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8608588 - Flags: review?(peterv) → review+
Samy, when you have updated attachment 8611156 [details] [diff] [review], I think you should request the landing of the patch (add checkin-needed in the keyword section). Otherwise, rebase will be hard.
Flags: needinfo?(samy)
Sure. I'll make the last modifications and request to merge my patches.
Flags: needinfo?(samy)
Peter, can you please review E129?
Attachment #8611156 - Attachment is obsolete: true
Attachment #8616616 - Flags: review+
Comment on attachment 8611155 [details] [diff] [review]
Fix flake8's E129 warnings in dom/bindings's Python code

Review of attachment 8611155 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +61,5 @@
> +            type.isEnum() or
> +            type.isString() or
> +            type.isAny() or
> +            type.isObject() or
> +            type.isSpiderMonkeyInterface()):

I talked to Boris about this, we both don't like this. Boris also pointed out that PEP 8 actually allows the existing style as one of the options (see https://www.python.org/dev/peps/pep-0008/#multiline-if-statements). So I'm going to r- as it seems almost all of the patch consists of this type of fix.
Attachment #8611155 - Flags: review?(peterv) → review-
Attachment #8608377 - Attachment is obsolete: true
Attachment #8645851 - Flags: review+
Attachment #8611143 - Attachment is obsolete: true
Attachment #8645852 - Flags: review+
Attachment #8594482 - Attachment is obsolete: true
Attachment #8645853 - Flags: review+
Attachment #8594483 - Attachment is obsolete: true
Attachment #8645854 - Flags: review+
Attachment #8611147 - Attachment is obsolete: true
Attachment #8645856 - Flags: review+
Attachment #8611155 - Attachment is obsolete: true
Attachment #8645857 - Flags: review+
Attachment #8611154 - Attachment is obsolete: true
Attachment #8594481 - Attachment is obsolete: true
Attachment #8616616 - Attachment is obsolete: true
Attachment #8645859 - Flags: review+
Attachment #8611159 - Attachment is obsolete: true
Attachment #8645860 - Flags: review+
Attachment #8611160 - Attachment is obsolete: true
Attachment #8645861 - Flags: review+
Attachment #8608588 - Attachment is obsolete: true
Attachment #8645862 - Flags: review+
Attachment #8611152 - Attachment is obsolete: true
Attachment #8645863 - Flags: review+
@peterv: Hi. Sorry for the delay. What do you want to do about E129 warnings? Just ignore them?
Attachment #8645852 - Attachment is obsolete: true
Attachment #8646049 - Flags: review+
Attachment #8645853 - Attachment is obsolete: true
Attachment #8646051 - Flags: review+
Attachment #8645854 - Attachment is obsolete: true
Attachment #8646053 - Flags: review+
Attachment #8645856 - Attachment is obsolete: true
Attachment #8646054 - Flags: review+
Attachment #8645863 - Attachment is obsolete: true
Attachment #8646055 - Flags: review+
Attachment #8645857 - Attachment is obsolete: true
Attachment #8646056 - Flags: review+
Attachment #8645859 - Attachment is obsolete: true
Attachment #8646057 - Flags: review+
Attachment #8645860 - Attachment is obsolete: true
Attachment #8646058 - Flags: review+
Attachment #8645861 - Attachment is obsolete: true
Attachment #8646059 - Flags: review+
Attachment #8645862 - Attachment is obsolete: true
Attachment #8646060 - Flags: review+
Attachment #8646054 - Attachment description: 1142609_E126.patch → Fix PEP 8 E126 warnings in dom/bindings's Python code
Keywords: checkin-needed
(In reply to Samy Dindane from comment #98)
> @peterv: Hi. Sorry for the delay. What do you want to do about E129
> warnings? Just ignore them?

I think so, yes.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.