Changing --milestone to use JSON source and adding prompt for branch mismatch.

Chanot 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

[Dan Beam, 2013-04-29 17:40:18]

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

[M-A Ruel, 2013-04-29 18:01:33]

I'm tad busy today, Isaac can you review it and I'll rubberstamp afterward.

[Dan Beam, 2013-05-02 05:19:37]

ping

[Dan Beam, 2013-05-03 09:27:31]

pong

[Isaac (away), 2013-05-03 10:00:50]

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

[Dan Beam, 2013-05-03 17:05:43]

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.

[M-A Ruel, 2013-05-03 17:19:52]

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:

[Dan Beam, 2013-05-03 18:36:17]

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.

[Isaac (away), 2013-05-04 02:27:45]

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.

[Dan Beam, 2013-05-04 04:11:34]

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.

[commit-bot: I haz the power, 2013-05-04 04:12:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14544004/24001

[commit-bot: I haz the power, 2013-05-04 04:14:03]

Change committed as 198282