Split generic subcommand code off its own module.

Split generic subcommand code off its own module.

Use the code in git_cl.py, since it was the more evolved. Add documentation
and clean up the structure along the way.

This makes it possible to easily reuse the generic subcommand handling code.

As a first step, only git_cl.py is using it. Eventually, gclient and gcl could
be switch over.

R=iannucci@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=218072

[M-A Ruel, 2013-08-15 16:30:23]

I'm doing it so I can reuse it for isolate.py. I wanted to do this for a long
time but was too lazy. :)

[iannucci, 2013-08-15 20:46:03]

https://codereview.chromium.org/23250002/diff/14001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode41
subcommand.py:41: MAIN_MODULE = sys.modules['__main__']
Hm.. I don't like this (esp. modifying it in the test).

Why not put the functionality into a class, and then you can just pass the class
the module to handle. Then you don't need to rely on magic globals.

You could even have the default module value for the class be
sys.modules['__main__']

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode49
subcommand.py:49: return hook
I've actually always wondered why this doesn't just the docstring of the
function (or maybe just the first line). Seems redundant.

In any event you should probably also use functools.wraps

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode55
subcommand.py:55: The commands must be in the '__main__' modules. To import a
command from a
I think some of these docstrings need to be updated a bit too.

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode72
subcommand.py:72: def get_command(name):
Hm... I would expect this to be equivalent to get_commands()[name], but it has
some additional magic. Maybe the function name needs to be clarified a bit?

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode124
subcommand.py:124: parser.description = sys.modules['__main__'].__doc__.strip()
+ '\n'
This shouldn't be sys.modules['__main__'], I don't think?

[M-A Ruel, 2013-08-15 20:56:55]

https://codereview.chromium.org/23250002/diff/14001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode41
subcommand.py:41: MAIN_MODULE = sys.modules['__main__']
On 2013/08/15 20:46:04, iannucci wrote:
> Hm.. I don't like this (esp. modifying it in the test).
> 
> Why not put the functionality into a class, and then you can just pass the
class
> the module to handle. Then you don't need to rely on magic globals.
> 
> You could even have the default module value for the class be
> sys.modules['__main__']

I'll prototype this and will reply.

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode49
subcommand.py:49: return hook
On 2013/08/15 20:46:04, iannucci wrote:
> I've actually always wondered why this doesn't just the docstring of the
> function (or maybe just the first line). Seems redundant.
> 
> In any event you should probably also use functools.wraps

wraps() is not applicable because it's not a hook, it's modifying the function
itself.

It's not in the docstring because it's a separate OptionParser item,
parser.usage != parser.description. The usage is printed first.

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode72
subcommand.py:72: def get_command(name):
On 2013/08/15 20:46:04, iannucci wrote:
> Hm... I would expect this to be equivalent to get_commands()[name], but it has
> some additional magic. Maybe the function name needs to be clarified a bit?

That was ripped directly from git_cl.py The docstring explains what it does. And
the function name says it gets a command. What's your suggestion?

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode124
subcommand.py:124: parser.description = sys.modules['__main__'].__doc__.strip()
+ '\n'
On 2013/08/15 20:46:04, iannucci wrote:
> This shouldn't be sys.modules['__main__'], I don't think?

Exact, fixing.

[M-A Ruel, 2013-08-15 21:08:27]

https://codereview.chromium.org/23250002/diff/19001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/19001/subcommand.py#newcode65
subcommand.py:65: return sys.modules.get(self.module_name) or
sys.modules.get('__main__')
This is tricky because:
- 'git_cl' is not present when running git-cl.
- It must be 'git_cl' when running the test.

So it's not less obscure than the global variable technique IMHO.

[iannucci, 2013-08-15 21:18:52]

https://codereview.chromium.org/23250002/diff/19001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/19001/subcommand.py#newcode61
subcommand.py:61: self.module_name = module
Why not

def __init__(self, module=None):
  self.module = module or sys.modules['__main__']

?

Then the test can import and pass in git_cl, and inside git_cl it'll just have a
reference to the main module?

[iannucci, 2013-08-15 21:23:22]

https://codereview.chromium.org/23250002/diff/14001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode49
subcommand.py:49: return hook
On 2013/08/15 20:56:55, M-A Ruel wrote:
> On 2013/08/15 20:46:04, iannucci wrote:
> > I've actually always wondered why this doesn't just the docstring of the
> > function (or maybe just the first line). Seems redundant.
> > 
> > In any event you should probably also use functools.wraps
> 
> wraps() is not applicable because it's not a hook, it's modifying the function
> itself.

Ah right.

> 
> It's not in the docstring because it's a separate OptionParser item,
> parser.usage != parser.description. The usage is printed first.

Right I see that, I'm just meaning that maybe there could just be a convention
of

def CMDthingy(...):
  """"[lots of things]
  
  This runs the thingy command on |lots of things|
  """"
  ...

I guess I'm +-0 on this though :)


Also, if you're going to require the @usage decorator and modify the function,
why not just look for [x for x in module.__dict__.values() if hasattr(x,
'usage_more')] instead of the weird CMD name hacking?

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode72
subcommand.py:72: def get_command(name):
On 2013/08/15 20:56:55, M-A Ruel wrote:
> On 2013/08/15 20:46:04, iannucci wrote:
> > Hm... I would expect this to be equivalent to get_commands()[name], but it
has
> > some additional magic. Maybe the function name needs to be clarified a bit?
> 
> That was ripped directly from git_cl.py The docstring explains what it does.
And
> the function name says it gets a command. What's your suggestion?

