Opened 8 months ago

Closed 4 months ago

#29152 closed Cleanup/optimization (fixed)

Allow more control over ArgumentParser initialization in management commands

Reported by: Dmitry Owned by: humbertotm
Component: Core (Management commands) Version: 2.0
Severity: Normal Keywords:
Cc: Tom Forbes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello everyone.
Noticed today that there is no way to customize CommandParser (ArgumentParser subclass), initialized in management commands (https://github.com/django/django/blob/master/django/core/management/base.py#L227). There is no option (or i was unable to find it - please correct me in this case) to pass any kwargs in CommandParser constructor. The public method create_parser contains some django-related hard-coded add_arguments calls, so overriding it directly leads to some copy paste.

I needed to use some custom formatter for ArgumentParser (https://docs.python.org/3/library/argparse.html?highlight=argparse#argparse.ArgumentDefaultsHelpFormatter) to add default values to help page, but was unable to do this properly. There is also some other usefull keyword arguments available in ArgumentParser.

As a proposal a new method could be added to BaseCommand (let's say BaseCommand.get_parser_instance), for example like this:

def get_parser_instance(self):
        return CommandParser(
            self, prog="%s %s" % (os.path.basename(prog_name), subcommand),
            description=self.help or None,
        )

def create_parser(self, prog_name, subcommand)::
       parser = self.get_parser_instance()
       ... etc

This should not break anything in BaseCommand, but will allow have more control on parser's creation overriding just get_parser_instance method.
Please sorry for mistakes.

Change History (15)

comment:1 Changed 8 months ago by Dmitry

Summary: management commands rigid ArgumentParser initializationmanagement commands - more control on ArgumentParser initialization

comment:2 Changed 8 months ago by Dmitry

Type: New featureCleanup/optimization

comment:3 Changed 8 months ago by Tim Graham

Summary: management commands - more control on ArgumentParser initializationAllow more control over ArgumentParser initialization in management commands
Triage Stage: UnreviewedAccepted

Would a hook to provide some additional keyword arguments be sufficient?

comment:4 in reply to:  3 Changed 8 months ago by Dmitry

Replying to Tim Graham:

Would a hook to provide some additional keyword arguments be sufficient?

Yeah, this will be great, maybe the better way. I was thinking about way to implement this, but was unable to find beautiful enough way to add such hook. Maybe you have some good ideas.

As a reference my idea was something like this:

  1. Add parser_kwargs argument in BaseCommand.init, defaults to {}
            def __init__(self, stdout=None, stderr=None, no_color=False, parser_kwargs=None):
            self.stdout = OutputWrapper(stdout or sys.stdout)
            self.stderr = OutputWrapper(stderr or sys.stderr)
            if no_color:
                self.style = no_style()
            else:
                self.style = color_style()
                self.stderr.style_func = self.style.ERROR
           if not parser_kwargs:
               parser_kwargs = {}
    
  1. use parser_kwargs in create_parser method
        def create_parser(self, prog_name, subcommand):
            """
            Create and return the ``ArgumentParser`` which will be used to
            parse the arguments to this command.
            """
            if not 'prog' in self.parser_kwargs:
                self.parser_kwargs['prog'] = "%s %s" % (os.path.basename(prog_name), subcommand)
            if not 'description' in self.parser_kwargs:
                self.parser_kwargs['description'] = self.help or None
            parser = CommandParser(
                self, **self.parser_kwargs
            )
    

The problem is that in this case you will have two conflicting way to define description (using parser_kwargs or help), but possibly it's not a big issue.

Last edited 8 months ago by Dmitry (previous) (diff)

comment:5 Changed 8 months ago by Tom Forbes

The create_parser method could just pass in all kwargs to CommandParser, rather than having a parser_kwargs instance variable?

def create_parser(self, **kwargs):
    return super().create_parser(my_custom_arg=123)

# In BaseCommand
def create_parser(self, **kwargs):
    kwargs.setdefault('description', ...)
    return CommandParser(self, **kwargs)

Might be a bit more flexible

comment:6 Changed 8 months ago by Tom Forbes

Cc: Tom Forbes added

comment:7 in reply to:  5 ; Changed 8 months ago by Dmitry

Replying to Tom Forbes:

The create_parser method could just pass in all kwargs to CommandParser, rather than having a parser_kwargs instance variable?

def create_parser(self, **kwargs):
    return super().create_parser(my_custom_arg=123)

# In BaseCommand
def create_parser(self, **kwargs):
    kwargs.setdefault('description', ...)
    return CommandParser(self, **kwargs)

Might be a bit more flexible

I agree, this looks better.

comment:8 Changed 4 months ago by humbertotm

Owner: changed from nobody to humbertotm
Status: newassigned

Hi, guys.

I would like to work on this.
There's been no activity in this thread lately, so I'm hoping this is still a valid issue. If not, please let me know.
Thanks!

comment:9 in reply to:  7 Changed 4 months ago by humbertotm

Replying to Dmitry:

Replying to Tom Forbes:

The create_parser method could just pass in all kwargs to CommandParser, rather than having a parser_kwargs instance variable?

def create_parser(self, **kwargs):
    return super().create_parser(my_custom_arg=123)

# In BaseCommand
def create_parser(self, **kwargs):
    kwargs.setdefault('description', ...)
    return CommandParser(self, **kwargs)

Might be a bit more flexible

I agree, this looks better.

By the looks of it, I am guessing that every call to this method in the code base would have to change in order to fit the proposed method signature, right?

Last edited 4 months ago by humbertotm (previous) (diff)

comment:10 Changed 4 months ago by humbertotm

This is the fix I'm proposing based on my understanding of the issue. As far as I understand, the requested enhancement requires that CommandParser can ultimately receive any additional args that it can pass to its parent class ArgumentParser. What I did was adding an additional positional argument in the form of kwargs to the create_parser() method in BaseCommand as follows:

def create_parser(self, prog_name, subcommand, **kwargs):
        """
        Create and return the ``ArgumentParser`` which will be used to
        parse the arguments to this command. Hi.
        """
        default_kwargs = {
            'prog': '%s %s' % (os.path.basename(prog_name), subcommand),
            'description': self.help or None,
            'formatter_class': DjangoHelpFormatter,
            'missing_args_message': getattr(self, 'missing_args_message', None),
            'called_from_command_line': getattr(self, '_called_from_command_line', None)
        }
        kwargs.update(default_kwargs)
        parser = CommandParser(**kwargs)
        ...

This way, the kwargs provided to the method call are merged with the default kwargs into a single dictionary to be passed on to CommandParser. These changes do not break the test suite, but additional regression tests are still pending.
Let me know if I'm failing to see or understand something by following this approach.
Thanks!

comment:11 Changed 4 months ago by Claude Paroz

Has patch: set
Needs tests: set

comment:12 in reply to:  11 ; Changed 4 months ago by humbertotm

Replying to Claude Paroz:

Hi, Claude!

I was going through the tests dir looking for the appropriate place for this fix's test, but I am a bit lost.
The /tests/admin_scripts/tests.py file seems like a good candidate, but I have my doubts.
Any guidance on this would be greatly appreciated!

comment:13 in reply to:  12 Changed 4 months ago by Carlton Gibson

Replying to humbertotm:

The /tests/admin_scripts/tests.py file seems like a good candidate

Either that or tests/user_commands.

comment:14 Changed 4 months ago by Claude Paroz

+1 for user_commands.

comment:15 Changed 4 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e95008f2:

Fixed #29152 -- Allowed passing kwargs to ArgumentParser initialization in management commands.

Note: See TracTickets for help on using tickets.
Back to Top