[Django] #29133: django.core.management.call_command fails if required option passed in via **options
Django
**options
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: nobody
Tomic |
Type: | Status: assigned
Uncategorized |
Component: Core | Version: 2.0
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
If `call_command` is called with a required option, e.g;
{{{
management.call_command('required_option', "--need-me=asdf", "--need-me-
too=fdsa")
management.call_command('required_option', need_me="asdf",
need_me_too="fdsa")
}}}
The first call succeeds, but the second fails with an exception:
{{{
django.core.management.base.CommandError: Error: the following arguments
are required: -n/--need-me, -t/--need-me-too
}}}
The problem is due to `parse_args` being called in `call_command` without
checking for potentially required arguments in `**options`
--
Ticket URL: <https://code.djangoproject.com/ticket/29133>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
**options
-------------------------------------+-------------------------------------
| Tomic
Type: Uncategorized | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Alex Tomic
Comment:
First time contributor, pardon for any newbie mistakes in the process!
This bug has an easy workaround, but it cost me enough time today that I
thought I would finally try to get my hands dirty in the code. PR has
been created here: https://github.com/django/django/pull/9701
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Tomic | Owner: Alex
Type: | Tomic
Cleanup/optimization | Status: assigned
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: Uncategorized => Cleanup/optimization
Comment:
I'm not sure if the additional code complexity is worth supporting this. I
noted this recommendation from the Python documentation, "Required options
are generally considered bad form because users expect options to be
optional, and thus they should be avoided when possible." An alternative
could be to document the limitation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Tomic | Owner: Alex
Type: | Tomic
Cleanup/optimization | Status: assigned
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
-------------------------------------+-------------------------------------
Comment (by Alex Tomic):
Replying to [comment:2 Tim Graham]:
> I'm not sure if the additional code complexity is worth supporting this.
I noted this recommendation from the Python documentation, "Required
options are generally considered bad form because users expect options to
be optional, and thus they should be avoided when possible." An
alternative could be to document the limitation.
Thanks for the quick feedback.
I figured this would be a 1-2 line fix, but it turned out to be a bit more
complex than I thought. I agree it's a bit unnecessary given that the
python docs recommend against this practice. Ironically, I ran up against
this as I was trying to wrap a few such unwieldy management commands doing
exactly that.
I don't believe it is universally agreed upon that -/-- parameters should
always be optional, though, outside of the Python world. Getopt_long [ 1 ]
and the Open Group utility conventions [ 2 ], for example, don't recommend
against it as far as I can tell.
All that said, if this patch seems too risky to fix such a minor issue,
I'd also be happy to contribute a documentation change instead.
[1] https://www.gnu.org/software/libc/manual/html_node/Getopt-Long-
Options.html#Getopt-Long-Options
[2]
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
e.g. Section 12.1.9
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Tomic | Owner: Alex
Type: | Tomic
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
After looking at the fix, I simplified it a bit and I think it's fine to
fix this. I left comments for improvement on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Tomic | Owner: Alex
Type: | Tomic
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
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Thanks for the feedback. I've simplified the test case and supporting
management command to only test what was not working before, and pushed up
a new commit.
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Tomic | Owner: Alex
Type: | Tomic
Component: Core (Management | Version: 2.0
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a1a3e515616da102fc48a1e1af8a5b2f429f747e" a1a3e51]:
{{{
#!CommitTicketReference repository=""
revision="a1a3e515616da102fc48a1e1af8a5b2f429f747e"
Fixed #29133 -- Fixed call_command() crash if a required option is passed
in options.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29133#comment:6>