maybe find_closest_command?

[M-A Ruel, 2013-08-16 13:47:20]

On 2013/08/15 21:23:22, iannucci wrote:
> Also, if you're going to require the @usage decorator and modify the function,
> why not just look for [x for x in module.__dict__.values() if hasattr(x,
> 'usage_more')] instead of the weird CMD name hacking?

@usage is not required at all. For example, see CMDbaseurl or CMDstatus.

I also like enforcing the CMD prefix, it makes it self-descriptive and easier to
scan for commands in a longer script.


Also note about the module issue;
$ cat foo.py
import sys
bar = 'allo'
def try_print(name):
  if name in sys.modules:
    print('%s.bar = %s' %
        (name, getattr(sys.modules[name], 'bar', 'NOT DEFINED')))
  else:
    print('%s is not reachable' % name)
try_print('foo')
try_print('__main__')
try_print(__name__)

$ cat bar.py
from foo import try_print
print('From bar now.')
try_print('foo')
try_print('__main__')
try_print(__name__)

$ python foo.py
foo is not reachable
__main__.bar = allo
__main__.bar = allo

$ python bar.py
foo.bar = allo
__main__.bar = NOT DEFINED
foo.bar = allo
From bar now.
foo.bar = allo
__main__.bar = NOT DEFINED
__main__.bar = NOT DEFINED


In particular, foo is not reachable from within foo but it is if imported from
bar, i.e. the first line of each output.
__main__ will always point to the main module, except when running python
interactively where it's not defined.

So I can't use 'git_cl' from within git_cl and I can't use __main__ from the
unit test. As such, I think the "global variable hack" wasn't this bad after
all; it was much more obvious what was happening. Now, one has to read really
carefully to understand the trick being done. I think it obscured the code more
than anything else.

https://codereview.chromium.org/23250002/diff/14001/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode72
subcommand.py:72: def get_command(name):
On 2013/08/15 21:23:22, iannucci wrote:
> maybe find_closest_command?

Done.

[M-A Ruel, 2013-08-16 13:51:11]

On 2013/08/16 13:47:20, M-A Ruel wrote:
> On 2013/08/15 21:23:22, iannucci wrote:
> > Also, if you're going to require the @usage decorator and modify the
function,
> > why not just look for [x for x in module.__dict__.values() if hasattr(x,
> > 'usage_more')] instead of the weird CMD name hacking?
> 
> @usage is not required at all. For example, see CMDbaseurl or CMDstatus.
> 
> I also like enforcing the CMD prefix, it makes it self-descriptive and easier
to
> scan for commands in a longer script.
> 
> 
> Also note about the module issue;
> $ cat foo.py
> import sys
> bar = 'allo'
> def try_print(name):
>   if name in sys.modules:
>     print('%s.bar = %s' %
>         (name, getattr(sys.modules[name], 'bar', 'NOT DEFINED')))
>   else:
>     print('%s is not reachable' % name)
> try_print('foo')
> try_print('__main__')
> try_print(__name__)
> 
> $ cat bar.py
> from foo import try_print
> print('From bar now.')
> try_print('foo')
> try_print('__main__')
> try_print(__name__)
> 
> $ python foo.py
> foo is not reachable
> __main__.bar = allo
> __main__.bar = allo
> 
> $ python bar.py
> foo.bar = allo
> __main__.bar = NOT DEFINED
> foo.bar = allo
> From bar now.
> foo.bar = allo
> __main__.bar = NOT DEFINED
> __main__.bar = NOT DEFINED
> 
> 
> In particular, foo is not reachable from within foo but it is if imported from
> bar, i.e. the first line of each output.
> __main__ will always point to the main module, except when running python
> interactively where it's not defined.
> 
> So I can't use 'git_cl' from within git_cl and I can't use __main__ from the
> unit test. As such, I think the "global variable hack" wasn't this bad after
> all; it was much more obvious what was happening. Now, one has to read really
> carefully to understand the trick being done. I think it obscured the code
more
> than anything else.
> 
> https://codereview.chromium.org/23250002/diff/14001/subcommand.py
> File subcommand.py (right):
> 
> https://codereview.chromium.org/23250002/diff/14001/subcommand.py#newcode72
> subcommand.py:72: def get_command(name):
> On 2013/08/15 21:23:22, iannucci wrote:
> > maybe find_closest_command?
> 
> Done.

[M-A Ruel, 2013-08-16 14:00:15]

Ok, I'm silly. Improved documentation to alert that __name__ must always be
used. This fixes all the problems. Removed the fallback to __main__. See
patchset 8.

[iannucci, 2013-08-16 19:53:06]

Ok, this lgtm :)

https://codereview.chromium.org/23250002/diff/34003/subcommand.py
File subcommand.py (right):

https://codereview.chromium.org/23250002/diff/34003/subcommand.py#newcode73
subcommand.py:73: itself with 'script'. __name__ always match the right value.
Ah, good call. This works :)

[commit-bot: I haz the power, 2013-08-16 20:04:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/23250002/34003

[commit-bot: I haz the power, 2013-08-16 20:06:19]

Change committed as 218072