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.

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?

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

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.

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.

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.

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?

We could perhaps address the backslash-continuation spacing weirdness by using parens instead of backslash-continuations...

Samy, any news on it?

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.

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

You should have a look to the hg history of the files.

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

Peter will do it.

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):

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.

E121 errors recently appeared.

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.

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.

Updated for the latest code changes.

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.

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.

Updated for the latest code changes.

Updated for the latest code changes.

Updated for the latest code changes.

Updated for the latest code changes.

Updated for the latest code changes.

E131 errors recently appeared.

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.

Comment on attachment 8594482 [details] [diff] [review] Fix flake8's E124 warnings in dom/bindings's Python code Already reviewed.

Comment on attachment 8594483 [details] [diff] [review] Fix flake8's E125 warnings in dom/bindings's Python code Already reviewed.

Comment on attachment 8594481 [details] [diff] [review] Fix flake8's E122 warnings in dom/bindings's Python code Already reviewed.

Comment on attachment 8608377 [details] [diff] [review] Fix flake8's E111/E113 warnings in dom/bindings's Python code Already reviewed.

Comment on attachment 8608570 [details] [diff] [review] Fix flake8's E127 warnings in dom/bindings's Python code Already reviewed.

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.

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.

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.

Update of the previous patch with Peter's comments.

Update of the previous patch with Peter's comments.

Update of the previous patch with Peter's comments.

Updated for the latest code changes.

Update of the previous patch with Peter's comments.

Update of the previous patch with Peter's comments.

Update of the previous patch with Peter's comments.

Updated for the latest code changes.

Updated for the latest code changes.

Thanks for your feedback Peter. I made the necessary changes and rebased the other patches.

Comment on attachment 8608592 [details] [diff] [review] Fix flake8's E131 warnings in dom/bindings's Python code No more E131 errors.

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.

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.

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.

Sure. I'll make the last modifications and request to merge my patches.

Peter, can you please review E129?

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.

@peterv: Hi. Sorry for the delay. What do you want to do about E129 warnings? Just ignore them?

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

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

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