Closed Bug 1464869 Opened 7 years ago Closed 7 years ago

flake8/pep8 more code

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

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

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(17 files)

Bug 1464869 - Fix flake8/pep8 issue by hand in mozglue/
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1464869 - Run autopep8 on xpcom/
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in xpcom/
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1464869 - Run autopep8 on accessible
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in accessible/
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1464869 - Run autopep8 on memory/
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in memory/
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1464869 - Run autopep8 on js/
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in js/
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Bug 1464869 - Add accessible/, js/, mobile/, memory/, mozglue/, security/, testing/mozharness/mozharness/mozilla/testing/ and xpcom/ to the list of checked directory for flake8
59 bytes, text/x-review-board-request
Details
Bug 1464869 - Run autopep8 on security/
59 bytes, text/x-review-board-request
franziskus
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in security/
59 bytes, text/x-review-board-request
franziskus
: review+
Details
Bug 1464869 - Also ignore security/nss/
59 bytes, text/x-review-board-request
ahal
: review+
Details
bug 1464869 - autopep8 on testing/mozharness/mozharness/mozilla/testing/
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in testing/mozharness/mozharness/mozilla/testing/
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1464869 - autopep8 on mobile/
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1464869 - Fix flake8/pep8 issue by hand in mobile/
59 bytes, text/x-review-board-request
nalexander
: review+
Details
+++ This bug was initially created as a clone of Bug #1463425 +++
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review253370 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 1) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review253376 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 2) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review253384 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 3) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981227 [details] Bug 1464869 - Also ignore security/nss/ https://reviewboard.mozilla.org/r/247316/#review253526
Attachment #8981227 - Flags: review?(ahal) → review+
Comment on attachment 8981214 [details] Bug 1464869 - Add accessible/, js/, mobile/, memory/, mozglue/, security/, testing/mozharness/mozharness/mozilla/testing/ and xpcom/ to the list of checked directory for flake8 https://reviewboard.mozilla.org/r/247310/#review253534
Attachment #8981214 - Flags: review+
Comment on attachment 8981205 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in mozglue/ https://reviewboard.mozilla.org/r/247290/#review254362 Code analysis found 12 defects in this patch: - 12 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: js/src/gdb/tests/test-ExecutableAllocator.py:13 (Diff revision 5) > > run_fragment('ExecutableAllocator.onepool') > > reExecPool = 'ExecutablePool [a-f0-9]{8,}-[a-f0-9]{8,}' > assert_regexp_pretty('pool', reExecPool) > -assert_regexp_pretty('execAlloc', 'ExecutableAllocator\(\[' +reExecPool+ '\]\)') > +assert_regexp_pretty('execAlloc', 'ExecutableAllocator\(\[' + reExecPool + '\]\)') Error: Undefined name 'assert_regexp_pretty' [flake8: F821] ::: js/src/gdb/tests/test-ExecutableAllocator.py:17 (Diff revision 5) > assert_regexp_pretty('pool', reExecPool) > -assert_regexp_pretty('execAlloc', 'ExecutableAllocator\(\[' +reExecPool+ '\]\)') > +assert_regexp_pretty('execAlloc', 'ExecutableAllocator\(\[' + reExecPool + '\]\)') > > run_fragment('ExecutableAllocator.twopools') > > -assert_regexp_pretty('execAlloc', 'ExecutableAllocator\(\[' + reExecPool + ', ' + reExecPool + '\]\)') > +assert_regexp_pretty( Error: Undefined name 'assert_regexp_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Interpreter.py:13 (Diff revision 5) > > run_fragment('Interpreter.AbstractFramePtr') > > assert_pretty('ifptr', 'AbstractFramePtr ((js::InterpreterFrame *) ) = {ptr_ = 146464513}') > assert_pretty('bfptr', 'AbstractFramePtr ((js::jit::BaselineFrame *) ) = {ptr_ = 3135025122}') > -assert_pretty('rfptr', 'AbstractFramePtr ((js::jit::RematerializedFrame *) ) = {ptr_ = 3669732611}') > +assert_pretty( Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:19 (Diff revision 5) > > # This depends on implementation details of arrays, but since HeapSlot is > # not a public type, I'm not sure how to avoid doing *something* ugly. > assert_pretty('((js::NativeObject *) array.ptr)->elements_[0]', '$JS::Value("plinth")') > > -run_fragment('Root.barriers'); > +run_fragment('Root.barriers') Error: Undefined name 'run_fragment' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:21 (Diff revision 5) > # not a public type, I'm not sure how to avoid doing *something* ugly. > assert_pretty('((js::NativeObject *) array.ptr)->elements_[0]', '$JS::Value("plinth")') > > -run_fragment('Root.barriers'); > +run_fragment('Root.barriers') > > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:22 (Diff revision 5) > assert_pretty('((js::NativeObject *) array.ptr)->elements_[0]', '$JS::Value("plinth")') > > -run_fragment('Root.barriers'); > +run_fragment('Root.barriers') > > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') > +assert_pretty('heapptr', '(JSObject *) [object Object]') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:23 (Diff revision 5) > > -run_fragment('Root.barriers'); > +run_fragment('Root.barriers') > > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > -assert_pretty('relocatable', '(JSObject *) [object Object]'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') > +assert_pretty('heapptr', '(JSObject *) [object Object]') > +assert_pretty('relocatable', '(JSObject *) [object Object]') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:24 (Diff revision 5) > -run_fragment('Root.barriers'); > +run_fragment('Root.barriers') > > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > -assert_pretty('relocatable', '(JSObject *) [object Object]'); > -assert_pretty('val', '$JS::Value((JSObject *) [object Object])'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') > +assert_pretty('heapptr', '(JSObject *) [object Object]') > +assert_pretty('relocatable', '(JSObject *) [object Object]') > +assert_pretty('val', '$JS::Value((JSObject *) [object Object])') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:25 (Diff revision 5) > > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > -assert_pretty('relocatable', '(JSObject *) [object Object]'); > -assert_pretty('val', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') > +assert_pretty('heapptr', '(JSObject *) [object Object]') > +assert_pretty('relocatable', '(JSObject *) [object Object]') > +assert_pretty('val', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:26 (Diff revision 5) > -assert_pretty('prebarriered', '(JSObject *) [object Object]'); > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > -assert_pretty('relocatable', '(JSObject *) [object Object]'); > -assert_pretty('val', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('prebarrieredValue', '$JS::Value((JSObject *) [object Object])'); > +assert_pretty('prebarriered', '(JSObject *) [object Object]') > +assert_pretty('heapptr', '(JSObject *) [object Object]') > +assert_pretty('relocatable', '(JSObject *) [object Object]') > +assert_pretty('val', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('prebarrieredValue', '$JS::Value((JSObject *) [object Object])') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-Root.py:27 (Diff revision 5) > -assert_pretty('heapptr', '(JSObject *) [object Object]'); > -assert_pretty('relocatable', '(JSObject *) [object Object]'); > -assert_pretty('val', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('prebarrieredValue', '$JS::Value((JSObject *) [object Object])'); > -assert_pretty('relocatableValue', '$JS::Value((JSObject *) [object Object])'); > +assert_pretty('heapptr', '(JSObject *) [object Object]') > +assert_pretty('relocatable', '(JSObject *) [object Object]') > +assert_pretty('val', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('heapValue', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('prebarrieredValue', '$JS::Value((JSObject *) [object Object])') > +assert_pretty('relocatableValue', '$JS::Value((JSObject *) [object Object])') Error: Undefined name 'assert_pretty' [flake8: F821] ::: js/src/gdb/tests/test-prettyprinters.py:24 (Diff revision 5) > assert_eq(implemented_type_names('f'), ['F', 'C', 'D']) > assert_eq(implemented_type_names('h'), ['H', 'F', 'G', 'C', 'D']) > > # Check that our pretty-printers aren't interfering with printing other types. > assert_pretty('10', '10') > -assert_pretty('(void*) 0', '') # Because of 'set print address off' > +assert_pretty('(void*) 0', '') # Because of 'set print address off' Error: Undefined name 'assert_pretty' [flake8: F821]
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review254632 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 4) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Attachment #8981205 - Flags: review?(mh+mozilla)
Attachment #8981206 - Flags: review?(nika)
Attachment #8981207 - Flags: review?(nika)
Attachment #8981208 - Flags: review?(mh+mozilla)
Attachment #8981209 - Flags: review?(mh+mozilla)
Attachment #8981210 - Flags: review?(n.nethercote)
Attachment #8981211 - Flags: review?(n.nethercote)
Attachment #8981212 - Flags: review?(jorendorff)
Attachment #8981213 - Flags: review?(jorendorff)
Attachment #8981215 - Flags: review?(franziskuskiefer)
Attachment #8981216 - Flags: review?(franziskuskiefer)
Attachment #8982477 - Flags: review?(jmaher)
Attachment #8982478 - Flags: review?(jmaher)
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review254950 I didn't look at the actual changes yet but the NSS changes have to go into a separate patch.
Attachment #8981215 - Flags: review?(franziskuskiefer)
Comment on attachment 8981210 [details] Bug 1464869 - Run autopep8 on memory/ https://reviewboard.mozilla.org/r/247302/#review254956 r=me after confirmation on the `def` question below. ::: memory/replace/dmd/dmd.py:221 (Diff revision 2) > - fix = lambda line: fixModule.fixSymbols(line) > + > + def fix(line): return fixModule.fixSymbols(line) > elif sysname == 'Darwin': > import fix_macosx_stack as fixModule > - fix = lambda line: fixModule.fixSymbols(line) > + > + def fix(line): return fixModule.fixSymbols(line) Is this change legit? I'm hazy enough on how `def` works in Python (in comparison to assignment) to not be certain.
Attachment #8981210 - Flags: review?(n.nethercote) → review+
Comment on attachment 8981211 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in memory/ https://reviewboard.mozilla.org/r/247304/#review254958
Attachment #8981211 - Flags: review?(n.nethercote) → review+
> Is this change legit? I'm hazy enough on how `def` works in Python (in > comparison to assignment) to not be certain. This change was done by autopep8. pep8 says "Always use a def statement instead of an assignment statement that binds a lambda expression directly to an identifier." See: https://www.python.org/dev/peps/pep-0008/#programming-recommendations
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review254962 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 6) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255026 lgtm
Attachment #8981215 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8981216 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in security/ https://reviewboard.mozilla.org/r/247314/#review255030 ::: security/manager/ssl/tests/unit/pycms.py:136 (Diff revision 7) > signerInfo['issuerAndSerialNumber'] = issuerAndSerialNumber > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > - authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > + authenticatedAttributes = ( Why this?
Comment on attachment 8981210 [details] Bug 1464869 - Run autopep8 on memory/ https://reviewboard.mozilla.org/r/247302/#review254956 > Is this change legit? I'm hazy enough on how `def` works in Python (in comparison to assignment) to not be certain. Yes, this is valid python. Whether we should have this rule enabled or not is debateable.. I'd prefer if `autopep8` had moved the `return` statement to a separate line, but oh well.
Comment on attachment 8981206 [details] Bug 1464869 - Run autopep8 on xpcom/ https://reviewboard.mozilla.org/r/247294/#review255164 ::: xpcom/base/ErrorList.py:655 (Diff revision 2) > - errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_WRONG_RESPONSETYPE_FOR_RESPONSETEXT"] = FAILURE(1023) > - errors["NS_ERROR_DOM_INVALID_STATE_XHR_CHUNKED_RESPONSETYPES_UNSUPPORTED_FOR_SYNC"] = FAILURE(1024) > - errors["NS_ERROR_DOM_INVALID_ACCESS_XHR_TIMEOUT_AND_RESPONSETYPE_UNSUPPORTED_FOR_SYNC"] = FAILURE(1025) > + errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_WRONG_RESPONSETYPE_FOR_RESPONSETEXT"] = FAILURE( > + 1023) > + errors["NS_ERROR_DOM_INVALID_STATE_XHR_CHUNKED_RESPONSETYPES_UNSUPPORTED_FOR_SYNC"] = FAILURE( > + 1024) > + errors["NS_ERROR_DOM_INVALID_ACCESS_XHR_TIMEOUT_AND_RESPONSETYPE_UNSUPPORTED_FOR_SYNC"] = FAILURE( > + 1025) > This is pretty ugly, but I suppose it overflows the 80 column limit? I'm mildly surprised that this wasn't handled as: errors[""] = \ FAILURE(1023)
Attachment #8981206 - Flags: review?(nika) → review+
Comment on attachment 8981207 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in xpcom/ https://reviewboard.mozilla.org/r/247296/#review255166 ::: xpcom/base/ErrorList.py:656 (Diff revision 2) > errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_INVALID_CONTEXT"] = FAILURE(1018) > errors["NS_ERROR_DOM_INVALID_STATE_XHR_MUST_BE_OPENED"] = FAILURE(1019) > errors["NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_SENDING"] = FAILURE(1020) > errors["NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_LOADING_OR_DONE"] = FAILURE(1021) > errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_WRONG_RESPONSETYPE_FOR_RESPONSEXML"] = FAILURE(1022) > - errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_WRONG_RESPONSETYPE_FOR_RESPONSETEXT"] = FAILURE( > + errors["NS_ERROR_DOM_INVALID_STATE_XHR_HAS_WRONG_RESPONSETYPE_FOR_RESPONSETEXT"] = FAILURE(1023) # NOQA: E501 I see you agree ^_^ ::: xpcom/idl-parser/xpidl/xpidl.py:690 (Diff revision 2) > - if self.attributes.scriptable and realbase.attributes.builtinclass and not self.attributes.builtinclass: > - raise IDLError("interface '%s' is not builtinclass but derives from builtinclass '%s'" % ( > - self.name, self.base), self.location) > + if (self.attributes.scriptable and realbase.attributes.builtinclass and > + not self.attributes.builtinclass): > + raise IDLError("interface '%s' is not builtinclass but derives from " Is this what pep8 recommends? I've always been unsure how to cleanly wrap conditionals in if statements in python, 'cause this is a bit odd looking. ::: xpcom/typelib/xpt/tools/xpt.py:1161 (Diff revision 2) > > def code_gen(self, typelib, cd): > string_index = cd.add_string(self.name) > > # The static cast is needed for disambiguation. > - return "{%d, %s, XPTConstValue(static_cast<%s>(%d))}" % (string_index, > + return ("{%d, %s, XPTConstValue(static_cast<%s>(%d))}" % Some of these might be cleaner as: return "..." % \ (...) ::: xpcom/typelib/xpt/tools/xpt.py:1216 (Diff revision 2) > self._name_offset = 0 > self._namespace_offset = 0 > self.xpt_filename = None > > def __repr__(self): > - return "Interface('%s', '%s', '%s', methods=%s)" % (self.name, self.iid, self.namespace, self.methods) > + return ("Interface('%s', '%s', '%s', methods=%s)" % I think using \ to escape the newline is cleaner than the extra brackets.
Attachment #8981207 - Flags: review?(nika) → review+
Comment on attachment 8982477 [details] bug 1464869 - autopep8 on testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248444/#review255218 a lot of great changes here, but many changes that don't seem to make the code more readable or maintainable. ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:434 (Diff revision 2) > del os.environ['GCOV_PREFIX'] > del os.environ['JS_CODE_COVERAGE_OUTPUT_DIR'] > > if not self.ccov_upload_disabled: > - grcov_output_file, jsvm_output_file = self.parse_coverage_artifacts(self.gcov_dir, self.jsvm_dir) > + grcov_output_file, jsvm_output_file = self.parse_coverage_artifacts( > + self.gcov_dir, self.jsvm_dir) this makes it harder to read ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:102 (Diff revision 2) > self.gecko_profile = self.config.get('gecko_profile') > self.gecko_profile_interval = self.config.get('gecko_profile_interval') > - self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > - self.mitmproxy_pageset = None # zip file found on tooltool that contains all of the mitmproxy recordings > - self.mitmproxy_recordings_file_list = self.config.get('mitmproxy', None) # files inside the recording set > - self.mitmdump = None # path to mitmdump tool itself, in py3 venv > + self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > + self.mitmproxy_pageset = None # zip file found on tooltool that contains all of the mitmproxy recordings > + self.mitmproxy_recordings_file_list = self.config.get( > + 'mitmproxy', None) # files inside the recording set could we just move the comment to above the line instead of splitting the line? this is not the most pleasant change to read. ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:124 (Diff revision 2) > def query_abs_dirs(self): > if self.abs_dirs: > return self.abs_dirs > abs_dirs = super(Raptor, self).query_abs_dirs() > - abs_dirs['abs_blob_upload_dir'] = os.path.join(abs_dirs['abs_work_dir'], 'blobber_upload_dir') > + abs_dirs['abs_blob_upload_dir'] = os.path.join( > + abs_dirs['abs_work_dir'], 'blobber_upload_dir') I don't like this change either ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:135 (Diff revision 2) > """return options to raptor""" > # binary path > binary_path = self.binary_path or self.config.get('binary_path') > if not binary_path: > - self.fatal("Raptor requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") > + self.fatal( > + "Raptor requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") the quote is already too long, can we not just leave it inside the self.fatal() call? ::: testing/mozharness/mozharness/mozilla/testing/talos.py:198 (Diff revision 2) > self.benchmark_zip = None > - self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > - self.mitmproxy_recording_set = None # zip file found on tooltool that contains all of the mitmproxy recordings > - self.mitmproxy_recordings_file_list = self.config.get('mitmproxy', None) # files inside the recording set > - self.mitmdump = None # path to mitdump tool itself, in py3 venv > + self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > + # zip file found on tooltool that contains all of the mitmproxy recordings > + self.mitmproxy_recording_set = None > + self.mitmproxy_recordings_file_list = self.config.get( > + 'mitmproxy', None) # files inside the recording set just like in the previous file I am not eager to see this line split; I am fine moving the comment to the previous line ::: testing/mozharness/mozharness/mozilla/testing/talos.py:220 (Diff revision 2) > def query_abs_dirs(self): > if self.abs_dirs: > return self.abs_dirs > abs_dirs = super(Talos, self).query_abs_dirs() > - abs_dirs['abs_blob_upload_dir'] = os.path.join(abs_dirs['abs_work_dir'], 'blobber_upload_dir') > + abs_dirs['abs_blob_upload_dir'] = os.path.join( > + abs_dirs['abs_work_dir'], 'blobber_upload_dir') same here, I don't like splitting this join() call ::: testing/mozharness/mozharness/mozilla/testing/talos.py:303 (Diff revision 2) > """return options to talos""" > # binary path > binary_path = self.binary_path or self.config.get('binary_path') > if not binary_path: > - self.fatal("Talos requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") > + self.fatal( > + "Talos requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") same here, this is much easier to read inside the fatal() call ::: testing/mozharness/mozharness/mozilla/testing/talos.py:381 (Diff revision 2) > > - > tooltool_artifacts = [] > if self.query_pagesets_name(): > - tooltool_artifacts.append({'name': self.pagesets_name, 'manifest': self.pagesets_name_manifest}) > + tooltool_artifacts.append( > + {'name': self.pagesets_name, 'manifest': self.pagesets_name_manifest}) I really don't like this ::: testing/mozharness/mozharness/mozilla/testing/talos.py:385 (Diff revision 2) > - tooltool_artifacts.append({'name': self.pagesets_name, 'manifest': self.pagesets_name_manifest}) > + tooltool_artifacts.append( > + {'name': self.pagesets_name, 'manifest': self.pagesets_name_manifest}) > > if self.query_benchmark_zip(): > - tooltool_artifacts.append({'name': self.benchmark_zip, 'manifest': self.benchmark_zip_manifest}) > + tooltool_artifacts.append( > + {'name': self.benchmark_zip, 'manifest': self.benchmark_zip_manifest}) and this ::: testing/mozharness/mozharness/mozilla/testing/talos.py:510 (Diff revision 2) > if self.mitmproxy_rel_bin: > return self.mitmproxy_rel_bin > if self.query_talos_json_config() and self.suite is not None: > config_key = "mitmproxy_release_bin_" + platform > - self.mitmproxy_rel_bin = self.talos_json_config['suites'][self.suite].get(config_key, False) > + self.mitmproxy_rel_bin = self.talos_json_config['suites'][self.suite].get( > + config_key, False) I don't like this ::: testing/mozharness/mozharness/mozilla/testing/talos.py:538 (Diff revision 2) > """Mitmproxy requires external playback archives to be downloaded and extracted""" > if self.mitmproxy_recording_set: > return self.mitmproxy_recording_set > if self.query_talos_json_config() and self.suite is not None: > - self.mitmproxy_recording_set = self.talos_json_config['suites'][self.suite].get('mitmproxy_recording_set', False) > + self.mitmproxy_recording_set = self.talos_json_config['suites'][self.suite].get( > + 'mitmproxy_recording_set', False) I don't like this. ::: testing/mozharness/mozharness/mozilla/testing/testbase.py:228 (Diff revision 2) > if c.get("installer_url") is None: > self.exception("You must use --installer-url with developer_config.py") > if c.get("require_test_zip"): > if not c.get('test_url') and not c.get('test_packages_url'): > - self.exception("You must use --test-url or --test-packages-url with developer_config.py") > + self.exception( > + "You must use --test-url or --test-packages-url with developer_config.py") I really don't like this change. ::: testing/mozharness/mozharness/mozilla/testing/testbase.py:629 (Diff revision 2) > if self.platform_name() not in ('win32', 'win64'): > self.chmod(abs_nodejs_path, 0755) > self.nodejs_path = abs_nodejs_path > else: > - self.warning("nodejs path was given but couldn't be found. Tried looking in '%s'" % abs_nodejs_path) > + self.warning( > + "nodejs path was given but couldn't be found. Tried looking in '%s'" % abs_nodejs_path) I don't like this change.
Attachment #8982477 - Flags: review?(jmaher) → review-
Assignee: nobody → sledru
Comment on attachment 8982478 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248446/#review255220 a lot of great cleanup- a few smaller issues ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:247 (Diff revision 2) > return > > self.gcov_dir, self.jsvm_dir = self.set_coverage_env(os.environ) > > - def parse_coverage_artifacts(self, gcov_dir, jsvm_dir, merge=False, output_format='lcov', filter_covered=False): > + def parse_coverage_artifacts(self, gcov_dir, jsvm_dir, merge=False, output_format='lcov', > + filter_covered=False): can we just split more of the params onto the second line? maybe even one param/line? ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:101 (Diff revision 2) > self.tests = None > self.gecko_profile = self.config.get('gecko_profile') > self.gecko_profile_interval = self.config.get('gecko_profile_interval') > self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > - self.mitmproxy_pageset = None # zip file found on tooltool that contains all of the mitmproxy recordings > + self.mitmproxy_pageset = None # zip file found on tooltool that contains all of the > + # mitmproxy recordings I don't like splitting the comment this way. It is an end of line comment and now onto a second line. Can we move this comment above the line and split it if needed? ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:138 (Diff revision 2) > # binary path > binary_path = self.binary_path or self.config.get('binary_path') > if not binary_path: > self.fatal( > - "Raptor requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") > + "Raptor requires a path to the binary. You can specify binary_path or add " > + "download-and-extract to your action list.") I would prefer we have a: msg = "..." self.fatal(msg) where msg could end up being multiline ::: testing/mozharness/mozharness/mozilla/testing/talos.py:52 (Diff revision 2) > {'substr': r'''talosError''', 'level': CRITICAL}, > {'regex': re.compile(r'''No machine_name called '.*' can be found'''), 'level': CRITICAL}, > {'substr': r"""No such file or directory: 'browser_output.txt'""", > 'level': CRITICAL, > - 'explanation': r"""Most likely the browser failed to launch, or the test was otherwise unsuccessful in even starting."""}, > + 'explanation': "Most likely the browser failed to launch, or the test was otherwise " > + "unsuccessful in even starting."}, if we are going to change this from r'''...''', we should update the rest of the strings in this file to match the format. ::: testing/mozharness/mozharness/mozilla/testing/talos.py:308 (Diff revision 2) > # binary path > binary_path = self.binary_path or self.config.get('binary_path') > if not binary_path: > self.fatal( > - "Talos requires a path to the binary. You can specify binary_path or add download-and-extract to your action list.") > + "Talos requires a path to the binary. You can specify binary_path or add " > + "download-and-extract to your action list.") I would prefer to use: msg = "..." self.fatal(msg) ::: testing/mozharness/mozharness/mozilla/testing/testbase.py:631 (Diff revision 2) > self.chmod(abs_nodejs_path, 0755) > self.nodejs_path = abs_nodejs_path > else: > self.warning( > - "nodejs path was given but couldn't be found. Tried looking in '%s'" % abs_nodejs_path) > + "nodejs path was given but couldn't be found. Tried looking in '%s'" % > + abs_nodejs_path) I would rather see: msg = "..." self.warning(msg % abs_nodejs_path)
Attachment #8982478 - Flags: review?(jmaher) → review-
Comment on attachment 8982477 [details] bug 1464869 - autopep8 on testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248444/#review255218 > I don't like this change. To be clear, these changes were all automatic.
Comment on attachment 8982478 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248446/#review255220 > if we are going to change this from r'''...''', we should update the rest of the strings in this file to match the format. explanation is the only occurence and this isn't a regular expression, this is why I removed it here. Others are!
Comment on attachment 8981207 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in xpcom/ https://reviewboard.mozilla.org/r/247296/#review255166 > I see you agree ^_^ \o/ > Is this what pep8 recommends? I've always been unsure how to cleanly wrap conditionals in if statements in python, 'cause this is a bit odd looking. https://www.python.org/dev/peps/pep-0008/#maximum-line-length "The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses." I will drop your comment (hope you don't mind) > I think using \ to escape the newline is cleaner than the extra brackets. same as above (pep8)
Comment on attachment 8981216 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in security/ https://reviewboard.mozilla.org/r/247314/#review255030 > Why this? Because the line below goes way above 99 char per line. Adding the "(" allows the next line to be placed below the "="
Comment on attachment 8983216 [details] Bug 1464869 - autopep8 on mobile/ https://reviewboard.mozilla.org/r/249070/#review255246 I find a lot of these to look really bad to my eye, but I'm not going to stand in the way of progress. It's worth noting that `mobile/android/docs/conf.py` is generated code culted from elsewhere in the tree. (Perhaps ahal knows if it can go away; I will NI him.) The two `mobile/**/filter.py` files came from compare-locales originally. I'm not sure if they are still mirrored in some way. I will ask Pike.
Attachment #8983216 - Flags: review?(nalexander) → review+
Comment on attachment 8983217 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in mobile/ https://reviewboard.mozilla.org/r/249072/#review255248
Attachment #8983217 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #92) > Comment on attachment 8983216 [details] > Bug 1464869 - autopep8 on mobile/ > > https://reviewboard.mozilla.org/r/249070/#review255246 > > I find a lot of these to look really bad to my eye, but I'm not going to > stand in the way of progress. > > It's worth noting that `mobile/android/docs/conf.py` is generated code > culted from elsewhere in the tree. (Perhaps ahal knows if it can go away; I > will NI him.) ahal: this file came in the original landing of https://bugzilla.mozilla.org/show_bug.cgi?id=1230786. I believe you've done recent work on all of these documentation flows; is this still needed? > The two `mobile/**/filter.py` files came from compare-locales originally. > I'm not sure if they are still mirrored in some way. I will ask Pike. Pike: are the filter.py files in the tree authoritative, or are they mirrors from an upstream repository?
Flags: needinfo?(l10n)
Flags: needinfo?(ahal)
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255256 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 7) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981205 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in mozglue/ https://reviewboard.mozilla.org/r/247292/#review255266
Attachment #8981205 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8981208 [details] Bug 1464869 - Run autopep8 on accessible https://reviewboard.mozilla.org/r/247298/#review255268
Attachment #8981208 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8981209 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in accessible/ https://reviewboard.mozilla.org/r/247300/#review255270
Attachment #8981209 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8983216 [details] Bug 1464869 - autopep8 on mobile/ https://reviewboard.mozilla.org/r/249070/#review255278 Code analysis found 10 defects in this patch: - 10 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: mobile/android/mach_commands.py:80 (Diff revision 1) > > bindings_inputs = list(itertools.chain(*((input, stem(input)) for input in inputs))) > bindings_args = '-Pgenerate_sdk_bindings_args={}'.format(':'.join(bindings_inputs)) > > - ret = self.gradle(self.substs['GRADLE_ANDROID_GENERATE_SDK_BINDINGS_TASKS'] + [bindings_args] + args, verbose=True) > + ret = self.gradle( > + self.substs['GRADLE_ANDROID_GENERATE_SDK_BINDINGS_TASKS'] + [bindings_args] + args, verbose=True) Error: Line too long (109 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:89 (Diff revision 1) > @SubCommand('android', 'generate-generated-jni-wrappers', > - """Generate GeckoView JNI wrappers used when building GeckoView.""") > + """Generate GeckoView JNI wrappers used when building GeckoView.""") > @CommandArgument('args', nargs=argparse.REMAINDER) > def android_generate_generated_jni_wrappers(self, args): > - ret = self.gradle(self.substs['GRADLE_ANDROID_GENERATE_GENERATED_JNI_WRAPPERS_TASKS'] + args, verbose=True) > + ret = self.gradle( > + self.substs['GRADLE_ANDROID_GENERATE_GENERATED_JNI_WRAPPERS_TASKS'] + args, verbose=True) Error: Line too long (101 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:94 (Diff revision 1) > > return ret > > - > @SubCommand('android', 'generate-fennec-jni-wrappers', > - """Generate Fennec-specific JNI wrappers used when building Firefox for Android.""") > + """Generate Fennec-specific JNI wrappers used when building Firefox for Android.""") Error: Line too long (100 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:156 (Diff revision 1) > > # And make the report display as soon as possible. > failed = root.findall('testcase/error') or root.findall('testcase/failure') > if failed: > - print('TEST-UNEXPECTED-FAIL | android-test | There were failing tests. See the reports at: {}/{}/index.html'.format(root_url, report)) > + print( > + 'TEST-UNEXPECTED-FAIL | android-test | There were failing tests. See the reports at: {}/{}/index.html'.format(root_url, report)) Error: Line too long (152 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:215 (Diff revision 1) > objdir='gradle/build/mobile/android/app/reports') > > reports = (self.substs['GRADLE_ANDROID_APP_VARIANT_NAME'],) > for report in reports: > - f = open(os.path.join(self.topobjdir, 'gradle/build/mobile/android/app/reports/lint-results-{}.xml'.format(report)), 'rt') > + f = open(os.path.join( > + self.topobjdir, 'gradle/build/mobile/android/app/reports/lint-results-{}.xml'.format(report)), 'rt') Error: Line too long (116 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:255 (Diff revision 1) > # XML report(s) to report errors and link to the HTML > # report(s) for human consumption. > import xml.etree.ElementTree as ET > > - f = open(os.path.join(self.topobjdir, 'gradle/build/mobile/android/app/reports/checkstyle/checkstyle.xml'), 'rt') > + f = open(os.path.join(self.topobjdir, > + 'gradle/build/mobile/android/app/reports/checkstyle/checkstyle.xml'), 'rt') Error: Line too long (105 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:316 (Diff revision 1) > objdir='gradle/build/mobile/android/app/reports/findbugs') > > reports = (self.substs['GRADLE_ANDROID_APP_VARIANT_NAME'],) > for report in reports: > try: > - f = open(os.path.join(self.topobjdir, 'gradle/build/mobile/android/app/reports/findbugs', 'findbugs-{}-output.xml'.format(report)), 'rt') > + f = open(os.path.join(self.topobjdir, 'gradle/build/mobile/android/app/reports/findbugs', Error: Line too long (105 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:340 (Diff revision 1) > for error in root.findall('./BugInstance'): > # There's no particular advantage to formatting the > # error, so for now let's just output the <error> XML > # tag. > - print('TEST-UNEXPECTED-FAIL | {}:{} | {}'.format(report, error.get('type'), error.find('Class').get('classname'))) > + print('TEST-UNEXPECTED-FAIL | {}:{} | {}'.format(report, > + error.get('type'), error.find('Class').get('classname'))) Error: Line too long (122 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:367 (Diff revision 1) > - """Create GeckoView archives. > + """Create GeckoView archives. > See http://firefox-source-docs.mozilla.org/build/buildsystem/toolchains.html#firefox-for-android-with-gradle""") > @CommandArgument('args', nargs=argparse.REMAINDER) > def android_archive_geckoview(self, args): > - ret = self.gradle(self.substs['GRADLE_ANDROID_ARCHIVE_GECKOVIEW_TASKS'] + ["--continue"] + args, verbose=True) > + ret = self.gradle( > + self.substs['GRADLE_ANDROID_ARCHIVE_GECKOVIEW_TASKS'] + ["--continue"] + args, verbose=True) Error: Line too long (104 > 99 characters) [flake8: E501] ::: mobile/android/mach_commands.py:527 (Diff revision 1) > """ > @Command('android-emulator', category='devenv', > - conditions=[], > + conditions=[], > - description='Run the Android emulator with an AVD from test automation.') > + description='Run the Android emulator with an AVD from test automation.') > @CommandArgument('--version', metavar='VERSION', choices=['4.3', '6.0', '7.0', 'x86', 'x86-6.0', 'x86-7.0'], > - help='Specify Android version to run in emulator. One of "4.3", "6.0", "7.0", "x86", "x86-6.0", or "x86-7.0".', > + help='Specify Android version to run in emulator. One of "4.3", "6.0", "7.0", "x86", "x86-6.0", or "x86-7.0".', Error: Line too long (132 > 99 characters) [flake8: E501]
Comment on attachment 8981216 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in security/ https://reviewboard.mozilla.org/r/247314/#review255366
Attachment #8981216 - Flags: review?(franziskuskiefer) → review+
(In reply to Nick Alexander :nalexander from comment #92) > > The two `mobile/**/filter.py` files came from compare-locales originally. > I'm not sure if they are still mirrored in some way. I will ask Pike. The filter.py files are in-tree configuration data for compare-locales. So they're actually m-c code and the changes look OK to me (just indention on those two).
Flags: needinfo?(l10n)
Comment on attachment 8982477 [details] bug 1464869 - autopep8 on testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248444/#review255388 my original complaints were not addressed. I understand a tool did this, but we do not need to enable every rule, or we can restructure the code to meet the requirements of rules. ::: testing/mozharness/mozharness/mozilla/testing/try_tools.py (Diff revisions 2 - 3) > - url = '{}json-pushes?changeset={}&full=1'.format(repo_url, rev) > - > - pushinfo = self.load_json_from_url(url) > - for k, v in pushinfo.items(): > - if isinstance(v, dict) and 'changesets' in v: > - msg = v['changesets'][-1]['desc'] why is this block removed?
Attachment #8982477 - Flags: review?(jmaher) → review-
Comment on attachment 8982478 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248446/#review255392 one small nit, otherwise thanks for adjusting everything :) ::: testing/mozharness/mozharness/mozilla/testing/talos.py:200 (Diff revisions 2 - 3) > self.benchmark_zip = None > self.mitmproxy_rel_bin = None # some platforms download a mitmproxy release binary > # zip file found on tooltool that contains all of the mitmproxy recordings > self.mitmproxy_recording_set = None > - self.mitmproxy_recordings_file_list = self.config.get( > - 'mitmproxy', None) # files inside the recording set > + self.mitmproxy_recordings_file_list = self.config.get('mitmproxy', None) # files inside > + # the recording set can we put this comment above the line of code- it is a bit confusing reading this split comment.
Attachment #8982478 - Flags: review?(jmaher) → review+
> my original complaints were not addressed. I did address them but in the "by hand" patch. I prefer to keep the autopep8 self contained. > why is this block removed? Typo, I will fix that
ok, let me look over the two patches- I didn't realize the difference there.
(In reply to Nick Alexander :nalexander from comment #94) > > It's worth noting that `mobile/android/docs/conf.py` is generated code > > culted from elsewhere in the tree. (Perhaps ahal knows if it can go away; I > > will NI him.) > > ahal: this file came in the original landing of > https://bugzilla.mozilla.org/show_bug.cgi?id=1230786. I believe you've > done recent work on all of these documentation flows; is this still needed? Assuming you never need to build these docs outside of the context of mozilla-central and the build system, yes these conf.py files can be removed.
Flags: needinfo?(ahal)
Comment on attachment 8982477 [details] bug 1464869 - autopep8 on testing/mozharness/mozharness/mozilla/testing/ https://reviewboard.mozilla.org/r/248444/#review255496 in looking at this combined with the by hand changes, all is well.
Attachment #8982477 - Flags: review- → review+
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255598 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 8) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981212 [details] Bug 1464869 - Run autopep8 on js/ https://reviewboard.mozilla.org/r/247306/#review255600 For the most part, this is great! I spot-checked the patch and found one place where the meaning is changed (see the next-to-last comment below), but even that seems likely to be a merge error (or something like that) rather than a problem with the tool itself. However, the tool enforces PEP 8's rule against long lines (it imposes a 79 character limit) by breaking lines in some weird places. I left a few comments about this but quickly gave up; I did not tag every weird line break I saw. On average it seems like about half of them are weird. If possible, please re-run the tool with line-breaking disabled. If not, I guess we'll live with it. In a few places, the tool inserts blank lines between a comment and a class or function (see comment below). Again, please fix it if possible. ::: js/src/builtin/intl/make_intl_data.py:904 (Diff revision 4) > - inZoneInfo64 = lambda zone: zone in zoneinfoZones or zone in zoneinfoLinks > + def inZoneInfo64(zone): return zone in zoneinfoZones or zone in zoneinfoLinks > > # Remove legacy ICU time zones from zoneinfo64 data. > (legacyZones, legacyLinks) = readICULegacyZones(icuDir) > zoneinfoZones = {zone for zone in zoneinfoZones if zone not in legacyZones} > - zoneinfoLinks = {zone: target for (zone, target) in zoneinfoLinks.items() if zone not in legacyLinks} > + zoneinfoLinks = {zone: target for ( Here's an example of what seems like a really weird place to break a line. ::: js/src/builtin/intl/make_intl_data.py:1025 (Diff revision 4) > raise RuntimeError("Additional links present in ICU, did you forget " > "to run intl/update-tzdata.sh? %s" % additionalTimeZones) > > result = chain( > # IANA links which have a different target in ICU. > - ((zone, target, icuLinks[zone]) for (zone, target) in ianaLinks.items() if isICULink(zone) and target != icuLinks[zone]), > + ((zone, target, icuLinks[zone]) for (zone, target) another ::: js/src/devtools/automation/autospider.py:403 (Diff revision 4) > shutil.copyfile(configure + ".in", configure) > os.chmod(configure, 0755) > > # Run configure > if not args.noconf: > - run_command(['sh', '-c', posixpath.join(PDIR.js_src, 'configure') + ' ' + CONFIGURE_ARGS], check=True) > + run_command(['sh', '-c', posixpath.join(PDIR.js_src, 'configure') + another ::: js/src/devtools/rootAnalysis/analyze.py:32 (Diff revision 4) > exec(compile(open(thefile).read(), filename=thefile, mode="exec"), globals) > > + > def env(config): > e = dict(os.environ) > - e['PATH'] = ':'.join(p for p in (config.get('gcc_bin'), config.get('sixgill_bin'), e['PATH']) if p) > + e['PATH'] = ':'.join(p for p in (config.get('gcc_bin'), extra bad one ::: js/src/devtools/rootAnalysis/explain.py:60 (Diff revision 4) > if m: > # Function names are surrounded by single quotes. Field calls > # are unquoted. > current_gcFunction = m.group(2) > hazardousGCFunctions[current_gcFunction].append(line) > - hazardOrder.append((current_gcFunction, len(hazardousGCFunctions[current_gcFunction]) - 1)) > + hazardOrder.append((current_gcFunction, len( another ::: js/src/gdb/mozilla/JSString.py:15 (Diff revision 4) > - chr = unichr # replace with teh unicodes > + chr = unichr # replace with teh unicodes > > # Forget any printers from previous loads of this module. > mozilla.prettyprinters.clear_module_printers(__name__) > > # Cache information about the JSString type for this objfile. Unfortunate separation of the comment from the class it's meant to comment. Of course it ought to be a docstring... maybe *before* running the tool, you'd be willing to fix these up somehow...? I know that's a lot to ask. There are 97 of them, most but not all in js/src/gdb, according to this: ``` cd js/src find . -name 'build_*' -prune -o -name '*.py' -print | xargs perl -ne 'print "$ARGV:$prevline:$prev$ARGV:$.:$_" if /^\s*(class|def)\s/ and $prev =~ /^\s*#/; $prev=$_; $prevline=$.;' ``` ::: js/src/gdb/mozilla/Root.py:10 (Diff revision 4) > from mozilla.prettyprinters import pretty_printer, template_pretty_printer > > # Forget any printers from previous loads of this module. > mozilla.prettyprinters.clear_module_printers(__name__) > > # Common base class for all the rooting template pretty-printers. All these another ::: js/src/gdb/mozilla/Root.py:83 (Diff revision 4) > + > @template_pretty_printer("js::BarrieredBase") > class BarrieredBase(Common): > member = 'value' > > # Return the referent of a HeapPtr, Rooted, or Handle. another ::: js/src/tests/lib/jittests.py:121 (Diff revision 4) > > self.jitflags = [] # jit flags to enable > self.slow = False # True means the test is slow-running > - self.allow_oom = False # True means that OOM is not considered a failure > - self.allow_unhandlable_oom = False # True means CrashAtUnhandlableOOM > + self.allow_oom = False # True means that OOM is not considered a failure > + self.allow_unhandlable_oom = False # True means CrashAtUnhandlableOOM > - # is not considered a failure > + # is not considered a failure The dedents in this hunk seem bad. Fix manually by moving each comment to the preceding line? ::: js/src/tests/lib/progressbar.py:35 (Diff revision 4) > assert self.conservative_isatty() > > self.prior = None > self.atLineStart = True > - self.counters_fmt = fmt # [{str:str}] Describtion of how to lay out each > + self.counters_fmt = fmt # [{str:str}] Describtion of how to lay out each > - # field in the counters map. > + # field in the counters map. Here's another one. ::: js/src/tests/lib/tests.py:159 (Diff revision 4) > def __init__(self, path): > self.path = path # str: path of JS file relative to tests root dir > self.options = [] # [str]: Extra options to pass to the shell > self.jitflags = [] # [str]: JIT flags to pass to the shell > self.test_reflect_stringify = None # str or None: path to > - # reflect-stringify.js file to test > + # reflect-stringify.js file to test and another ::: js/src/tests/test262-update.py:30 (Diff revision 4) > - "regexp-dotall", > + "regexp-dotall", > - "regexp-lookbehind", > + "regexp-lookbehind", > - "regexp-named-groups", > + "regexp-named-groups", > - "regexp-unicode-property-escapes", > + "regexp-unicode-property-escapes", > - "numeric-separator-literal", > + "numeric-separator-literal", > + "json-superset", That's weird. This string seems to have been added? Perhaps some version control oddity? ::: js/src/tests/test262-update.py:644 (Diff revision 4) > parser.add_argument("--revision", default="HEAD", > help="Git revision (default: %(default)s)") > parser.add_argument("--out", default="test262", > help="Output directory. Any existing directory will be removed! (default: %(default)s)") > - parser.add_argument("--pull", help="Import contents from a Pull Request specified by its number") > - parser.add_argument("--local", help="Import new and modified contents from a local folder, a new folder will be created on local/branch_name") > + parser.add_argument( > + "--pull", help="Import contents from a Pull Request specified by its number") Better to manually break these two lines to be like the others nearby, I think.
Attachment #8981212 - Flags: review?(jorendorff) → review+
Thanks for the reviews! Jason, I fixed the issue that you mentioned in https://reviewboard.mozilla.org/r/247308/diff/6/ Last one before landing!
Flags: needinfo?(jorendorff)
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255622 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 9) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981213 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in js/ https://reviewboard.mozilla.org/r/247308/#review255610 This is less exciting. I hope I caught everything... ::: js/src/builtin/intl/make_intl_data.py:1454 (Diff revision 6) > help="Update language-subtag-registry") > parser_tags.add_argument("--url", > metavar="URL", > default="https://www.iana.org/assignments/language-subtag-registry", > type=EnsureHttps, > - help="Download url for language-subtag-registry.txt (default: %(default)s)") > + help="Download url for language-subtag-registry.txt" This line break seems to have removed a space from the help string. Please put it back. ::: js/src/devtools/rootAnalysis/analyze.py:245 (Diff revision 6) > help='command to build the tree being analyzed') > parser.add_argument('--tag', '-t', type=str, nargs='?', > help='name of job, also sets build command to "build.<tag>"') > parser.add_argument('--expect-file', type=str, nargs='?', > - help='deprecated option, temporarily still present for backwards compatibility') > + help='deprecated option, temporarily still present for backwards' > + 'compatibility') Same here. ::: js/src/gdb/mozilla/autoload.py (Diff revision 6) > import mozilla.Root > import mozilla.jsid > import mozilla.jsval > import mozilla.unwind > > -# The user may have personal pretty-printers. Get those, too, if they exist. If there's a good reason to delete this, please file a separate bug and r?tromey. In any case please put it back for now. ::: js/src/gdb/run-tests.py:178 (Diff revision 6) > '--ex', 'add-auto-load-safe-path %s' % (OPTIONS.bindir,), > '--ex', 'set env LD_LIBRARY_PATH %s' % (OPTIONS.bindir,), > '--ex', 'file %s' % (os.path.join(OPTIONS.bindir, 'gdb-tests'),), > '--eval-command', 'python testlibdir=%r' % (testlibdir,), > '--eval-command', 'python testscript=%r' % (self.test_path,), > - '--eval-command', 'python exec(open(%r).read())' % os.path.join(testlibdir, 'catcher.py')] > + '--eval-command', 'python exec(open(%r).read())' % Please break after the comma instead. (If the line is still too long, breaking this return statement after `return [` and breaking before the final `]` will save a few more characters.) ::: js/src/gdb/taskpool.py:91 (Diff revision 6) > flags = fcntl.fcntl(p.stdout, fcntl.F_GETFL) > fcntl.fcntl(p.stdout, fcntl.F_SETFL, flags | os.O_NONBLOCK) > flags = fcntl.fcntl(p.stderr, fcntl.F_GETFL) > fcntl.fcntl(p.stderr, fcntl.F_SETFL, flags | os.O_NONBLOCK) > > - t.start(p, time.time() + self.timeout) > + task.start(p, time.time() + self.timeout) Please also fix the use of `t` on the next line. ::: js/src/gdb/tests/test-unwind.py (Diff revision 6) > > # Only on the right platforms. > if platform.machine() == 'x86_64' and platform.system() == 'Linux': > # Only test when gdb has the unwinder feature. > try: > - import gdb.unwinder This change is wrong (see comment). Please put the imports back and silence the warning. ::: js/src/jit-test/jit_test.py:275 (Diff revision 6) > options.exclude += [os.path.join('debug', 'Script-getOffsetsCoverage-02.js')] > > if options.exclude_from: > with open(options.exclude_from) as fh: > for line in fh: > - line = line.strip() > + line_ = line.strip() Hmm. Is there really a style rule that requires this? It seems much worse to me; can we please not do this? ::: js/src/tests/lib/results.py:100 (Diff revision 6) > expected_rcs.append(3) > if test.error not in err: > failures += 1 > results.append((cls.FAIL, "Expected uncaught error: {}".format(test.error))) > > - if rc and not rc in expected_rcs: > + if rc and rc not in expected_rcs: This is likely a bug fix, but a nontrivial change to the meaning of the program belongs in a separate patch (and don't be surprised if it burns the tree--who knows how long this has been broken or who's depending on it). ::: js/src/tests/test262-export.py:432 (Diff revision 6) > parser.add_argument("--out", default="test262/export", > - help="Output directory. Any existing directory will be removed! (default: %(default)s)") > + help="Output directory. Any existing directory will be removed! " > + "(default: %(default)s)") > parser.add_argument("--exportshellincludes", action="store_true", > - help="Optionally export shell.js files as includes in exported tests. Only use for testing, do not use for exporting to test262 (test262 tests should have as few dependencies as possible).") > + help="Optionally export shell.js files as includes in exported tests." > + "Only use for testing, do not use for exporting to test262 (test262 tests " Another space was deleted here.
Attachment #8981213 - Flags: review?(jorendorff) → review+
That was fast! Thanks.
Flags: needinfo?(jorendorff)
Comment on attachment 8981213 [details] Bug 1464869 - Fix flake8/pep8 issue by hand in js/ https://reviewboard.mozilla.org/r/247308/#review255610 > Hmm. Is there really a style rule that requires this? It seems much worse to me; can we please not do this? line is already defined above, just renaming to avoid name conflict > This is likely a bug fix, but a nontrivial change to the meaning of the program belongs in a separate patch (and don't be surprised if it burns the tree--who knows how long this has been broken or who's depending on it). it isn't a bug fix. Python recommends foo not in structure instead of not foo in structure
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255780 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 10) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Comment on attachment 8981215 [details] Bug 1464869 - Run autopep8 on security/ https://reviewboard.mozilla.org/r/247312/#review255782 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: security/manager/ssl/tests/unit/pycms.py:137 (Diff revision 11) > signerInfo['digestAlgorithm'] = self.pykeyHashToDigestAlgorithm(pykeyHash) > rsa = rfc2459.AlgorithmIdentifier() > rsa['algorithm'] = rfc2459.rsaEncryption > rsa['parameters'] = univ.Null() > authenticatedAttributes = self.buildAuthenticatedAttributes(digestValue, > - implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) > + implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0)) Error: Line too long (138 > 100 characters) [flake8: E501]
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79526c87b470 Fix flake8/pep8 issue by hand in mozglue/ r=glandium https://hg.mozilla.org/integration/autoland/rev/60441ab73a45 Run autopep8 on xpcom/ r=Nika https://hg.mozilla.org/integration/autoland/rev/11c2016bd2d2 Fix flake8/pep8 issue by hand in xpcom/ r=Nika https://hg.mozilla.org/integration/autoland/rev/a9f0b8f30751 Run autopep8 on accessible r=glandium https://hg.mozilla.org/integration/autoland/rev/2c96cab39473 Fix flake8/pep8 issue by hand in accessible/ r=glandium https://hg.mozilla.org/integration/autoland/rev/ecb4abccd843 Run autopep8 on memory/ r=njn https://hg.mozilla.org/integration/autoland/rev/f2c1f77e2a72 Fix flake8/pep8 issue by hand in memory/ r=njn https://hg.mozilla.org/integration/autoland/rev/4924d6ee1b83 Run autopep8 on js/ r=jorendorff https://hg.mozilla.org/integration/autoland/rev/fd9de8dd00a4 Fix flake8/pep8 issue by hand in js/ r=jorendorff https://hg.mozilla.org/integration/autoland/rev/f9a66e05d90e Run autopep8 on security/ r=fkiefer https://hg.mozilla.org/integration/autoland/rev/9968319230a7 Fix flake8/pep8 issue by hand in security/ r=fkiefer https://hg.mozilla.org/integration/autoland/rev/6576d0302313 autopep8 on testing/mozharness/mozharness/mozilla/testing/ r=jmaher https://hg.mozilla.org/integration/autoland/rev/c019186dbab5 Fix flake8/pep8 issue by hand in testing/mozharness/mozharness/mozilla/testing/ r=jmaher https://hg.mozilla.org/integration/autoland/rev/10e8e89771c9 autopep8 on mobile/ r=nalexander https://hg.mozilla.org/integration/autoland/rev/f2c0d683fdf7 Fix flake8/pep8 issue by hand in mobile/ r=nalexander https://hg.mozilla.org/integration/autoland/rev/11897c124ce2 Add accessible/, js/, mobile/, memory/, mozglue/, security/, testing/mozharness/mozharness/mozilla/testing/ and xpcom/ to the list of checked directory for flake8 r=ahal https://hg.mozilla.org/integration/autoland/rev/0230fba67294 Also ignore security/nss/ r=ahal
This added a bunch of newlines to unwind.py that make it less readable. For example consider: https://dxr.mozilla.org/mozilla-central/source/js/src/gdb/mozilla/unwind.py#531-536 This comment describes the following class, but there is one newline above the comment and two after it. Is there some way to redo the comment to make it pep-friendly? Or should I just turn all comments into doc strings or something like that?
Flags: needinfo?(sledru)
Tom, could you please a new bug for this? This bug is 2 months old and should not be used. Thanks!
Flags: needinfo?(sledru)
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: