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 38705: Added support for applying a review chain (apply-reviews.py).
Date Fri, 16 Oct 2015 06:50:20 GMT


> On Oct. 14, 2015, 5:55 a.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.

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


> On Oct. 14, 2015, 5:55 a.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?

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?


> On Oct. 14, 2015, 5:55 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 24
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line24>
> >
> >     if you are doing this often enough, it's much better to compile this to a constant
Pattern and then use that instead.

It's done only once per review, so compiling will not give us any significant performance
increase.


> On Oct. 14, 2015, 5:55 a.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`?

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


> On Oct. 14, 2015, 5:55 a.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`)

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.


- Artem


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


On Oct. 8, 2015, 11:26 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2015, 11:26 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
> 
>


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