You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/07/30 23:58:08 UTC
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11085
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
WIP: IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line.
Testing:
Added some violations to this code change.
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 72 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/11085/1
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2894/
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 16:27:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/121/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 23:05:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Thanks. Makes sense to me. I think the whitelist/blacklist approach will be handy.
http://gerrit.cloudera.org:8080/#/c/11085/5/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/5/bin/jenkins/critique-gerrit-review.py@65
PS5, Line 65: EXCLUDE_FILE_PATTERNS = [
nice
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 23:05:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has removed a vote on this change.
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@114
PS2, Line 114: # This is a long line that I will remove before merging ===================================================
line too long (107 > 90)
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@115
PS2, Line 115: # This is a line with a tab that I will remove
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@115
PS2, Line 115: # This is a line with a tab that I will remove
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@192
PS2, Line 192:
flake8: E305 expected 2 blank lines after class or function definition, found 1
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 00:08:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/108/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 00:27:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 5:
(4 comments)
Added a --dryrun mode to disable the postback to gerrit. I was already running it locally to test but relying on the ssh to gerrit failing.
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@124
PS2, Line 124: # This is a long line that I will remove before merging ===================================================
: # This is a line with a tab that I will remove
: def get_misc_comments(revision):
> I think you can compress lines 121-126 by just using
Done
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@148
PS2, Line 148: break
> Should this be diff_line[2:]? i.e., I think that additions are prefix with
They're not in this output mode at least - there are plenty of lines with + and no subsequent space.
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@162
PS2, Line 162:
> What's going to be our policy for ignoring these? Do we want to whitelist t
I added a whitelist of source file extensions so it will only apply to those.
I think we still want to check comments - comments are actually one of the more common places for weird formatting in my experience.
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@167
PS2, Line 167: # Check for trailing whitespace.
> nit: s/tables/tabs
Removed since makefile isn't in whitelists anyway.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 22:18:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2894/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 06:27:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/120/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 23:00:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@113
PS1, Line 113: # This is a long line that I will remove before merging ===================================================
line too long (107 > 90)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@114
PS1, Line 114:
flake8: E501 line too long (107 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@114
PS1, Line 114: # This is a line with a tab that I will remove
tab used for whitespace
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 16:33:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 7: Verified+1
Hit some build flakiness that I need to investigate. Precommit tests don't exercise this code but all RAT and other checks passed.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 17:23:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@127
PS1, Line 127: raise Exception("Did not expect flake8-diff to write to stderr:\n{0}".format(stderr))
flake8: E305 expected 2 blank lines after class or function definition, found 1
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 00:03:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@113
PS1, Line 113: # This is a long line that I will remove before merging ===================================================
line too long (107 > 90)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@114
PS1, Line 114:
flake8: E501 line too long (107 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@114
PS1, Line 114: # This is a line with a tab that I will remove
tab used for whitespace
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 16:33:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/131/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 07:00:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11085
to look at the new patch set (#2).
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
WIP: IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line.
Testing:
Added some violations to this code change.
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 71 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/11085/2
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@127
PS1, Line 127: raise Exception("Did not expect flake8-diff to write to stderr:\n{0}".format(stderr))
flake8: E305 expected 2 blank lines after class or function definition, found 1
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 23:59:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11085
to look at the new patch set (#6).
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line. These are applied
to Java, Python, C++, shell and thrift source files with
some exclusions.
Adds a --dryrun option that does not try to post back the
comments to gerrit.
Remove the option to specify a git revision since flake8-diff doesn't
work correctly when run on source that isn't checked out.
Testing:
Added some violations to this code change that will be
fixed before merging. The output is:
{
"comments": {
"bin/jenkins/critique-gerrit-review.py": [
{
"message": "flake8: E501 line too long (107 > 90 characters)",
"range": {
"start_character": 90,
"start_line": 124,
"end_line": 124,
"end_character": 91
}
},
{
"message": "tab used for whitespace",
"line": 125
}
]
}
}
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 91 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/11085/6
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/110/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 00:45:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 06:27:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line. These are applied
to Java, Python, C++, shell and thrift source files with
some exclusions.
Adds a --dryrun option that does not try to post back the
comments to gerrit.
Remove the option to specify a git revision since flake8-diff doesn't
work correctly when run on source that isn't checked out.
Testing:
Added some violations to this code change that will be
fixed before merging. The output is:
{
"comments": {
"bin/jenkins/critique-gerrit-review.py": [
{
"message": "flake8: E501 line too long (107 > 90 characters)",
"range": {
"start_character": 90,
"start_line": 124,
"end_line": 124,
"end_character": 91
}
},
{
"message": "tab used for whitespace",
"line": 125
}
]
}
}
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Reviewed-on: http://gerrit.cloudera.org:8080/11085
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Tim Armstrong <ta...@cloudera.com>
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 91 insertions(+), 5 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved
Tim Armstrong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 2:
(4 comments)
Do we need a mode to run this manually?
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@124
PS2, Line 124: stdout, _ = git_diff_proc.communicate()
: if git_diff_proc.returncode != 0:
: raise Exception("Git diff exited with code:\n{0}".format(git_diff_proc.returncode))
I think you can compress lines 121-126 by just using
stdout = subprocess.check_output(...)
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@148
PS2, Line 148: add_misc_comments_for_line(comments, diff_line[1:], curr_file, curr_line_num)
Should this be diff_line[2:]? i.e., I think that additions are prefix with "+ " (2 characters).
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@162
PS2, Line 162: if len(line) > 90:
What's going to be our policy for ignoring these? Do we want to whitelist this to source code (py, cc, h, java) and non-comment (i.e., strip off "//") and limit the number of hits overall?
git grep -E '.{81}' returns quite a few of results...
http://gerrit.cloudera.org:8080/#/c/11085/2/bin/jenkins/critique-gerrit-review.py@167
PS2, Line 167: # Check for tabs (unless it's a Makefile, which requires tables).
nit: s/tables/tabs
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 20:53:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11085
to look at the new patch set (#4).
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line. These are applied
to Java, Python, C++, shell and thrift source files with
some exclusions.
Adds a --dryrun option that does not try to post back the
comments to gerrit.
Remove the option to specify a git revision since flake8-diff doesn't
work correctly when run on source that isn't checked out.
Testing:
Added some violations to this code change that will be
fixed before merging.
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
---
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/kudu/util/website_util.h
M bin/jenkins/critique-gerrit-review.py
3 files changed, 95 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/11085/4
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] WIP: IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: WIP: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@113
PS1, Line 113: =
flake8: E501 line too long (107 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@113
PS1, Line 113: # This is a long line that I will remove before merging ===================================================
line too long (107 > 90)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@114
PS1, Line 114: # This is a line with a tab that I will remove
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@115
PS1, Line 115: def get_misc_comments(revision):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@173
PS1, Line 173:
flake8: E303 too many blank lines (3)
http://gerrit.cloudera.org:8080/#/c/11085/1/bin/jenkins/critique-gerrit-review.py@192
PS1, Line 192: a[k].extend(v)
flake8: E305 expected 2 blank lines after class or function definition, found 1
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 00:06:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 06:27:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11085
to look at the new patch set (#5).
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
IMPALA-7317: comment on long lines and whitespace
Add some basic formatting checks that can be implemented
easily by parsing the diff line-by-line. These are applied
to Java, Python, C++, shell and thrift source files with
some exclusions.
Adds a --dryrun option that does not try to post back the
comments to gerrit.
Remove the option to specify a git revision since flake8-diff doesn't
work correctly when run on source that isn't checked out.
Testing:
Added some violations to this code change that will be
fixed before merging. The output is:
{
"comments": {
"bin/jenkins/critique-gerrit-review.py": [
{
"message": "flake8: E501 line too long (107 > 90 characters)",
"range": {
"start_character": 90,
"start_line": 124,
"end_line": 124,
"end_character": 91
}
},
{
"message": "tab used for whitespace",
"line": 125
}
]
}
}
Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
---
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/kudu/util/website_util.h
M bin/jenkins/critique-gerrit-review.py
3 files changed, 95 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/11085/5
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-7317: comment on long lines and whitespace
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11085 )
Change subject: IMPALA-7317: comment on long lines and whitespace
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2897/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36bb99560ab09d7753ff93227d1c4972582770f2
Gerrit-Change-Number: 11085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 17:22:11 +0000
Gerrit-HasComments: No