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

Reply via email to