----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39410/#review103208 -----------------------------------------------------------
support/apply-reviews.py (line 23) <https://reviews.apache.org/r/39410/#comment161177> hey, as mentioned, could you please remove whitespace around `=` in args call (and default params) here and everywhere. thanks! support/apply-reviews.py (line 27) <https://reviews.apache.org/r/39410/#comment161178> is this right? by looking at the args names, looks like it returns the URL of a PR given its number? either the comment or the args names are wrong. also, I would much prefer `pull_request_number` support/apply-reviews.py (line 44) <https://reviews.apache.org/r/39410/#comment161179> could you please add an @param and explain what `options` is and what does it look like? (also an @type would be awesome) support/apply-reviews.py (lines 46 - 47) <https://reviews.apache.org/r/39410/#comment161181> is this dead code? please remove. support/apply-reviews.py (line 48) <https://reviews.apache.org/r/39410/#comment161180> this will cause a KeyError if `reveiw_id` is not there. you can achieve the same result (but better) with: ``` if options.get('review_id'): ``` (this also protects you further below in the call to `format`) support/apply-reviews.py (line 97) <https://reviews.apache.org/r/39410/#comment161182> ``` if not options or 'dry_run' not in options: ``` support/apply-reviews.py (line 103) <https://reviews.apache.org/r/39410/#comment161183> it would be really nice if we could make our code work under both 2.7 and 3.x Python ;) support/apply-reviews.py (line 133) <https://reviews.apache.org/r/39410/#comment161184> this line exceeds 100 char (I'm almost sure) you can break it using \ or, even better, build the command via string.join(): ``` ' '.join(['wget', '--no-check-certificate', '--no-verbose', '-O {}.patch'.format(patch_id(options)), patch_url(options)]) ``` support/apply-reviews.py (line 136) <https://reviews.apache.org/r/39410/#comment161185> s/beacuse/because support/apply-reviews.py (line 145) <https://reviews.apache.org/r/39410/#comment161186> unnecessary parentheses; also, please use `get()` instead of `[]` (unless you are terminally positive both keys are there every time) support/apply-reviews.py (line 156) <https://reviews.apache.org/r/39410/#comment161187> you don't really need to escape the quotes, just take advantage of Python being able to use single and double quotes interchangeably (or even use """ if you really want to be hip :) support/apply-reviews.py (lines 179 - 181) <https://reviews.apache.org/r/39410/#comment161188> this looks a bit ugly - prefer: ``` message = '{summary}\n\n{description}\n\nThis closes: {pr}'. format(summary=title, description=description, pr=pr_number) ``` support/apply-reviews.py (lines 184 - 188) <https://reviews.apache.org/r/39410/#comment161189> no space before `:` (also, it's more "pythonic" to use double quotes for the dict's keys - but really a micro-nit!) support/apply-reviews.py (lines 200 - 205) <https://reviews.apache.org/r/39410/#comment161191> could you please reformat this code to something more readable? ``` username = review.get('links').get('submitter').get('title') user = url_to_json(user_url(username)).get('user') url = review_url(rid) author = '{author} {email}'.format(author=user.get('fullname'), email=user.get('email')) message = '\n\n'.join([review.get('summary'), review.get('description'), 'Review: {}'.format(url) ``` support/apply-reviews.py (lines 251 - 255) <https://reviews.apache.org/r/39410/#comment161192> Much better: ``` options = { 'review_id': args.review_id, 'dry_run': args.dry_run, 'no_amend': args.no_amend, 'github': args.github } ``` or if you really want to be pythonic: ``` options = dict() for k in ['review_id', 'dry_run', 'no_amend', 'github']: options[k] = args.getattr(k) ``` although, I'm not sure right now if this will work with your having made `github` and `review_id` mutually exclusive (but, then again, the dotted notation should blow up too if the arg is not there?) support/apply-reviews.py (lines 265 - 270) <https://reviews.apache.org/r/39410/#comment161193> this code look familiar and I remember already commenting about `applied` :) - Marco Massenzio On Oct. 18, 2015, 10:32 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39410/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2015, 10:32 p.m.) > > > Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco > Massenzio, and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > ------- > > Added support for github to apply-reviews.py. > > > Diffs > ----- > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39410/diff/ > > > Testing > ------- > > Tested with python 2.7 > > > Thanks, > > Artem Harutyunyan > >
