----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38705/#review100505 -----------------------------------------------------------
Mostly just python style and commenting. My python is a bit rusty though. Could you also mention in "Testing Done" whether this is python 2 or 3 or both? support/apply-reviews.py (line 9) <https://reviews.apache.org/r/38705/#comment157711> Spaces around `=`? support/apply-reviews.py (line 11) <https://reviews.apache.org/r/38705/#comment157713> Could you change these method comments to python docstrings comments? i.e. ``` def review_url(id): """Returns the Review Board URL for the given review ID.""" ``` support/apply-reviews.py (line 13) <https://reviews.apache.org/r/38705/#comment157712> Might be cleaner/safer to use urlparse.urljoin for this: https://docs.python.org/2/library/urlparse.html#urlparse.urljoin support/apply-reviews.py (line 15) <https://reviews.apache.org/r/38705/#comment157714> Docstring-ify. Looks like this method is just json-ifying an HTTP GET from the url. (Reword comment?) support/apply-reviews.py (line 20) <https://reviews.apache.org/r/38705/#comment157715> Docstring-ify. support/apply-reviews.py (lines 26 - 28) <https://reviews.apache.org/r/38705/#comment157716> Docstring-ify. Is the list of reviews in reverse order of the chain? support/apply-reviews.py (line 29) <https://reviews.apache.org/r/38705/#comment157720> I believe the python convention for (not-really) "private" helper methods is a leading underscore. i.e. `def _parent_review(...)` support/apply-reviews.py (line 40) <https://reviews.apache.org/r/38705/#comment157721> Extra newline here. support/apply-reviews.py (lines 44 - 45) <https://reviews.apache.org/r/38705/#comment157717> Docstring-ify. support/apply-reviews.py (line 46) <https://reviews.apache.org/r/38705/#comment157722> Seems like you could combine this into `parent_review_ex` by changing the function prototype to: `parent_review(id, list=[])` support/apply-reviews.py (line 49) <https://reviews.apache.org/r/38705/#comment157718> Docstring-ify. support/apply-reviews.py (line 55) <https://reviews.apache.org/r/38705/#comment157719> Docstring-ify. support/apply-reviews.py (line 56) <https://reviews.apache.org/r/38705/#comment157725> It would be great if this included a snippet of the review's summary. support/apply-reviews.py (line 68) <https://reviews.apache.org/r/38705/#comment157724> Seems like you don't need a `-r` in this case, because we'd only use this script for reviewboard. i.e. s/-r/review/ support/apply-reviews.py (line 72) <https://reviews.apache.org/r/38705/#comment157723> Add `'--dry-run'` to this option? - Joseph Wu On Sept. 23, 2015, 9:26 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38705/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2015, 9:26 p.m.) > > > Review request for mesos, Joris Van Remoortere, Joseph Wu, and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/38705/diff/ > > > Testing > ------- > > > Thanks, > > Artem Harutyunyan > >
