Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120083002:
Enable stricter pylint checks. (Closed)
DescriptionEnable stricter pylint checks.
The webkitpy presubmit only checks modified lines, so if we enable some pylint checks that the codebase doesn't currently conform to, that will be OK -- this just means that as code is edited in the future, it can slowly be changed to pass these checks, and we can refine the pylint checks over time.
BUG=598897
Committed: https://crrev.com/79192ab645a00c75263cc7175e962c0cbba1c187
Cr-Commit-Position: refs/heads/master@{#407816}
Patch Set 1 #Patch Set 2 : Enable more pylint checks. #
Total comments: 7
Patch Set 3 : Remove comment changes #Patch Set 4 : Move comment so that it doesn't interfere with disable. #Messages
Total messages: 26 (14 generated)
Description was changed from ========== Enable stricter pylint checks. BUG=598897 ========== to ========== Enable stricter pylint checks. The webkitpy presubmit only checks modified lines, so if we enable some pylint checks that the codebase doesn't currently conform to, that will be OK -- this just means that as code is edited in the future, it can slowly be changed to pass these checks, and we can refine the pylint checks over time. BUG=598897 ==========
https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:65: design, design covers all of the "too-many-x" warnings. https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:158: max-module-lines=4000 Enabled line-too-long and file-too-long warnings, so these numbers are now significant. Reason for the number 4000 is: checkers/cpp.py is around 4000 lines. We could reduce this number later. https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:170: bad-functions=apply The map and filter functions appear in this codebase sometimes, and I think they're generally accepted. We could add more functions to this list though. https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:173: module-rgx=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+)|([a-z-][a-z0-9-]*))$ The scripts in Tools/Scripts have file names with dashes; to avoid pylint complaining about these, I added a regex for lower case with dashes. https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:201: good-names=i,j,k,ex,Run,_,_log _log is used as a constant name frequently in this codebase, which is OK.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, tansell@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Does this list of rules in pylintrc (and changes to the comments) seem OK? Note that this will apply for the presubmit for modified lines in the future, but will enable more pylint warnings for now; if there's any specific warnings that we want find we want to disable again then they could be disabled later.
Generally I like the idea here. If Dirk is happy with it, then do submit. https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:219: # Maximum number of locals for function / method body. I don't think we should change all the comment lines in this CL.
https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:219: # Maximum number of locals for function / method body. On 2016/07/21 at 05:37:54, mithro wrote: > I don't think we should change all the comment lines in this CL. Comment changes now removed from this CL
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/21 16:04:51, qyearsley wrote: > https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): > > https://codereview.chromium.org/2120083002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:219: # Maximum number of > locals for function / method body. > On 2016/07/21 at 05:37:54, mithro wrote: > > I don't think we should change all the comment lines in this CL. > > Comment changes now removed from this CL LGTM then.
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Enable stricter pylint checks. The webkitpy presubmit only checks modified lines, so if we enable some pylint checks that the codebase doesn't currently conform to, that will be OK -- this just means that as code is edited in the future, it can slowly be changed to pass these checks, and we can refine the pylint checks over time. BUG=598897 ========== to ========== Enable stricter pylint checks. The webkitpy presubmit only checks modified lines, so if we enable some pylint checks that the codebase doesn't currently conform to, that will be OK -- this just means that as code is edited in the future, it can slowly be changed to pass these checks, and we can refine the pylint checks over time. BUG=598897 Committed: https://crrev.com/79192ab645a00c75263cc7175e962c0cbba1c187 Cr-Commit-Position: refs/heads/master@{#407816} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/79192ab645a00c75263cc7175e962c0cbba1c187 Cr-Commit-Position: refs/heads/master@{#407816}
Message was sent while issue was closed.
lgtm also (belatedly). |
