|
|
DescriptionUpdate Python style guide to point out exceptions only apply to legacy scripts
Review-Url: https://codereview.chromium.org/2626863005
Cr-Commit-Position: refs/heads/master@{#444608}
Committed: https://chromium.googlesource.com/chromium/src/+/27e7d60bc5bb4afb081116e39905aa66e7cf2489
Patch Set 1 #
Total comments: 1
Patch Set 2 : follow->following #Patch Set 3 : New scripts should be PEP-8 compliant #Messages
Total messages: 23 (6 generated)
ktyliu@chromium.org changed reviewers: + brettw@chromium.org
Hi Brett, I would like to tweak the wording of the Python part of the style guide you wrote, pointing out that the exception only applies to parts of the code base. In particular, some parts of the Chromium codebase use (PEP-8) lower_underscore function names and/or 4-space indent: https://cs.chromium.org/search/?q=f:src/.*py$+def%5B+%5D%5Ba-z%5D+case:yes&sq... And Blink strictly enforces PEP-8 in presubmit checker: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp... Please point out if you see issues with the proposal. Thanks, Kevin
drive-by typo finder https://codereview.chromium.org/2626863005/diff/1/styleguide/styleguide.md File styleguide/styleguide.md (right): https://codereview.chromium.org/2626863005/diff/1/styleguide/styleguide.md#ne... styleguide/styleguide.md:19: Some parts of the Chromium codebase have the follow exceptions: follow => following
On 2017/01/13 at 17:15:23, danakj wrote: > drive-by typo finder > > https://codereview.chromium.org/2626863005/diff/1/styleguide/styleguide.md > File styleguide/styleguide.md (right): > > https://codereview.chromium.org/2626863005/diff/1/styleguide/styleguide.md#ne... > styleguide/styleguide.md:19: Some parts of the Chromium codebase have the follow exceptions: > follow => following Thanks for the catch! Fixed
How does one know where such exceptions apply?
On 2017/01/17 at 20:53:15, brettw wrote: > How does one know where such exceptions apply? PEP-8 should be the preferred, unless the existing script or immediate codebase have these exceptions? Please advise if you have some preferred wording. Thanks
For context, I didn't write the style guide here, I copied it from the sites page. If we say that different parts of the code have different styles, we should be able to tell people how to figure out which style should apply. How do I tell if I should write with 2 or 4 space indents?
On 2017/01/18 at 22:22:23, brettw wrote: > For context, I didn't write the style guide here, I copied it from the sites page. > > If we say that different parts of the code have different styles, we should be able to tell people how to figure out which style should apply. How do I tell if I should write with 2 or 4 space indents? Thanks for clarification that you didn't write this style guide. cs.chromium.org search results show that there's now more .py scripts that follow PEP-8 style with 6,635 scripts having lower case function names vs 3,755 having capital case function names. Reworded to just acknowledge there's legacy scripts with the exceptions but new scripts should be PEP-8 compliant.
Description was changed from ========== Update Python style guide to point out exceptions only apply to parts of codebase ========== to ========== Update Python style guide to point out exceptions only apply to legacy scripts ==========
lgtm
The CQ bit was checked by ktyliu@chromium.org
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": 1484793434321900, "parent_rev": "db28fd072daf04bd58087e14eece78753a9f0389", "commit_rev": "27e7d60bc5bb4afb081116e39905aa66e7cf2489"}
Message was sent while issue was closed.
Description was changed from ========== Update Python style guide to point out exceptions only apply to legacy scripts ========== to ========== Update Python style guide to point out exceptions only apply to legacy scripts Review-Url: https://codereview.chromium.org/2626863005 Cr-Commit-Position: refs/heads/master@{#444608} Committed: https://chromium.googlesource.com/chromium/src/+/27e7d60bc5bb4afb081116e39905... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/27e7d60bc5bb4afb081116e39905...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Was this discussed or announced somewhere? I happened to see this by coincidence today, and I wasn't aware of this change. And judging from reviews and CLs, nobody else is either.
Message was sent while issue was closed.
3019 hits for 2 indent: https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5CS+pcr... 79 hits for 4 indent: https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+... So this change isn't documenting existing practice. Unless there was some discussion about this somewhere, I think this should be reverted.
Message was sent while issue was closed.
On 2017/04/06 14:38:51, Nico wrote: > 3019 hits for 2 indent: > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5CS+pcr... > > 79 hits for 4 indent: > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+... > > So this change isn't documenting existing practice. Unless there was some > discussion about this somewhere, I think this should be reverted. ktyliu: ping ^
Message was sent while issue was closed.
On 2017/04/06 14:38:51, Nico wrote: > 3019 hits for 2 indent: > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5CS+pcr... > > 79 hits for 4 indent: > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+... > > So this change isn't documenting existing practice. Unless there was some > discussion about this somewhere, I think this should be reverted. there's 7,700 hits for 4 indents overall: https://cs.chromium.org/search/?q=f:py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+%5CS+p... are you suggesting that it's a bad idea to adhere to PEP-8 style in Chromium?
Message was sent while issue was closed.
On 2017/04/14 06:24:56, ktyliu_google wrote: > On 2017/04/06 14:38:51, Nico wrote: > > 3019 hits for 2 indent: > > > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5CS+pcr... > > > > 79 hits for 4 indent: > > > https://cs.chromium.org/search/?q=f:src/.*py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+... > > > > So this change isn't documenting existing practice. Unless there was some > > discussion about this somewhere, I think this should be reverted. > > there's 7,700 hits for 4 indents overall: > https://cs.chromium.org/search/?q=f:py$+%5Cn%5CS.*:%5Cn%5C+%5C+%5C+%5C+%5CS+p... Sure, if you include all the third-party code that was written being PEP-8 compliant. > are you suggesting that it's a bad idea to adhere to PEP-8 style in Chromium? I don't have an opinion on this, but we (chrome, before me) made the decision to use https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#i... style and ~all non-third-party code is consistent with that. If you want to change that, this should be discussed somewhere, not just landed in a CL that nobody saw saying that it documents existing practice, which it doesn't.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2819823002/ by thakis@chromium.org. The reason for reverting is: This doesn't only apply to legacy scripts; revert pending real discussion somewhere..
Message was sent while issue was closed.
Sorry, my understanding of the Python style was not very good. |