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