Closed
Bug 1142609
Opened 10 years ago
Closed 9 years ago
Python code in dom/bindings should follow the flake8 convention
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: samy, Mentored)
References
Details
(Whiteboard: [good first bug][lang=Python])
Attachments
(12 files, 50 obsolete files)
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
samy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Comment 1•10 years ago
|
||
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 | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → samy
Assignee | ||
Updated•10 years ago
|
Attachment #8577225 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8577226 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8577227 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8577228 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8577230 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8577229 -
Flags: review?(peterv)
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
(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)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8577229 -
Attachment is obsolete: true
Attachment #8577229 -
Flags: review?(peterv)
Attachment #8579377 -
Flags: review?(peterv)
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8577230 -
Flags: review?(peterv) → review+
Updated•10 years ago
|
Attachment #8579377 -
Flags: review?(peterv) → review+
Comment 16•10 years ago
|
||
We could perhaps address the backslash-continuation spacing weirdness by using parens instead of backslash-continuations...
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8577227 -
Attachment is obsolete: true
Flags: needinfo?(samy)
Attachment #8594480 -
Flags: review?(peterv)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8577228 -
Attachment is obsolete: true
Attachment #8594481 -
Flags: review?(peterv)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8579377 -
Attachment is obsolete: true
Attachment #8594482 -
Flags: review?(peterv)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8577230 -
Attachment is obsolete: true
Attachment #8594483 -
Flags: review?(peterv)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8577226 -
Attachment is obsolete: true
Attachment #8594484 -
Flags: review?(peterv)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8577225 -
Attachment is obsolete: true
Attachment #8594485 -
Flags: review?(peterv)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8594486 -
Flags: review?(peterv)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8594487 -
Flags: review?(peterv)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8594488 -
Flags: review?(peterv)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8594489 -
Flags: review?(peterv)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8594490 -
Flags: review?(peterv)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8594491 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8594480 -
Attachment description: 1142609_E111_E113.patch → Fix flake8's E111/113 warnings in dom/bindings's Python code
Assignee | ||
Comment 30•10 years ago
|
||
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!
Assignee | ||
Comment 31•10 years ago
|
||
> (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/
Reporter | ||
Comment 32•9 years ago
|
||
Samy, I think you should look for an other reviewer.
Flags: needinfo?(samy)
Assignee | ||
Comment 33•9 years ago
|
||
(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)
Reporter | ||
Comment 34•9 years ago
|
||
You should have a look to the hg history of the files.
Assignee | ||
Updated•9 years ago
|
Attachment #8594480 -
Flags: review?(peterv) → review?(jcoppeard)
Assignee | ||
Comment 35•9 years ago
|
||
@jcoppeard, can you please review my patches?
Thank you.
Comment 36•9 years ago
|
||
(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)
Comment 38•9 years ago
|
||
Why are none of the r+'s carried over? Did all of the patches change substantially since I did those reviews?
Assignee | ||
Comment 39•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8594480 -
Attachment is obsolete: true
Attachment #8594480 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 40•9 years ago
|
||
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):
Assignee | ||
Comment 41•9 years ago
|
||
Please ignore the last two comments, still learning… Sorry for the noise.
Assignee | ||
Comment 42•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
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)
Assignee | ||
Comment 43•9 years ago
|
||
E121 errors recently appeared.
Attachment #8608393 -
Flags: review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8594481 -
Attachment is obsolete: true
Attachment #8594481 -
Flags: review?(peterv)
Assignee | ||
Comment 44•9 years ago
|
||
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
Assignee | ||
Comment 45•9 years ago
|
||
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. :)
Assignee | ||
Comment 46•9 years ago
|
||
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.
Assignee | ||
Comment 47•9 years ago
|
||
Updated for latest code changes.
Attachment #8594484 -
Attachment is obsolete: true
Attachment #8594484 -
Flags: review?(peterv)
Attachment #8608414 -
Flags: review?(peterv)
Assignee | ||
Comment 48•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594485 -
Attachment is obsolete: true
Attachment #8594485 -
Flags: review?(peterv)
Attachment #8608422 -
Flags: review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8594485 -
Attachment is obsolete: false
Assignee | ||
Comment 49•9 years ago
|
||
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)
Assignee | ||
Comment 50•9 years ago
|
||
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.
Assignee | ||
Comment 51•9 years ago
|
||
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)
Assignee | ||
Comment 52•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594486 -
Attachment is obsolete: true
Attachment #8594486 -
Flags: review?(peterv)
Attachment #8608571 -
Flags: review?(peterv)
Assignee | ||
Comment 53•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594487 -
Attachment is obsolete: true
Attachment #8594487 -
Flags: review?(peterv)
Attachment #8608577 -
Flags: review?(peterv)
Assignee | ||
Comment 54•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594488 -
Attachment is obsolete: true
Attachment #8594488 -
Flags: review?(peterv)
Attachment #8608583 -
Flags: review?(peterv)
Assignee | ||
Comment 55•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594489 -
Attachment is obsolete: true
Attachment #8594489 -
Flags: review?(peterv)
Attachment #8608586 -
Flags: review?(peterv)
Assignee | ||
Comment 56•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594491 -
Attachment is obsolete: true
Attachment #8594491 -
Flags: review?(peterv)
Attachment #8608588 -
Flags: review?(peterv)
Assignee | ||
Comment 57•9 years ago
|
||
E131 errors recently appeared.
Attachment #8608592 -
Flags: review?(peterv)
Assignee | ||
Comment 58•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8594481 -
Flags: review?(peterv)
Assignee | ||
Comment 59•9 years ago
|
||
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)
Assignee | ||
Comment 60•9 years ago
|
||
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)
Assignee | ||
Comment 61•9 years ago
|
||
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)
Assignee | ||
Comment 62•9 years ago
|
||
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)
Assignee | ||
Comment 63•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8608377 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8594481 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8594482 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8594483 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8608570 -
Flags: review+
Updated•9 years ago
|
Attachment #8594490 -
Flags: review?(peterv) → review+
Comment 64•9 years ago
|
||
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 65•9 years ago
|
||
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 66•9 years ago
|
||
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 67•9 years ago
|
||
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+
Assignee | ||
Comment 68•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8608414 -
Attachment is obsolete: true
Attachment #8611125 -
Flags: review+
Assignee | ||
Comment 69•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8608393 -
Attachment is obsolete: true
Attachment #8611143 -
Flags: review+
Assignee | ||
Comment 70•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8611125 -
Attachment is obsolete: true
Attachment #8611147 -
Flags: review+
Assignee | ||
Comment 71•9 years ago
|
||
Updated for the latest code changes.
Attachment #8608570 -
Attachment is obsolete: true
Attachment #8611152 -
Flags: review+
Assignee | ||
Comment 72•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8608571 -
Attachment is obsolete: true
Attachment #8611154 -
Flags: review+
Assignee | ||
Comment 73•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8608577 -
Attachment is obsolete: true
Attachment #8608577 -
Flags: review?(peterv)
Attachment #8611155 -
Flags: review+
Assignee | ||
Comment 74•9 years ago
|
||
Update of the previous patch with Peter's comments.
Attachment #8608583 -
Attachment is obsolete: true
Attachment #8608583 -
Flags: review?(peterv)
Attachment #8611156 -
Flags: review+
Assignee | ||
Comment 75•9 years ago
|
||
Updated for the latest code changes.
Attachment #8608586 -
Attachment is obsolete: true
Attachment #8608586 -
Flags: review?(peterv)
Attachment #8611159 -
Flags: review+
Assignee | ||
Comment 76•9 years ago
|
||
Updated for the latest code changes.
Attachment #8594490 -
Attachment is obsolete: true
Attachment #8611160 -
Flags: review+
Assignee | ||
Comment 77•9 years ago
|
||
Thanks for your feedback Peter. I made the necessary changes and rebased the other patches.
Assignee | ||
Updated•9 years ago
|
Attachment #8611160 -
Flags: review+ → review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8611159 -
Flags: review+ → review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8611156 -
Flags: review+ → review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8611155 -
Flags: review+ → review?(peterv)
Assignee | ||
Comment 78•9 years ago
|
||
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)
Assignee | ||
Comment 79•9 years ago
|
||
Note: the merge order is E111/E113 > E121 > E122 > E124 > E125 > E126 > E127 > E128 > E129 > E2xx > E3xx > E5xx > E7xx.
Comment 80•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8611159 -
Flags: review?(peterv) → review+
Updated•9 years ago
|
Attachment #8611160 -
Flags: review?(peterv) → review+
Comment 81•9 years ago
|
||
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+
Reporter | ||
Comment 82•9 years ago
|
||
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)
Assignee | ||
Comment 83•9 years ago
|
||
Sure. I'll make the last modifications and request to merge my patches.
Flags: needinfo?(samy)
Assignee | ||
Comment 84•9 years ago
|
||
Peter, can you please review E129?
Assignee | ||
Comment 85•9 years ago
|
||
Attachment #8611156 -
Attachment is obsolete: true
Attachment #8616616 -
Flags: review+
Comment 86•9 years ago
|
||
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-
Assignee | ||
Comment 87•9 years ago
|
||
Attachment #8608377 -
Attachment is obsolete: true
Attachment #8645851 -
Flags: review+
Assignee | ||
Comment 88•9 years ago
|
||
Attachment #8611143 -
Attachment is obsolete: true
Attachment #8645852 -
Flags: review+
Assignee | ||
Comment 89•9 years ago
|
||
Attachment #8594482 -
Attachment is obsolete: true
Attachment #8645853 -
Flags: review+
Assignee | ||
Comment 90•9 years ago
|
||
Attachment #8594483 -
Attachment is obsolete: true
Attachment #8645854 -
Flags: review+
Assignee | ||
Comment 91•9 years ago
|
||
Attachment #8611147 -
Attachment is obsolete: true
Attachment #8645856 -
Flags: review+
Assignee | ||
Comment 92•9 years ago
|
||
Attachment #8611155 -
Attachment is obsolete: true
Attachment #8645857 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8611154 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8594481 -
Attachment is obsolete: true
Assignee | ||
Comment 93•9 years ago
|
||
Attachment #8616616 -
Attachment is obsolete: true
Attachment #8645859 -
Flags: review+
Assignee | ||
Comment 94•9 years ago
|
||
Attachment #8611159 -
Attachment is obsolete: true
Attachment #8645860 -
Flags: review+
Assignee | ||
Comment 95•9 years ago
|
||
Attachment #8611160 -
Attachment is obsolete: true
Attachment #8645861 -
Flags: review+
Assignee | ||
Comment 96•9 years ago
|
||
Attachment #8608588 -
Attachment is obsolete: true
Attachment #8645862 -
Flags: review+
Assignee | ||
Comment 97•9 years ago
|
||
Attachment #8611152 -
Attachment is obsolete: true
Attachment #8645863 -
Flags: review+
Assignee | ||
Comment 98•9 years ago
|
||
@peterv: Hi. Sorry for the delay. What do you want to do about E129 warnings? Just ignore them?
Assignee | ||
Comment 99•9 years ago
|
||
Attachment #8645852 -
Attachment is obsolete: true
Attachment #8646049 -
Flags: review+
Assignee | ||
Comment 100•9 years ago
|
||
Attachment #8646050 -
Flags: review+
Assignee | ||
Comment 101•9 years ago
|
||
Attachment #8645853 -
Attachment is obsolete: true
Attachment #8646051 -
Flags: review+
Assignee | ||
Comment 102•9 years ago
|
||
Attachment #8645854 -
Attachment is obsolete: true
Attachment #8646053 -
Flags: review+
Assignee | ||
Comment 103•9 years ago
|
||
Attachment #8645856 -
Attachment is obsolete: true
Attachment #8646054 -
Flags: review+
Assignee | ||
Comment 104•9 years ago
|
||
Attachment #8645863 -
Attachment is obsolete: true
Attachment #8646055 -
Flags: review+
Assignee | ||
Comment 105•9 years ago
|
||
Attachment #8645857 -
Attachment is obsolete: true
Attachment #8646056 -
Flags: review+
Assignee | ||
Comment 106•9 years ago
|
||
Attachment #8645859 -
Attachment is obsolete: true
Attachment #8646057 -
Flags: review+
Assignee | ||
Comment 107•9 years ago
|
||
Attachment #8645860 -
Attachment is obsolete: true
Attachment #8646058 -
Flags: review+
Assignee | ||
Comment 108•9 years ago
|
||
Attachment #8645861 -
Attachment is obsolete: true
Attachment #8646059 -
Flags: review+
Assignee | ||
Comment 109•9 years ago
|
||
Attachment #8645862 -
Attachment is obsolete: true
Attachment #8646060 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8646054 -
Attachment description: 1142609_E126.patch → Fix PEP 8 E126 warnings in dom/bindings's Python code
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 110•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd660fe49a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/58d754b672e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d811fac5d33
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5f935df2c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f5802b6784
https://hg.mozilla.org/integration/mozilla-inbound/rev/d459be3c83e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/909c65a99bb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed3871c8b48
https://hg.mozilla.org/integration/mozilla-inbound/rev/df60475dada2
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c1736050cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b27f46cb532
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0847dc7dfb
Keywords: checkin-needed
Comment 111•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3bd660fe49a7
https://hg.mozilla.org/mozilla-central/rev/58d754b672e8
https://hg.mozilla.org/mozilla-central/rev/3d811fac5d33
https://hg.mozilla.org/mozilla-central/rev/5d5f935df2c3
https://hg.mozilla.org/mozilla-central/rev/b5f5802b6784
https://hg.mozilla.org/mozilla-central/rev/d459be3c83e7
https://hg.mozilla.org/mozilla-central/rev/909c65a99bb9
https://hg.mozilla.org/mozilla-central/rev/0ed3871c8b48
https://hg.mozilla.org/mozilla-central/rev/df60475dada2
https://hg.mozilla.org/mozilla-central/rev/44c1736050cb
https://hg.mozilla.org/mozilla-central/rev/2b27f46cb532
https://hg.mozilla.org/mozilla-central/rev/0c0847dc7dfb
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 112•9 years ago
|
||
(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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•