----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38883/#review102624 -----------------------------------------------------------
Again lots of nit-picking, feel free to ignore what you disagree with strongly :) Also, instead of using your homemade `shell()` method and `Subprocess`, given the usage pattern I've noticed, you may want to consider the `sh` module: http://amoffat.github.io/sh/ once you start using it, it gets pretty awesome! support/apply-reviews.py (line 29) <https://reviews.apache.org/r/38883/#comment160357> always best to prefer `str.join()` to join strings; in this case, however, as you need to terminate with the `/` it's best to use `format()`: ``` return "{url}/{user}/".format(url=USER_URL, user=user) ``` of course: ``` '/'.join([USER_URL, user, '']) ``` would have also been acceptable. support/apply-reviews.py (line 34) <https://reviews.apache.org/r/38883/#comment160358> same here, please support/apply-reviews.py (line 40) <https://reviews.apache.org/r/38883/#comment160361> (setting aside for a second that we should use `requests.get()` instead :) can you please that you get a 200 OK code and that there is actually JSON content? with `requests` this is as easy as: ``` import requests ... try: headers = {'AcceptContent': "application/json"} r = requests.get(url, timeout=50, headers=headers) if 200 <= r.status_code < 300 and r.headers.get('content-type') == 'application/json': return r.json() else: # do something with error pass except requestsTimeoutError: # log the error and probably give up, the server is down pass ``` support/apply-reviews.py (line 59) <https://reviews.apache.org/r/38883/#comment160366> this will throw a KeyError if either of the keys are missing (and the user will be none the wiser): ``` if 'review_request' in json_obj and not json_obj['review_request].get('depends_on'): ... do something ``` (also see my comments in the other review about the lines above this) support/apply-reviews.py (line 78) <https://reviews.apache.org/r/38883/#comment160367> ``` from __future__ import print_function: ... print(output) ``` (this way it'll all work also for those us using a modern Python interpreter :) ) thanks! support/apply-reviews.py (line 86) <https://reviews.apache.org/r/38883/#comment160390> please don't do this - prefer the use of `**kwargs` or, in this case, it's much easier to do something like: ``` def remove_patch(review_id, options=None): options = options or {'dry_run': False} ... ``` support/apply-reviews.py (line 89) <https://reviews.apache.org/r/38883/#comment160368> consider using the `sh` module: ``` import sh cmd = "{rev_id}.patch".format(rev_id=review_id) sh.rm("-f", cmd) ``` support/apply-reviews.py (line 93) <https://reviews.apache.org/r/38883/#comment160392> can you please explains what is `options` supposed to be / contain? I think you expect a `dict` here, it would be great to have something like... ``` """ blah blah @param options: these are the fuz bits that god the baz, in a map that can contain the following keys: [`dry_run`, `verbose`, `bitz`] and if not specified assumes `dry_run` to be False. @type options: dict """ ``` support/apply-reviews.py (line 107) <https://reviews.apache.org/r/38883/#comment160394> prefer the use of `str.format()` instead (here and everywhere else) support/apply-reviews.py (lines 126 - 130) <https://reviews.apache.org/r/38883/#comment160399> I really really don't like this 'leading underscore' and same-naming: makes reading the code really difficult and, thanks to Python happily ignoring different types, the likelihood of bugs super-high. also, seems to me that you never use `_review` anywhere else, so that's what I'd do: ``` data = url_to_json(review_url(review_id)) if data: review = data.get('review_request') else: # something bad happened bail ``` same goes for `_user` too. support/apply-reviews.py (lines 133 - 139) <https://reviews.apache.org/r/38883/#comment160400> more pythonic: ``` review_data = { 'summary': review['summary'], 'description': review['description'], 'url': review_url(review_id), 'author': user['fullname'], 'email': user['email'] 'message': '\n\n'.join([review['summary'], review['description'], 'Review : {}'.format(review_data['url'])]) } ``` - Marco Massenzio On Oct. 8, 2015, 6:28 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38883/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2015, 6:28 p.m.) > > > Review request for mesos, Benjamin Hindman, 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/38883/diff/ > > > Testing > ------- > > Tested the script with python 2.7. > > > Thanks, > > Artem Harutyunyan > >
