[Django] #29152: management commands rigid ArgumentParser initialization
Django
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner: nobody
Type: New | Status: new
feature |
Component: Core | Version: 2.0
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29152>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: dmippolitov | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: New feature => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:2>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Would a hook to provide some additional keyword arguments be sufficient?
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:3>
Django
commands
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dmitry):
Replying to [comment:3 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 = {}
}}}
2. 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'] = 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:4>
Django
commands
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tom Forbes):
The create_parser method could just pass in all kwargs to CommandParser,
rather than having a parser_kwargs instance variable?
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:5>
Django
commands
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Tom Forbes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:6>
Django
commands
Reporter: Dmitry | Owner: nobody
Cleanup/optimization |
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dmitry):
Replying to [comment:5 Tom Forbes]:
> The create_parser method could just pass in all kwargs to CommandParser,
rather than having a parser_kwargs instance variable?
>
>
> {{{#!python
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:7>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => humbertotm
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:8>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by humbertotm):
Replying to [comment:7 Dmitry]:
> Replying to [comment:5 Tom Forbes]:
> > The create_parser method could just pass in all kwargs to
CommandParser, rather than having a parser_kwargs instance variable?
> >
> >
> > {{{#!python
> > 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 this, I am guessing that every call to this method in the
code base would have to be modified to fit this new signature, right?
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:9>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:10>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:11>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by humbertotm):
Replying to [comment:11 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:12>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Replying to [comment:12 humbertotm]:
> The `/tests/admin_scripts/tests.py` file seems like a good candidate
Either that or `tests/user_commands`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:13>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
+1 for `user_commands`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:14>
Django
commands
-------------------------------------+-------------------------------------
Reporter: Dmitry | Owner:
Type: | humbertotm
Component: Core (Management | Version: 2.0
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e95008f2411d50929873f634c3e14ebac811fd28" e95008f2]:
{{{
#!CommitTicketReference repository=""
revision="e95008f2411d50929873f634c3e14ebac811fd28"
Fixed #29152 -- Allowed passing kwargs to ArgumentParser initialization in
management commands.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29152#comment:15>