Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 7 months ago by Dan Beam Modified:
7 years, 7 months ago Reviewers:
Isaac (away), M-A Ruel CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, James Hawkins, kareng Visibility:
Public. |
DescriptionChanot branch[0].isdigit()nging --milestone to use JSON source and adding prompt for branch mismatch.
Sample output:
$ drover --merge 196360 --milestone 27
Not all platforms have same branch number for M27.
Here's a list of platforms on each branch:
-(1453): win, ios, cf, mac, linux, android
-(1460): cros
Which branch? ('q' to cancel) 1453
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=198282
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 22
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/14544004/diff/2001/drover.py File drover.py (left): https://codereview.chromium.org/14544004/diff/2001/drover.py#oldcode10 drover.py:10: import string pylint was complaining that this wasn't used (which seems to be right?)
I'm tad busy today, Isaac can you review it and I'll rubberstamp afterward.
ping
pong
Looks good! Some style nits inline. https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py File drover.py (right): https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode378 drover.py:378: OMAHA_PROXY_URL = "http://omahaproxy.appspot.com/all?json=1" can we use https? https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode380 drover.py:380: request = urllib2.urlopen(OMAHA_PROXY_URL) I think this variable would be aptly named 'response', as it is a string of the response. https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode387 drover.py:387: response = json.load(request) nit: change 'response' to variable name that describes the dictionary. https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode389 drover.py:389: branches = {} import collections ... branches = collections.defaultdict(list) ... <remove 400 -402> branches[branch].append(os_version['os']) https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode392 drover.py:392: if not version['true_branch'] or not version['version']: I don't think this check is necessary. not version['true_branch'] will get caught in isdigit on line 398. not version['version'] will get caught in 'not mstone' on line 395. https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode398 drover.py:398: if not branch.isdigit(): this seems brittle, are they guaranteed to be ints? https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode403 drover.py:403: check if branches is empty? https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode404 drover.py:404: if len(branches.keys()) == 1: len(branches) https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode416 drover.py:416: user_input = raw_input("Which branch? ('q' to cancel) ") do you need to strip trailing newline? https://chromiumcodereview.appspot.com/14544004/diff/5002/drover.py#newcode419 drover.py:419: if user_input.lower().startswith('q'): how about also abort on empty input
https://codereview.chromium.org/14544004/diff/5002/drover.py File drover.py (right): https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode378 drover.py:378: OMAHA_PROXY_URL = "http://omahaproxy.appspot.com/all?json=1" On 2013/05/03 10:00:50, Isaac wrote: > can we use https? Done. https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode380 drover.py:380: request = urllib2.urlopen(OMAHA_PROXY_URL) On 2013/05/03 10:00:50, Isaac wrote: > I think this variable would be aptly named 'response', as it is a string of the > response. I've renamed to response, but this is actually a file-like object, not a string, that must be read() by json.load(). maybe you thought I was calling string loading methods, json.loads(), instead (very close)? https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode387 drover.py:387: response = json.load(request) On 2013/05/03 10:00:50, Isaac wrote: > nit: change 'response' to variable name that describes the dictionary. Done. https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode389 drover.py:389: branches = {} On 2013/05/03 10:00:50, Isaac wrote: > import collections > > ... > > branches = collections.defaultdict(list) > > ... > <remove 400 -402> > branches[branch].append(os_version['os']) Done. https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode392 drover.py:392: if not version['true_branch'] or not version['version']: On 2013/05/03 10:00:50, Isaac wrote: > I don't think this check is necessary. > > not version['true_branch'] will get caught in isdigit on line 398. > not version['version'] will get caught in 'not mstone' on line 395. I was getting None type, see below as to why this was popping https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode394 drover.py:394: mstone = version['version'].split('.') if version['version'] is None type this dies https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode398 drover.py:398: if not branch.isdigit(): if version['true_branch'] is None type this dies https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode398 drover.py:398: if not branch.isdigit(): On 2013/05/03 10:00:50, Isaac wrote: > this seems brittle, are they guaranteed to be ints? asked laforge@, didn't get back to me, made a little looser (only branch[0].isdigit()), this is to omit 'trunk' https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode403 drover.py:403: On 2013/05/03 10:00:50, Isaac wrote: > check if branches is empty? Done. https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode404 drover.py:404: if len(branches.keys()) == 1: On 2013/05/03 10:00:50, Isaac wrote: > len(branches) Done. https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode416 drover.py:416: user_input = raw_input("Which branch? ('q' to cancel) ") On 2013/05/03 10:00:50, Isaac wrote: > do you need to strip trailing newline? Done. (no, I didn't need to in testing but it doesn't hurt) https://codereview.chromium.org/14544004/diff/5002/drover.py#newcode419 drover.py:419: if user_input.lower().startswith('q'): On 2013/05/03 10:00:50, Isaac wrote: > how about also abort on empty input I dislike this behavior as if you've pressed enter while a script is running this will likely automatically fail here.
lgtm once Isaac approves. https://codereview.chromium.org/14544004/diff/12001/drover.py File drover.py (right): https://codereview.chromium.org/14544004/diff/12001/drover.py#newcode410 drover.py:410: print >> sys.stderr, """ I'm not a fan of """ mid-function. It definitely confuses my text editor. Please use explicit \n. https://codereview.chromium.org/14544004/diff/12001/drover.py#newcode419 drover.py:419: if user_input in branches.keys(): if user_input in branches:
https://codereview.chromium.org/14544004/diff/12001/drover.py File drover.py (right): https://codereview.chromium.org/14544004/diff/12001/drover.py#newcode410 drover.py:410: print >> sys.stderr, """ On 2013/05/03 17:19:52, Marc-Antoine Ruel wrote: > I'm not a fan of """ mid-function. It definitely confuses my text editor. Please > use explicit \n. Done. https://codereview.chromium.org/14544004/diff/12001/drover.py#newcode419 drover.py:419: if user_input in branches.keys(): On 2013/05/03 17:19:52, Marc-Antoine Ruel wrote: > if user_input in branches: Done.
lgtm thanks for fixes. https://chromiumcodereview.appspot.com/14544004/diff/20001/drover.py File drover.py (right): https://chromiumcodereview.appspot.com/14544004/diff/20001/drover.py#newcode396 drover.py:396: if not mstone or mstone[0] != str(milestone): if we're keeping line 393, 'not mstone' is redundant with not version['version']. split() on non-empty str always returns at least one entry.
https://chromiumcodereview.appspot.com/14544004/diff/20001/drover.py File drover.py (right): https://chromiumcodereview.appspot.com/14544004/diff/20001/drover.py#newcode396 drover.py:396: if not mstone or mstone[0] != str(milestone): On 2013/05/04 02:27:45, Isaac wrote: > if we're keeping line 393, 'not mstone' is redundant with not > version['version']. split() on non-empty str always returns at least one entry. > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/14544004/24001
Message was sent while issue was closed.
Change committed as 198282 |
