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

Reply via email to