Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11054 )
Change subject: IMPALA-7317: add scripts to post flake8 comments ...................................................................... Patch Set 13: (3 comments) Yeah I was anticipating that this might grow by adding more functions to generate comments. It's unclear if this is likely to be a useful utility library or just a script that runs on jenkins, so I figured it was easiest to keep it as a single-file script for now. http://gerrit.cloudera.org:8080/#/c/11054/13/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/11054/13/bin/jenkins/critique-gerrit-review.py@91 PS13, Line 91: VIOLATION_RE = re.compile(r"^([^:]*):([0-9]*):([0-9]*): (.*).*$") > Why "(.*).*$" here? What's the second .* possibly matching, since the first Yeah, that wasn't intended - leftover from an earlier iteration of the regex http://gerrit.cloudera.org:8080/#/c/11054/13/bin/jenkins/critique-gerrit-review.py@102 PS13, Line 102: "start_character": col - 1, "end_character": col}, > Based on your sample here, if the character is 1, we should probably highli Seems like a reasonable heuristic. http://gerrit.cloudera.org:8080/#/c/11054/13/bin/jenkins/critique-gerrit-review.py@124 PS13, Line 124: comments = defaultdict(lambda: []) : get_flake8_comments(comments) > It's a little weird that this isn't: I was anticipating that we might add more functions with the same API to build up a single dictionary, but it probably is still more readable in that case to return dictionaries and merge them. -- To view, visit http://gerrit.cloudera.org:8080/11054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d348ea4944f829a407bd7b2939654f272736170 Gerrit-Change-Number: 11054 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 30 Jul 2018 17:38:25 +0000 Gerrit-HasComments: Yes
