mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
Date Fri, 16 Oct 2015 16:15:27 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message