Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738823003:
webkitpy: Slightly adjusting pylintrc allow names. (Closed)
Descriptionwebkitpy: Slightly adjusting pylintrc allow names.
Justifications for each of the existing names have been added as well.
This adds two more allowed cases;
-----
with open(filename) as f:
This is a very common pattern in Python as you can't use the more
logical name "file" as it shadows the inbuilt file function
(which is equivalent to open). Other names include fd.
-----
try:
....
exception XXXError as e:
....
Another very common pattern in Python. Other names include err and exp.
-----
Review-Url: https://codereview.chromium.org/2738823003
Cr-Commit-Position: refs/heads/master@{#455664}
Committed: https://chromium.googlesource.com/chromium/src/+/3458e9e5a0092aaae6697435f2be3717a423766b
Patch Set 1 #Patch Set 2 : webkitpy: Slightly adjusting pylintrc allow names. #
Total comments: 6
Patch Set 3 : Small fixes #Messages
Total messages: 16 (7 generated)
Description was changed from ========== webkitpy: Slightly adjusting pylintrc allow names. Plus adding the places these short names are expected to be used. ========== to ========== webkitpy: Slightly adjusting pylintrc allow names. Justifications for each of the existing names have been added as well. This adds two more allowed cases; ----- with open(filename) as f: This is a very common pattern in Python as you can't use the more logical name "file" as it shadows the inbuilt file function (which is equivalent to open). Sometimes fd is preferred. ----- try: .... exception XXXError as e: .... Another very common pattern in Python. Other names include err and exp. ----- ==========
tansell@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
Description was changed from ========== webkitpy: Slightly adjusting pylintrc allow names. Justifications for each of the existing names have been added as well. This adds two more allowed cases; ----- with open(filename) as f: This is a very common pattern in Python as you can't use the more logical name "file" as it shadows the inbuilt file function (which is equivalent to open). Sometimes fd is preferred. ----- try: .... exception XXXError as e: .... Another very common pattern in Python. Other names include err and exp. ----- ========== to ========== webkitpy: Slightly adjusting pylintrc allow names. Justifications for each of the existing names have been added as well. This adds two more allowed cases; ----- with open(filename) as f: This is a very common pattern in Python as you can't use the more logical name "file" as it shadows the inbuilt file function (which is equivalent to open). Other names include fd. ----- try: .... exception XXXError as e: .... Another very common pattern in Python. Other names include err and exp. ----- ==========
Hi Dirk / Quinten, I'm suggest these slight changes to the existing pylintrc. What do you think? Tim 'mithro' Ansell
Seems reasonable to me, LGTM
I'll wait for Dirk's thoughts before submitting.
A couple quick nits https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:206: # These short names are acceptable in the following cases; Nit: semicolon -> colon https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:208: # i/j/k/x - for XXX in range(): Bikeshedding: I think in most cases where you'd want to use `x`, there's probably a better variable name. https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:209: # e/ex/err - exception XXXError as e: I'm not sure if ex and err need to be in the below list (since lowercase names longer than two characters are OK according to variable-rgx above) Also I personally feel that in that case it's nicer to use a word (e.g. error) as the variable name.
The suggested list is fine by me, but I tend to be very pro-short-name :). qyearsley@, it's up to you.
On 2017/03/08 at 21:08:30, dpranke wrote: > The suggested list is fine by me, but I tend to be very pro-short-name :). qyearsley@, it's up to you. I'm also OK with the change as-is, the above nits are just opinions, nothing important :-)
https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/pylintrc (right): https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:206: # These short names are acceptable in the following cases; On 2017/03/08 15:04:11, qyearsley wrote: > Nit: semicolon -> colon Done. https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:208: # i/j/k/x - for XXX in range(): On 2017/03/08 15:04:11, qyearsley wrote: > Bikeshedding: I think in most cases where you'd want to use `x`, there's > probably a better variable name. Actually the case where x makes sense, y and z also make sense. IE Where the values are coordinates. https://codereview.chromium.org/2738823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/pylintrc:209: # e/ex/err - exception XXXError as e: On 2017/03/08 15:04:11, qyearsley wrote: > I'm not sure if ex and err need to be in the below list (since lowercase names > longer than two characters are OK according to variable-rgx above) > > Also I personally feel that in that case it's nicer to use a word (e.g. error) > as the variable name. I removed ex and exp but as the following is extremely common in Python code, exception XXError as e: By the consistency rule I think we should aim to use that.
The CQ bit was checked by tansell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2738823003/#ps40001 (title: "Small fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489028485909460,
"parent_rev": "d5691fadbe2869c29e3983ceb3385bbff576c3cc", "commit_rev":
"3458e9e5a0092aaae6697435f2be3717a423766b"}
Message was sent while issue was closed.
Description was changed from ========== webkitpy: Slightly adjusting pylintrc allow names. Justifications for each of the existing names have been added as well. This adds two more allowed cases; ----- with open(filename) as f: This is a very common pattern in Python as you can't use the more logical name "file" as it shadows the inbuilt file function (which is equivalent to open). Other names include fd. ----- try: .... exception XXXError as e: .... Another very common pattern in Python. Other names include err and exp. ----- ========== to ========== webkitpy: Slightly adjusting pylintrc allow names. Justifications for each of the existing names have been added as well. This adds two more allowed cases; ----- with open(filename) as f: This is a very common pattern in Python as you can't use the more logical name "file" as it shadows the inbuilt file function (which is equivalent to open). Other names include fd. ----- try: .... exception XXXError as e: .... Another very common pattern in Python. Other names include err and exp. ----- Review-Url: https://codereview.chromium.org/2738823003 Cr-Commit-Position: refs/heads/master@{#455664} Committed: https://chromium.googlesource.com/chromium/src/+/3458e9e5a0092aaae6697435f2be... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3458e9e5a0092aaae6697435f2be... |
