You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2017/01/30 23:25:36 UTC
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Lars Volker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5829
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Add .pep8rc for Impala's Python style
Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
---
A .pep8rc
1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/5829/1
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 2: # E101 - Reindent all lines.
> Without E101 and E111, autopep8 will re-indent the whole file to use 4 spac
For what it's worth, pursuant to the discussion on dev@, I don't think this is our one shot at this. As a first goal (setting up tooling), I think that ignoring this is probably OK. We can revisit later, right?
PS1, Line 6: Add missing blank line
Does making the description more verbose affect the functionality?
"Expected 1 blank line, found 0. Add missing blank line."
http://gerrit.cloudera.org:8080/#/c/5829/1//COMMIT_MSG
Commit Message:
PS1, Line 7: Add .pep8rc for Impala's Python style
> No tools are automatically affected by this, but several can use it if poin
I would appreciate having this pointed out here. Just speaking for myself, if I had questions about this file, I'd probably start by looking at the git log and read the commit messages before I searched on a wiki somewhere.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 2: # E101 - Reindent all lines.
I moderately disagree with this being ignored. Can you talk about your reasonings for wanting to ignore it?
PS1, Line 5: # E251 - Remove whitespace around parameter '=' sign.
: # E301 - Add missing blank line.
I strongly disagree with these being ignored. Can you talk about your reasonings for wanting to ignore them?
http://gerrit.cloudera.org:8080/#/c/5829/1//COMMIT_MSG
Commit Message:
PS1, Line 7: Add .pep8rc for Impala's Python style
What tools get affected by the presence of this? Can it stay in the top-level Impala directory, or do I need to copy it to my homedir?
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
I didn't have time to look into this, but I also found yapf, a clang-format based python formatter. It is still on my list to look at this, but if keeping the change around clutters things up, I can abandon it and push something new when I get to it.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/5829 )
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Abandoned
Not finding time to work on this one.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-Change-Number: 5829
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 5: # E251 - Remove whitespace around parameter '=' sign.
: # E301 - Add missing blank line.
> Yes, that was my intention, too. Such a tool sounds very useful. Is it in a
https://github.com/jbapple-cloudera/clang-format-infer
It uses hill-climbing with random restarts to search the state space.
The config is pretty brittle, I must admit, but I think that's mainly a clang problem, so you might be able to repurpose it.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 2: # E101 - Reindent all lines.
> I moderately disagree with this being ignored. Can you talk about your reas
Without E101 and E111, autopep8 will re-indent the whole file to use 4 spaces instead of 2, and I couldn't find a way around this.
https://github.com/hhatto/autopep8/blob/master/autopep8.py#L3095
PS1, Line 5: # E251 - Remove whitespace around parameter '=' sign.
: # E301 - Add missing blank line.
> I strongly disagree with these being ignored. Can you talk about your reaso
I tried to make this match the style that I found, but I don't feel strongly about it and would be glad if we could get as close to PEP8 as possible. I'll comment more on this on dev@.
http://gerrit.cloudera.org:8080/#/c/5829/1//COMMIT_MSG
Commit Message:
PS1, Line 7: Add .pep8rc for Impala's Python style
> What tools get affected by the presence of this? Can it stay in the top-lev
No tools are automatically affected by this, but several can use it if pointed to it with a command line flag. Should we point this out here or would the wiki page be sufficient?
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
Any news with this?
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 5: # E251 - Remove whitespace around parameter '=' sign.
: # E301 - Add missing blank line.
> I can't speak for Lars, but I worked on our initial .clang-format, and I tr
Yes, that was my intention, too. Such a tool sounds very useful. Is it in any way re-usable and would you be able to share it?
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5829/1/.pep8rc
File .pep8rc:
PS1, Line 5: # E251 - Remove whitespace around parameter '=' sign.
: # E301 - Add missing blank line.
> I strongly disagree with these being ignored. Can you talk about your reaso
I can't speak for Lars, but I worked on our initial .clang-format, and I tried to make it match our existing style as closely as possible. I even wrote a little tool to explore the state space of config options and find the ones that match our codebase with as small a diff as possible. As a result of this matching, the options are a bit odd, but it will hopefully encourage new code to match the old style as closely as possible, which might be easier to read than having new code be in the accepted pep8 style and old code in the idiosyncratic Impala style.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5829/1//COMMIT_MSG
Commit Message:
PS1, Line 7: Add .pep8rc for Impala's Python style
> I would appreciate having this pointed out here. Just speaking for myself,
Or it could be a comment in the file itself.
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add .pep8rc for Impala's Python style
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: Add .pep8rc for Impala's Python style
......................................................................
Patch Set 1:
> Any news with this?
I should point out we had a discussion here:
http://mail-archives.apache.org/mod_mbox/incubator-impala-dev/201701.mbox/%3CCAM_aD9fFLEWBshRfaVEuHURZhTm0-VFReqO_dvuO0EvrtDNU3A%40mail.gmail.com%3E
The February portion of the thread continues here:
http://mail-archives.apache.org/mod_mbox/incubator-impala-dev/201702.mbox/%3CCAM_aD9dqUxF1dOQa=cG0xETUZHx-T4F3QLBWg94r39MOzoH18g@mail.gmail.com%3E
--
To view, visit http://gerrit.cloudera.org:8080/5829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I65a99fc6e364eb1e49e21d0ba97546bc25f65a27
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No