Closed Bug 951733 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: Mach Core, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: jgraham, Assigned: jgraham)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch bug951733.diff (obsolete) — Splinter Review
Attachment #8349532 - Flags: review?(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.
Attachment #8349532 - Flags: review?(gps) → review+
(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.
Attached patch bug951733.diff (obsolete) — Splinter Review
New patch which fixes review issues.
Attachment #8349532 - Attachment is obsolete: true
Attachment #8349983 - Flags: review?(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 on attachment 8349983 [details] [diff] [review]
bug951733.diff

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

Ship it!
Attachment #8349983 - Flags: review?(gps) → review+
Attachment #8349983 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d53d1c6cdf13
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Blocks: 1043524
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.