----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38705/#review101980 -----------------------------------------------------------
support/apply-reviews.py (line 7) <https://reviews.apache.org/r/38705/#comment159488> uhm... could we use `requests` instead? much more modern API and widespread use. support/apply-reviews.py (line 11) <https://reviews.apache.org/r/38705/#comment159484> nit: you are 'masking' the global builtin `id()` here - that's a PEP8 violation, could you please rename to something like `rid`? support/apply-reviews.py (line 13) <https://reviews.apache.org/r/38705/#comment159486> don't concatenate string, prefer the use of os.path.join(): ``` import os os.path.join(REVIEW_URL, rid) ``` (why do you need the trailing slash?) support/apply-reviews.py (line 19) <https://reviews.apache.org/r/38705/#comment159489> you should check for a non 4xx/5xx return code (or just check for a 2xx if you know for sure what the API returns). as you expect JSON, you should probably specify that in the `Accept-content` header too? support/apply-reviews.py (line 24) <https://reviews.apache.org/r/38705/#comment159490> if you are doing this often enough, it's much better to compile this to a constant Pattern and then use that instead. support/apply-reviews.py (line 29) <https://reviews.apache.org/r/38705/#comment159495> you should not call a param with the name of a type (`list` is a type in Python). also, it's bad practice to initialize it as you do there. BTW - can you please use the @param to explain what the method expects? Prefer: ``` def parent_review(rid, revs_list=None): """Returns a list with reversed chain of review requests for a given Review ID. """ result = revs_list or [] json_str = review_json(review_url(rid)) json_obj = json.loads(json_str) # Stop as soon as we stumble upon a submitted request. rr = json_obj.get('review_request') if rr and rr.get('status') == "submitted": return result for deps in rr.get('summary'): ... ``` Also, worth always using `get()` instead of accessor (`[]`) for data that you are not sure may or may not be there (it's an API response after all) - the latter will throw a KeyError if the key is missing; `get` will just give you back None (or you can pass a second optional default return value if you want). support/apply-reviews.py (line 40) <https://reviews.apache.org/r/38705/#comment159497> is this a recursion inside an iteration? can you please add a comment? I'm sure folks who may one day try to fix/augment this script will be just as confused... support/apply-reviews.py (line 45) <https://reviews.apache.org/r/38705/#comment159496> is this right? you return after the first iteration? if so, why not just getting the first item in `depends_on`? support/apply-reviews.py (line 56) <https://reviews.apache.org/r/38705/#comment159499> if you do this, this script will be probably useful to folks who use Python 3 too :) ``` from __future__ import print_function ... print('foo bar') ``` Also (but that's just something a bunch of us insisted upon) I prefer the use of `string.format()` to `%`: ``` print("Applying review {}: {}".format(review_id, summary)) ``` (same also below to build `cmd`) support/apply-reviews.py (line 90) <https://reviews.apache.org/r/38705/#comment159501> nit: `if r not in applied:` is more pythonic :) (also, PEP8 doesn't really like 1- and 2-letter variables) support/apply-reviews.py (line 91) <https://reviews.apache.org/r/38705/#comment159502> do you need a counter or something like that, or a flag would suffice? I'm not sure how you use `applied` as a dict (looks like you are just using it as a `set`?): ``` applied = set() for review, summary in reviews: if review not in applied: applied.add(review) print(apply_review(...)) ``` - Marco Massenzio On Oct. 8, 2015, 6:26 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38705/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2015, 6:26 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/38705/diff/ > > > Testing > ------- > > Tested the script with python 2.7. > > > Thanks, > > Artem Harutyunyan > >
