mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Artem Harutyunyan" <ar...@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 07:04:02 GMT


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


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