mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 38705: Added support for applying a review chain (apply-reviews.py).
Date Wed, 14 Oct 2015 12:55:23 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message