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