[Django] #29301: Reorder management command arguments in --help output to prioritize command-specific arguments
Django
command-specific arguments
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Foster |
Type: | Status: new
Uncategorized |
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 |
-------------------------------------+-------------------------------------
Currently if you run a custom management command with --help, you will get
output that looks like:
[[Image(https://s20.postimg.org/qhtlvybh9/BEFORE.png)]]
I have highlighted in yellow the useful information specific to the
command that is *not* boilerplate. Notice that most of this yellow text is
at the end of the output, with the boilerplate dominating what the user
reads first.
I propose reordering the options in the output so that the useful
information is at the *beginning* rather than the end, so that it looks
like the following:
[[Image(https://s20.postimg.org/m8ovttiil/AFTER.png)]]
Discussion on django-developers: https://groups.google.com/forum/#!topic
/django-developers/PByZfN_IccE
--
Ticket URL: <https://code.djangoproject.com/ticket/29301>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: Uncategorized | 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
-------------------------------------+-------------------------------------
Comment (by David Foster):
In case it makes a difference, I already have a patch that performs the
change proposed in the description.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:1>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Status: new
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
-------------------------------------+-------------------------------------
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:2>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
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
-------------------------------------+-------------------------------------
* owner: nobody => David Foster
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9871 PR]
It is not clear whether this kind of change merits a test or not. I did
not provide a test.
It seems unlike that the detail of the argument order would be significant
enough to call out in docs, so I did not make any docs changes either.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:3>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
This looks good to me.
* Functions correctly with manual testing
* Command parsing is directly tested in `tests/user_commands/tests.py`.
That exercises this change, without an explicit tests.
I'm going to mark it 'Ready for checkin' on that basis.
Maybe Tim sends it back for:
* An explicit test. (That custom option appears before, e.g. verbosity for
command X.)
* A mention in the release notes at least.
I wasn't sure on either of these.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:4>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Component: Core (Management | Version: 2.0
commands) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c3055242c81812278ebdc93dd109f30d2cbd1610" c3055242]:
{{{
#!CommitTicketReference repository=""
revision="c3055242c81812278ebdc93dd109f30d2cbd1610"
Fixed #29301 -- Made management command --help display command-specific
arguments before common arguments.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:5>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: closed
Component: Core (Management | Version: 2.0
commands) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jann Haber):
I have an observation on the change made above:
If any third-party app now adds one of the default arguments in the
.add_argument() function, django now fails trying to re-add the argument.
What would be the desired outcome in case of a conflict?
As an example:
django_nose adds a --version argument (showing the nose version). It
checks, if there is already a --version argument installed and if not,
adds the argument to the parser. In Django 2.0.5, it found the --version
argument from django and did nothing. In Django 2.1a1, it adds the
--version argument to the parser. Later, Django 2.1a1 fails trying to add
another --version argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:6>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Component: Core (Management | Version: 2.0
commands) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
* severity: Normal => Release blocker
Comment:
I think something was wrong here as coding logic was changed to fix a
formatting/display issue.
I tried another approach using a custom formatter instead, see
[https://github.com/django/django/pull/9971 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:7>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Foster):
> If a third-party app adds one of the default arguments in the
.add_argument() function, django now fails trying to re-add the argument.
What would be the desired outcome in case of a conflict?
(A) If we take the interpretation that the Django framework "owns" the
default arguments then the conflict should cause a failure. Adding new
default arguments would then be a backward-incompatible change. This is
the current state now that the original patch is merged.
(B) If we take the interpretation that management commands are ultimately
in control of their arguments, I would expect Django to back off if sees
that a management command added its own argument with a conflicting name.
Adding new default arguments would then be a minor change.
Before the original patch for this ticket was merged, add_arguments() was
allowed even more leeway than (B), potentially removing default arguments
outright, setting a custom formatters, or doing other unusual things, even
though those possibilities were not documented. I don't think we should
necessary support arbitrary actions in add_arguments().
I'm personally +0 on approach (B). It would involve a small additional
patch where Django checks whether each default argument already existed
before adding its own.
---
Claude, I'm resistant to introduce a custom formatter here that
reinterprets the argument order at format time, as it feels relatively
complex and overrides the default behavior in a potentially unexpected
way. We also have to duplicate the list of default arguments in multiple
places which smells like a design issue.
As a minor point, adding a custom formatter in Django would probably break
any custom formatter added by preexisting management commands, such as in
add_arguments(). Adding a custom formatter feels like an odd thing for a
management command to do and we don't suggest doing that in our
documentation, but it's possible that certain advanced third party
commands may do it anyway. They then don't benefit from the argument
reordering change, under the assumption that the per-command custom
formatter would clobber the Django custom formatter.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:8>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
Comment:
If we agree that Django should add its arguments after the command's
arguments, then yes, the B) approach with Django catching any conflict
exception would make sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:9>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jann Haber):
From my point of view, B) would still be backwards-incompatible, since
some management commands' help messages will then behave differently. In
my example, the option --version would then print the nose version, where
it previously used to print the django version. However, it would
certainly impact less projects than option A).
Option C), to keep the behaviour the way it was before and only changing
the order of the arguments in the help message (which is what Claude was
trying to achieve in the PR), seems to have the least impact on the
existing commands while still achieving the goal of this ticket.
In any case, at least if option A) or B) are selected, this change should
be documented in the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:10>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Jann, could you point us to the code you are referring to for django-nose?
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:11>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Foster):
Here's an implementation of approach (B):
[https://github.com/django/django/pull/9973 PR]. I believe it resolves the
backward-compatibility concern that Jann raised.
This PR is just to get feedback on a concrete implementation/approach. If
selected I agree that the release notes should be tweaked as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:12>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jann Haber):
For the django-nose implementation I was referring here:
https://github.com/django-nose/django-
nose/blob/master/django_nose/runner.py#L143
As far as I have understood the code, it collects the options that django
has already added and will skip (later in L166) adding options with the
same name. Nose also has a --version argument. In django 2.0.5, this
argument is not in the list of arguments of the 'test' management command,
since skipped by django-nose.
So option B) would avoid the errors, however the outcome may be different
(in this case for --version, it then prints the nose version instead of
(previously) the django version). To me, this sounds like a very minor
change, that would most likely not have any impact on the vast majority of
projects. Still, I believe the change should be documented if we go down
path B) .
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:13>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Foster):
Okay, the next steps I see are:
1a) Verify django-nose works okay with the B patch,
1b) Extend the B patch with release notes changes,
2) Mark this ticket as ready for checkin (or whatever the appropriate
status is) to summon a maintainer,
3) Have a design discussion with the maintainer about which approach and
patch (B or C) to take.
We're coming up on Memorial day weekend here in the USA, so I expect to be
mostly dormant myself until Wed 5/30.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:14>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam (Chainz) Johnson (added)
Comment:
Claude's custom formatter PR looks like the approach to me, since it is at
the presentation layer only.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:15>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jann Haber):
I have tested both PRs with django_nose and both don't make django-nose
crash anymore. However, the outcome is a little different (as expected).
The PR of Claude is missing to sort --verbosity to the end, however this
could be added easily to the "show_last" list. The other arguments are
sorted to the end as expected. The arguments are (other than the order of
the arguments) identical as in Django 2.0
The PR of David does not add the django --verbosity (-v) or --version,
since these are "overwritten" by the corresponding arguments of nose.
Please note, that it is already sufficient if e.g. only the short or only
the long version of the argument is used by the third-party command, to
have the ArgumentError appear and then to skip the entire argument. -v is
already used by nose, so the django --verbosity is not added at all (the
short version is also -v).
I am also in favour of the PR of Claude. It seems to be the least impact
option and achieves exactly what the PR was aiming for.
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:16>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Well, who'd have thought this would have been an issue? You live and
learn.
Thanks for the input all.
Having looked at the two PRs, and given that this issue has come up, I
think the formatter is the right way to go.
* It works and is API specifically targeting our usage.
* Formatting aside, adding the default arguments first probably does make
sense.
* It's not clear that the `_try_add_argument` wouldn't add a further bug
that we run into later-on.
We should document in the release notes that the new formatter is used, so
if it does break anything people know to adjust.
(I think the small BC risk here is worth the extra readability for normal
case.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:17>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:18>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: new
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:19>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Component: Core (Management | Version: 2.0
commands) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"ce3351b9508896afdf87d11bd64fd6b5ad928228" ce3351b9]:
{{{
#!CommitTicketReference repository=""
revision="ce3351b9508896afdf87d11bd64fd6b5ad928228"
Fixed #29301 -- Added custom help formatter to BaseCommand class
This partially reverts c3055242c81812278ebdc93dd109f30d2cbd1610.
Thanks Adam Johnson and Carlton Gibson for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:20>
Django
command-specific arguments
-------------------------------------+-------------------------------------
Type: | Foster
Cleanup/optimization | Status: closed
Component: Core (Management | Version: 2.0
commands) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz <claude@…>):
In [changeset:"33b5313d7c0869d6268fa8d550d20eaa172bdfb1" 33b5313d]:
{{{
#!CommitTicketReference repository=""
revision="33b5313d7c0869d6268fa8d550d20eaa172bdfb1"
[2.1.x] Fixed #29301 -- Added custom help formatter to BaseCommand class
This partially reverts c3055242c81812278ebdc93dd109f30d2cbd1610.
Thanks Adam Johnson and Carlton Gibson for the reviews.
Backport of ce3351b9508896afdf87d11bd64fd6b5ad928228 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29301#comment:21>