Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737963003:
Update pylintrc to catch more style violations. (Closed)
|
Created:
3 years, 9 months ago by kjellander_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate pylintrc to catch more style violations.
The current PyLint configuration doesn't catch violations
to the Chromium Python Style Guide in a good way.
This change adds mostly the correct style regex for functions
and methods, with most content copied from
https://cs.chromium.org/chromium/tools/depot_tools/pylintrc
and (since the former disables invalid-name)
https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc
Many settings are the defaults, but are now more explicit to make it
easier to find them for our users.
Also convert the previous one-line list of disabled lint check into
a one-per-line list instead.
Add import-error to the list of disabled lint checks.
This CL depends on https://codereview.webrtc.org/2812273002/
to be landed first in order to not produce a lot of errors.
BUG=webrtc:7303
NOTRY=True
TESTED=git cl presubmit passing when tested in
combination with https://codereview.webrtc.org/2812273002/
TBR=niklas.enbom@webrtc.org
Review-Url: https://codereview.webrtc.org/2737963003
Cr-Commit-Position: refs/heads/master@{#17685}
Committed: https://chromium.googlesource.com/external/webrtc/+/38c65c8fb481e2ee209ebc1b52460894787f7ae3
Patch Set 1 #Patch Set 2 : Allow longer test methods and string module use #
Total comments: 21
Patch Set 3 : Cleanup #
Total comments: 2
Patch Set 4 : Relax function length to 60 chars #Patch Set 5 : Added OWNERS #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. BUG=webrtc:7303 NOTRY=True TESTED=I verified the settings are sane using: pylint.py --rcfile=pylintrc filename.py on a few files. I also verified there wasn't too many errors in existing code by running (some fixes will be needed though): find webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; find tools-webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; ========== to ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. BUG=webrtc:7303 NOTRY=True TESTED=I verified the settings are sane using: pylint.py --rcfile=pylintrc filename.py on a few files. I also verified there wasn't too many errors in existing code by running (some fixes will be needed though): find webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; find tools-webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; ==========
kjellander@webrtc.org changed reviewers: + jansson@webrtc.org
Since you requested this you get the honor to review it :) I plan to send out a PSA and do fixes in follow-up CLs to isolate them from this change.
https://codereview.webrtc.org/2737963003/diff/20001/pylintrc File pylintrc (right): https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode1 pylintrc:1: # Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. I assume you wrote 2015 since that was the original creation date of the file but it was just missing the copyright header? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode9 pylintrc:9: Maybe add a comment pointing to the original file that this was copied from? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode105 pylintrc:105: required-attributes= This is intentionally empty? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode201 pylintrc:201: ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by Add them on separate lines as you did for disable= on line 14? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode217 pylintrc:217: import-graph= This is intentionally empty? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode221 pylintrc:221: ext-import-graph= This is intentionally empty? https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode225 pylintrc:225: int-import-graph= This is intentionally empty?
Updated according to comments. I also removed a few accidentally disabled warnings compared to PS#2 after verifying that they didn't produce any additional lint errors. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc File pylintrc (right): https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode1 pylintrc:1: # Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2017/03/09 09:22:18, janssonWebRTC wrote: > I assume you wrote 2015 since that was the original creation date of the file > but it was just missing the copyright header? Yes. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode9 pylintrc:9: On 2017/03/09 09:22:18, janssonWebRTC wrote: > Maybe add a comment pointing to the original file that this was copied from? Done. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode105 pylintrc:105: required-attributes= On 2017/03/09 09:22:18, janssonWebRTC wrote: > This is intentionally empty? Yeah, it's a copy-paste. Let's remove it instead (I think empty is the default, but it's slightly confusing so better clean it up). https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode201 pylintrc:201: ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by On 2017/03/09 09:22:18, janssonWebRTC wrote: > Add them on separate lines as you did for disable= on line 14? I don't have an example of it and I'd rather not spend time on testing that it works. The disable-list is likely to be worked on (preferably we remove more of them) so it makes more sense to have that one in a diff-friendly format. This is unlikely to change anytime soon so I hope we can just leave it as is. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode217 pylintrc:217: import-graph= On 2017/03/09 09:22:18, janssonWebRTC wrote: > This is intentionally empty? Yes, it's the default. I removed it instead. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode221 pylintrc:221: ext-import-graph= On 2017/03/09 09:22:18, janssonWebRTC wrote: > This is intentionally empty? Yes, it's the default. I removed it instead. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode225 pylintrc:225: int-import-graph= On 2017/03/09 09:22:18, janssonWebRTC wrote: > This is intentionally empty? Yes, it's the default. I removed it instead. https://codereview.webrtc.org/2737963003/diff/40001/pylintrc File pylintrc (right): https://codereview.webrtc.org/2737963003/diff/40001/pylintrc#newcode19 pylintrc:19: E0611, This comes from the PRESUBMIT.py, where three warnings were configured - better have them all in the pylintrc. The F0401 warning I translated to import-error instead. The other two I wasn't able to find a friendly name for so they're kept with their original IDs.
LGTM https://codereview.webrtc.org/2737963003/diff/20001/pylintrc File pylintrc (right): https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode1 pylintrc:1: # Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > I assume you wrote 2015 since that was the original creation date of the file > > but it was just missing the copyright header? > > Yes. Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode9 pylintrc:9: On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > Maybe add a comment pointing to the original file that this was copied from? > > Done. Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode105 pylintrc:105: required-attributes= On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > This is intentionally empty? > > Yeah, it's a copy-paste. Let's remove it instead (I think empty is the default, > but it's slightly confusing so better clean it up). Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode201 pylintrc:201: ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > Add them on separate lines as you did for disable= on line 14? > > I don't have an example of it and I'd rather not spend time on testing that it > works. The disable-list is likely to be worked on (preferably we remove more of > them) so it makes more sense to have that one in a diff-friendly format. This is > unlikely to change anytime soon so I hope we can just leave it as is. Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode217 pylintrc:217: import-graph= On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > This is intentionally empty? > > Yes, it's the default. I removed it instead. Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode221 pylintrc:221: ext-import-graph= On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > This is intentionally empty? > > Yes, it's the default. I removed it instead. Acknowledged. https://codereview.webrtc.org/2737963003/diff/20001/pylintrc#newcode225 pylintrc:225: int-import-graph= On 2017/03/09 09:36:09, kjellander_webrtc wrote: > On 2017/03/09 09:22:18, janssonWebRTC wrote: > > This is intentionally empty? > > Yes, it's the default. I removed it instead. Acknowledged. https://codereview.webrtc.org/2737963003/diff/40001/pylintrc File pylintrc (right): https://codereview.webrtc.org/2737963003/diff/40001/pylintrc#newcode19 pylintrc:19: E0611, On 2017/03/09 09:36:09, kjellander_webrtc wrote: > This comes from the PRESUBMIT.py, where three warnings were configured - better > have them all in the pylintrc. > > The F0401 warning I translated to import-error instead. The other two I wasn't > able to find a friendly name for so they're kept with their original IDs. Acknowledged.
Description was changed from ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. BUG=webrtc:7303 NOTRY=True TESTED=I verified the settings are sane using: pylint.py --rcfile=pylintrc filename.py on a few files. I also verified there wasn't too many errors in existing code by running (some fixes will be needed though): find webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; find tools-webrtc/ -name "*.py" -exec pylint.py --rcfile=pylintrc {} \; ========== to ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when ==========
Description was changed from ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when ========== to ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when tested in combination with https://codereview.webrtc.org/2812273002/ ==========
The CQ bit was checked by kjellander@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from jansson@webrtc.org Link to the patchset: https://codereview.webrtc.org/2737963003/#ps60001 (title: "Relax function length to 60 chars")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by kjellander@webrtc.org
kjellander@webrtc.org changed reviewers: + niklas.enbom@webrtc.org
+niklas.enbom@webrtc.org for root approval since I also added an OWNERS change for pylintrc
Description was changed from ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when tested in combination with https://codereview.webrtc.org/2812273002/ ========== to ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when tested in combination with https://codereview.webrtc.org/2812273002/ TBR=niklas.enbom@webrtc.org ==========
TBRing since I want this landed before the holiday and I don't want new errors to sneak in.
The CQ bit was checked by kjellander@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from jansson@webrtc.org Link to the patchset: https://codereview.webrtc.org/2737963003/#ps80001 (title: "Added OWNERS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492062016279140,
"parent_rev": "6c84c279cd9305294b8c1e2b57279533df372bd1", "commit_rev":
"38c65c8fb481e2ee209ebc1b52460894787f7ae3"}
Message was sent while issue was closed.
Description was changed from ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when tested in combination with https://codereview.webrtc.org/2812273002/ TBR=niklas.enbom@webrtc.org ========== to ========== Update pylintrc to catch more style violations. The current PyLint configuration doesn't catch violations to the Chromium Python Style Guide in a good way. This change adds mostly the correct style regex for functions and methods, with most content copied from https://cs.chromium.org/chromium/tools/depot_tools/pylintrc and (since the former disables invalid-name) https://cs.chromium.org/chromium/src/third_party/chromite/pylintrc Many settings are the defaults, but are now more explicit to make it easier to find them for our users. Also convert the previous one-line list of disabled lint check into a one-per-line list instead. Add import-error to the list of disabled lint checks. This CL depends on https://codereview.webrtc.org/2812273002/ to be landed first in order to not produce a lot of errors. BUG=webrtc:7303 NOTRY=True TESTED=git cl presubmit passing when tested in combination with https://codereview.webrtc.org/2812273002/ TBR=niklas.enbom@webrtc.org Review-Url: https://codereview.webrtc.org/2737963003 Cr-Commit-Position: refs/heads/master@{#17685} Committed: https://chromium.googlesource.com/external/webrtc/+/38c65c8fb481e2ee209ebc1b5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/38c65c8fb481e2ee209ebc1b5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
