951733 - Support passing an existing argparse.ArgumentParser to mach in order to populate the command arguments

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[James Graham [:jgraham]]

For tools that can be run through mach or directly, writing a lot of CommandArgument decorators isn't much fun since it's basically just code duplication. It should be possible instead to pass an existing argparse parser to mach and have it use that for the arguments.

Comment 1

[James Graham [:jgraham]]

Attachment: bug951733.diff

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8349532 [details] [diff] [review]
bug951733.diff

Review of attachment 8349532 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice! Just minor nits on the review.

Some tests would be nice. But I have a feeling you're about to provide a command that uses this feature. Plus, mach's unit tests aren't easy to run ATM. (Need to fix that.)

A good followup would be to have parser accept a callable that returns an argument parser. A variation of that is a callable that accepts an empty ArgumentParser that it populates. But these can be followups. Adding "TODO" or "FUTURE" comments in the code might be worthwhile.

::: python/mach/mach/base.py
@@ +80,5 @@
>          # in a given context.
>          'conditions',
>  
> +        #argparse.ArgumentParser instance to use as the basis for command
> +        #arguments.

Nit: Please add a space after #.

::: python/mach/mach/dispatcher.py
@@ +222,5 @@
> +        if handler.parser:
> +            c_parser = handler.parser
> +            c_parser.formatter_class = NoUsageFormatter
> +            #This renames the "optional arguments" group to "Command Arguments"
> +            c_parser._action_groups[1].title = 'Command Arguments'

What is the [1] here? Could it even not be 1? Please document this.

Comment 3

[James Graham [:jgraham]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8349532 [details] [diff] [review]
> bug951733.diff
> 
> Review of attachment 8349532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice! Just minor nits on the review.
> 
> Some tests would be nice. But I have a feeling you're about to provide a
> command that uses this feature. Plus, mach's unit tests aren't easy to run
> ATM. (Need to fix that.)

I am happy to write tests, but I'm not really sure what to test for this case. I couldn't see existing tests for the argument parsing stuff, but maybe I missed something.

> A good followup would be to have parser accept a callable that returns an
> argument parser. A variation of that is a callable that accepts an empty
> ArgumentParser that it populates. But these can be followups. Adding "TODO"
> or "FUTURE" comments in the code might be worthwhile.

So these wouldn't be useful for my use case, and might not be useful for many cases because the mach frontend is able to default some arguments (e.g. the location of the firefox binary) that need to be provided explicitly when the script is called directly. So I have to pass a parameter to the argparse-creating function to decide whether mandatory arguments are required. If we think this will be the common case then the other features like passing a callable don't make much sense. If we don't think that then they make a lot more sense.

> ::: python/mach/mach/base.py
> @@ +80,5 @@
> >          # in a given context.
> >          'conditions',
> >  
> > +        #argparse.ArgumentParser instance to use as the basis for command
> > +        #arguments.
> 
> Nit: Please add a space after #.
> 
> ::: python/mach/mach/dispatcher.py
> @@ +222,5 @@
> > +        if handler.parser:
> > +            c_parser = handler.parser
> > +            c_parser.formatter_class = NoUsageFormatter
> > +            #This renames the "optional arguments" group to "Command Arguments"
> > +            c_parser._action_groups[1].title = 'Command Arguments'
> 
> What is the [1] here? Could it even not be 1? Please document this.

Fixed these in the forthcoming patch.

Comment 4

[James Graham [:jgraham]]

Attachment: bug951733.diff

New patch which fixes review issues.

Comment 5

[Gregory Szorc [:gps]]

(In reply to James Graham [:jgraham] from comment #3)
> > Some tests would be nice. But I have a feeling you're about to provide a
> > command that uses this feature. Plus, mach's unit tests aren't easy to run
> > ATM. (Need to fix that.)
> 
> I am happy to write tests, but I'm not really sure what to test for this
> case. I couldn't see existing tests for the argument parsing stuff, but
> maybe I missed something.

Don't worry about writing tests.

> > A good followup would be to have parser accept a callable that returns an
> > argument parser. A variation of that is a callable that accepts an empty
> > ArgumentParser that it populates. But these can be followups. Adding "TODO"
> > or "FUTURE" comments in the code might be worthwhile.
> 
> So these wouldn't be useful for my use case, and might not be useful for
> many cases because the mach frontend is able to default some arguments (e.g.
> the location of the firefox binary) that need to be provided explicitly when
> the script is called directly. So I have to pass a parameter to the
> argparse-creating function to decide whether mandatory arguments are
> required. If we think this will be the common case then the other features
> like passing a callable don't make much sense. If we don't think that then
> they make a lot more sense.

The reason I suggested it is there are lots of libraries that expose a "get_argparser()" function or similar. For performance reasons, it's better to defer calling that function until dispatch time so unrelated commands don't negatively impact mach's startup/dispatch time. I suppose we can cross this bridge if it becomes an issues.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8349983 [details] [diff] [review]
bug951733.diff

Review of attachment 8349983 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

Comment 7

[James Graham [:jgraham]]

Attachment: Patch with correct commit message.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53d1c6cdf13

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d53d1c6cdf13