Opened 4 years ago

Closed 2 years ago

Last modified 4 months ago

#19973 closed Cleanup/optimization (fixed)

Management commands migration to argparse

Reported by: José Luis Patiño Andrés Owned by: José Luis Patiño Andrés
Component: Core (Management commands) Version: master
Severity: Normal Keywords: optparse, argparse, basecommand
Cc: hans@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The class BaseCommand, from which developers subclass their own management commands, is using the `optparse` module, which is actually deprecated.

If there are no important reasons to remain using the deprecated module, perhaps would be a good idea to port the BaseCommand code to the new module `argparse`, which will be used in Python 3.

Change History (20)

comment:1 Changed 4 years ago by Ramiro Morales

Easy pickings: unset
Triage Stage: UnreviewedAccepted

Good idea, has been discussed before (but there isn't anassociated ticket so thanks for opening this one).

But possible only once the minimal Python version supported by Django gets to the 2.7 level, such is the version where argparse got added to the stdlib.

comment:2 Changed 4 years ago by José Luis Patiño Andrés

Ok. I think that since I've took part some days ago in fixing the ticket #19877 and I've got familiar with that part, I can also try to fix this one. So it can be appended to Django master code when it reaches the 2.7 Python as minimal version supported.

comment:3 Changed 4 years ago by José Luis Patiño Andrés

Owner: changed from nobody to José Luis Patiño Andrés
Status: newassigned

comment:4 Changed 4 years ago by Carl Meyer

We'll need to provide a deprecation path and migration strategy for authors of custom management commands, since optparse is exposed directly to them via the definition of additional options for a custom command.

comment:5 Changed 4 years ago by Hans Andersen

Cc: hans@… added

comment:6 Changed 3 years ago by Marc Tamlyn

We are now able to do this as Django master is 2.7 only. I'm not having many great ideas about how to support both at once, my instinct says we'll need to do something like providing a flag on the Command instance to say "use_argpase" instead. Even this isn't clear in terms of how it would work.

Then again, there is no planned removal date for optparse (http://www.python.org/dev/peps/pep-0389/#deprecation-of-optparse) so it's not really urgent.

comment:7 Changed 3 years ago by Claude Paroz

Before starting with the implementation, there is a design decision to make. argparse doesn't provide the same declarative syntax that optparse used to provide with make_option. I see two main ways to go forward:

  1. Keep declarative syntax and mostly the same Command API that we have now. Instead of importing make_option from optparse, we would import it from a Django-provided utility, optionally renaming it make_argument. Internally, this would only build a list of parameters that we would then pass to add_argument calls in create_parser.
    Advantages: nicer declarative syntax, less changes for custom command writers (and accessorily easier for call_command to get default values without instantiating the parser)
  1. Depart from the current declarative syntax and change the documentation about adding options for custom commands, basically instructing to subclass create_parser (or more probably create a new subclassable method BaseCommand.add_arguments(self, parser)) and to add arguments by calling parser.add_argument(<params>).
    Advantages: closer approach to standard Python

comment:8 Changed 3 years ago by Jerome Leclanche

So on Pootle we've been working on moving away from optparse in all our backend scripts but due to the dependency django has on optparse for specifically this, we are forced to use both optparse and argparse in the code which makes kittens cry.

@claudep I would go with option 2. The advantage here is that it is much more powerful right out of the box. Subcommands would create their own parser and would be able to inherit the parent parser simply by calling super(). This allows easy creation of things such as subcommands and such.

Being declarative is nice in principle but doesn't really work out for argument parsing - there is a reason python changed the logic there.

Are you planning to work on this bug? I would love to help, FWIW.

comment:9 Changed 3 years ago by Claude Paroz

Thanks Adys for your input. Feel free to start hacking on it, and share your work.

comment:10 Changed 2 years ago by Claude Paroz

@Adys, I made some progress on this issue. If you have also worked on this, we might share our work, otherwise just wait that my branch is in a publishable state...

comment:11 Changed 2 years ago by Claude Paroz <claude@…>

In 96e4b52ab2c6fae3e46a197d44c42cb6ebde7d9c:

Converted Django scripts to argparse

Refs #19973.

comment:12 Changed 2 years ago by Claude Paroz

Has patch: set

Here is the pull request with my work so far: https://github.com/django/django/pull/2780

Comments welcome.

comment:13 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:14 Changed 2 years ago by Tim Graham

Claude, do you know if this will obsolete #11407?

comment:15 Changed 2 years ago by Claude Paroz

Hopefully, yes, but in any case, I'll revisit #11407 after committing this one.

comment:16 Changed 2 years ago by Claude Paroz

Resolution: fixed
Status: assignedclosed

Fixed in:

comment:17 Changed 2 years ago by Claude Paroz <claude@…>

In 5949c2118d7cc7cffe86cd9db1820b186b88bfc0:

Restored command error behavior when called from command line

Refs #19973.

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

In 6a70cb53:

Refs #19973 -- Removed optparse support in management commands per deprecation timeline.

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

In ccd5a23f:

Fixed #27000 -- Removed BaseCommand.usage() per deprecation timeline (refs #19973).

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

In 7dd8e53c:

[1.10.x] Fixed #27000 -- Removed BaseCommand.usage() per deprecation timeline (refs #19973).

Backport of ccd5a23fba91e66656bb19b60bb6d737cab728f5 from master

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