Reorder management command arguments in --help output to prioritize command-specific arguments

[David Foster]

Currently if you run a custom management command with --help, you will get output that looks like:

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:

Discussion on django-developers: ​https://groups.google.com/forum/#!topic/django-developers/PByZfN_IccE

[David Foster]

In case it makes a difference, I already have a patch that performs the change proposed in the description.

[David Foster]

​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.

[Carlton Gibson]

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 test.

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.

[Tim Graham <timograham@…>]

In c3055242:

Fixed #29301 -- Made management command --help display command-specific arguments before common arguments.

[Jann Haber]

I have an observation on the change made above:
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?

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.

[Claude Paroz]

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 ​PR.

[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.

(C) 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 formatter, 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 +1 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.

[Claude Paroz]

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.

[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.

[Claude Paroz]

Jann, could you point us to the code you are referring to for django-nose?

[David Foster]

Here's an implementation of approach (B): ​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.

[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) .

[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.

[Adam Johnson]

Claude's custom formatter PR looks like the approach to me, since it is at the presentation layer only.

[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 (except for 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.

[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.)

[Claude Paroz <claude@…>]

In ce3351b9:

Fixed #29301 -- Added custom help formatter to BaseCommand class

This partially reverts c3055242c81812278ebdc93dd109f30d2cbd1610.
Thanks Adam Johnson and Carlton Gibson for the reviews.

[Claude Paroz <claude@…>]

In 33b5313d:

[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.