> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 7
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line7>
> >
> >     uhm... could we use `requests` instead?
> >     much more modern API and widespread use.
> 
> Artem Harutyunyan wrote:
>     `requests` looks great, but it looks like it requires to be installed 
> separately. I would really like to restrict myself to only the modules 
> available with stock python distribution. Ditto for `sh`.

right, agree.
however, as I remarked in the other review, it would not be a major hurdle to 
get folks to pip install those two.

we demand they install a ton-worth of C++ and other assorted libraries, so 
asking to create a virtualenv and/or run `pip install sh requests` does not 
appear to me to be huge demand :)

(also, appy to write a 'checker' that can be run prior to these scripts and 
does the needful)


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 19
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line19>
> >
> >     you should check for a non 4xx/5xx return code (or just check for a 2xx 
> > if you know for sure what the API returns).
> >     
> >     as you expect JSON, you should probably specify that in the 
> > `Accept-content` header too?
> 
> Artem Harutyunyan wrote:
>     urllib2 throws an exception if an error occurs, which forces the program 
> to terminate. Termination is the right thing to do here, because there is no 
> recovery path, so I would only want to catch the exception to print a pretty 
> message, but the stack trace should be informative enough for the developer 
> who is running this script. 
>     As for the `content-type`, ReviewBoard is setting it to something 
> unorthodox (`Content-Type: 
> application/vnd.reviewboard.org.review-request+json`), do you think we should 
> verify against that specific value?

uh? really? fascinating...
what I was suggesting, though, was in the **Request** (asking the server to 
send you back JSON) in which case you either do get JSON (and don't need to 
check the `Content-Type`) or get a 415.

If you don't tell the server, then, yes, you have to check what the guy threw 
at you, as it's not in principle possible to know in advance what the default 
mimetype is.


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 45
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line45>
> >
> >     is this right?
> >     you return after the first iteration?
> >     
> >     if so, why not just getting the first item in `depends_on`?
> 
> Artem Harutyunyan wrote:
>     I think this is right. That statement is after the recursive call, so 
> actually when in gets there for the last time the lisst contains the entire 
> chain (whereas `depends_on` only contains immediate anchestor(s)).

weird.
not sure I completely like that - but I'll take your word for it that it works 
:)

do you mind terribly to add a comment, so we don't risk someone "fix it" by 
unindenting it sometime in the future?


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 56
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line56>
> >
> >     if you do this, this script will be probably useful to folks who use 
> > Python 3 too :)
> >     
> >     ```
> >     from __future__ import print_function
> >     
> >     ...
> >     
> >     print('foo bar')
> >     ```
> >     Also (but that's just something a bunch of us insisted upon) I prefer 
> > the use of `string.format()` to `%`:
> >     ```
> >     print("Applying review {}: {}".format(review_id, summary))
> >     ```
> >     (same also below to build `cmd`)
> 
> Artem Harutyunyan wrote:
>     I am not sure whether we should use python 3. Other python scripts in 
> Mesos repo seem to be written for 2.x versions, so I'd like to stay 
> consistent.

This works with Python 2.7 too (that's the point of the `import __future__`)


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101980
-----------------------------------------------------------


On Oct. 16, 2015, 6:50 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2015, 6:50 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/38705/diff/
> 
> 
> Testing
> -------
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Reply via email to