David Knupp has posted comments on this change. Change subject: IMPALA-3225: Add script to push from gerrit to ASF ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3468/1/bin/push_to_asf.py File bin/push_to_asf.py: PS1, Line 89: pass This can be removed, yeah? Line 104: OPTIONS = None Seems weird to stick this one constant midway down the file. Can it be moved up to the top with the others? PS1, Line 115: print >>sys.stderr, "No remote named 'apache'. Please set one up, for example with: " : print >>sys.stderr, " git remote add apache", APACHE_REPO This is probably just copy/pasted from the Kudu script, but using successive print statements simply to introduce newlines seems maybe... kludgy? It would admittedly be kind of a hassle to do this throughout (because it happens in several places), but maybe opt for constructing a properly formatted string for the desired output, and then pass it to sys.stderr.write(). Besides for sys.stderr.write(), another future-proof option to using a naked print statement is import the print_function from __future__, and passing in sys.stderr for the "file" param. Also, I don't know if it's going to ever be a concern of ours, but using the print statements as is (which is done liberally throughout) will mean that our code is guaranteed to never be python 3.x compatible. Is that EVER going to be an issue for us? These types of changes might be outside of the scope of this submission, and certainly less critical than actually getting this to work, but I thought it at least worth mentioning. PS1, Line 253: + Minor: Python concatenates adjacent strings within braces automatically, so this operator isn't needed. -- To view, visit http://gerrit.cloudera.org:8080/3468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15de939acc4b08a8f01511fffcb6ab1743d09c01 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
