This is an automatically generated e-mail. To reply, visit:

support/apply-reviews.py (line 7)

    uhm... could we use `requests` instead?
    much more modern API and widespread use.

support/apply-reviews.py (line 11)

    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)

    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)

    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)

    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)

    you should not call a param with the name of a type (`list` is a type in 
    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?
    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)

    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)

    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)

    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)

    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)

    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:

- 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

Reply via email to