Plumb in subcommand options

Plumb in subcommand options

Enable passing of options to subcommands by avoiding any parsing of these
at the top level.

BUG=chromium-os:11785
TEST=chromite build --clean x86-generic, check it does clean
python main_unittest.py  (run tests)

Change-Id: I4bfe86bc02d12aa207a294d143201e20875fba03

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=f5298ef

[sjg, 2011-04-04 18:20:33]

I have plumbed in the subcommand options in this CL, just as a clean up after
the last one. Next CLs do scanning for available subcmds and a bit of help
clean-up, so wanted this out of the way first.

[diandersAtChromium, 2011-04-04 20:42:48]

I don't think this code will work in all cases.  Can you try the test case and
let me know if I've understood things incorrectly?

http://codereview.chromium.org/6720024/diff/1/shell/main.py
File shell/main.py (right):

http://codereview.chromium.org/6720024/diff/1/shell/main.py#newcode142
shell/main.py:142: # We want to parse only the options that we understand at the
top level of
This code won't work, will it?  Try:
  chromite -v 3 -q --chroot myChroot build --clean x86-generic

Your code will see lots of "positional" arguments and will get confused.

http://codereview.chromium.org/6720024/diff/1/shell/main.py#newcode146
shell/main.py:146: # FIXME(sjg): Revisit this to include tolerant option parser
instead
nit: please use TODO.

http://codereview.chromium.org/6720024/diff/1/shell/main.py#newcode153
shell/main.py:153: # Option arguments are collected as it, but we only allow two
positional
nit: "as is"

[sjg, 2011-04-04 22:31:58]

PTAL I may have a simpler solution.

[diandersAtChromium, 2011-04-08 00:43:46]

I think it can be a little cleaner still...

http://codereview.chromium.org/6720024/diff/5001/shell/main.py
File shell/main.py (right):

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode142
shell/main.py:142: # We want to parse only the options that we understand at the
top level of
Maybe put this whole chunk of parsing code in a helper function?

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode151
shell/main.py:151: for arg in sys.argv:
This loop keeps confusing me the way it's written.  Perhaps:

cmd_args = list(sys.argv)
while (positional < 2) and cmd_args:
  arg = cmd_args.pop(0)
  if not arg.startswith('-'):
    positional += 1
  our_args.append(arg)

...I think that makes things roughly like the below "pull a few more off" case,
too, which is a bonus.  Maybe you can just always do a single 'while not
cmd_str' loop?

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode175
shell/main.py:175: arg = cmd_args[0]
arg = cmd_args.pop(0)

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode178
shell/main.py:178: if arg[0] != '-':
startswith

[sjg, 2011-04-08 20:25:26]

Thanks Doug, PTAL

http://codereview.chromium.org/6720024/diff/5001/shell/main.py
File shell/main.py (right):

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode142
shell/main.py:142: # We want to parse only the options that we understand at the
top level of
On 2011/04/08 00:43:46, diandersAtChromium wrote:
> Maybe put this whole chunk of parsing code in a helper function?

done - added a few tests also.

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode151
shell/main.py:151: for arg in sys.argv:
On 2011/04/08 00:43:46, diandersAtChromium wrote:
> This loop keeps confusing me the way it's written.  Perhaps:
> 
> cmd_args = list(sys.argv)
> while (positional < 2) and cmd_args:
>   arg = cmd_args.pop(0)
>   if not arg.startswith('-'):
>     positional += 1
>   our_args.append(arg)
> 
> ...I think that makes things roughly like the below "pull a few more off"
case,
> too, which is a bonus.  Maybe you can just always do a single 'while not
> cmd_str' loop?

Yes, and in fact this inspired me to simplify it a bit more. The turd is getting
pretty shiny now.

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode175
shell/main.py:175: arg = cmd_args[0]
On 2011/04/08 00:43:46, diandersAtChromium wrote:
> arg = cmd_args.pop(0)

done

http://codereview.chromium.org/6720024/diff/5001/shell/main.py#newcode178
shell/main.py:178: if arg[0] != '-':
On 2011/04/08 00:43:46, diandersAtChromium wrote:
> startswith

done

[diandersAtChromium, 2011-04-08 20:35:41]

LGTM w/ last nit.  Ooooo.  Shiny!

http://codereview.chromium.org/6720024/diff/9001/shell/main.py
File shell/main.py (right):

http://codereview.chromium.org/6720024/diff/9001/shell/main.py#newcode96
shell/main.py:96: # Helper function to parse arguments and split into one we
understand, and
Last nit: can you make this a docstring instead of a header comment?

[sjg, 2011-04-08 21:00:19]

http://codereview.chromium.org/6720024/diff/9001/shell/main.py
File shell/main.py (right):

http://codereview.chromium.org/6720024/diff/9001/shell/main.py#newcode96
shell/main.py:96: # Helper function to parse arguments and split into one we
understand, and
On 2011/04/08 20:35:41, diandersAtChromium wrote:
> Last nit: can you make this a docstring instead of a header comment?

doh.