> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > 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!

`sh` is indeed awesome, however it's not available by default, and I'd like to 
stick to stock modules only.


> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 40
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line40>
> >
> >     (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
> >     ```

In both cases (if it's not a `200` code, or if it's not a JSON) I want to 
terminate, and that's exactly what the script will do for me. Since the 
intended audience are developers I am willingly frugal when it comes to writing 
code, and tend to resort to stack traces wherever I can. What do you think?


> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 91
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line91>
> >
> >     ```
> >     from __future__ import print_function:
> >     
> >     ...
> >     
> >     print(output)
> >     ```
> >     (this way it'll all work also for those us using a modern Python 
> > interpreter :) )
> >     
> >     thanks!

Other scripts in the repo seem to be developed for Python 2.x, and I'd like to 
stay consistent.


> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 102
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line102>
> >
> >     consider using the `sh` module:
> >     ```
> >     import sh
> >     
> >     cmd = "{rev_id}.patch".format(rev_id=review_id)
> >     sh.rm("-f", cmd)
> >     
> >     ```

`sh` does not seem to be a stock module.


> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 106
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line106>
> >
> >     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
> >     """
> >     ```

Added a comment in main function.


- Artem


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
-----------------------------------------------------------


On Oct. 16, 2015, 12:03 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2015, 12:03 a.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