> On Oct. 14, 2015, 4:45 p.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 > > ``` > > Artem Harutyunyan wrote: > 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?
I don't like relying on stack traces (and Python's aren't particularly useful) - even as a developer, the message I derive is "the code is buggy" *not* something went wrong with the service. Providing a LOG(ERROR) with an explanation of whent went wrong would be a much better experience (and, in this specific case, I bet the stacktrace would be something along the lines of "Unknown JSON content" or something equally misleading). > On Oct. 14, 2015, 4:45 p.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! > > Artem Harutyunyan wrote: > Other scripts in the repo seem to be developed for Python 2.x, and I'd > like to stay consistent. that being exactly my point: what I suggested is consistent with 2.7, but can be used with 3.4 without any change. have your cake and eat it, too :D (and it's only good to be consistent with something that's good ;-) ) > On Oct. 14, 2015, 4:45 p.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) > > > > ``` > > Artem Harutyunyan wrote: > `sh` does not seem to be a stock module. good point! Although really easy to fix: ``` try: import sh except ImportError: print("This script requires the use of the `sh` module; please run: `sudo pip install sh`") exit(1) ``` (especially, as you remarked above, that users are developers, so shouldn't be afraid of doing this) If we want to be sophisticated, we could even have a `requirements.txt` that we'd run during the ./bootstrap script or document developers to run once. `pip install --upgrade -r requirements.txt` - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38883/#review102624 ----------------------------------------------------------- On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38883/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2015, 3:29 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 > >
