Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#13760 closed Cleanup/optimization (fixed)

Management commands should not specify the default value twice

Reported by: Paul McMillan Owned by: nobody
Component: Core (Management commands) Version: 1.2
Severity: Normal Keywords:
Cc: 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

Many of the management commands have a code pattern that seems likely to invite errors:

In the base of the Command class, they define options, including default values like this:

class Command(BaseCommand):
    option_list = BaseCommand.option_list + (
        make_option('--format', default='json', dest='format',
            help='Specifies the output serialization format for fixtures.'),

Then later on in the handle method, they get the option value, specifying a new default value if the option is not found, like this:

    def handle(self, *app_labels, **options):
        format = options.get('format','json')

This second default value is an error. Using optparse means that we are certain to receive a value for each option (it defaults to None). Using options.get() with a default value is redundant. Trying to get a non-existent option should throw an error, not return a default value.

Most of the management commands need this cleanup.

Attachments (3)

remove-dupe-defaults.diff (12.7 KB) - added by Adam Vandenberg 6 years ago.
Remove duplicated default option values.
remove-duplicate-defaults.diff (12.2 KB) - added by Adam Vandenberg 6 years ago.
Rebased against trunk.
13760-3.diff (12.4 KB) - added by Claude Paroz 5 years ago.
Rebased against trunk (once again)

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

Changed 6 years ago by Adam Vandenberg

Attachment: remove-dupe-defaults.diff added

Remove duplicated default option values.

Changed 6 years ago by Adam Vandenberg

Rebased against trunk.

comment:2 Changed 6 years ago by Ramiro Morales

See also #10080 and #14909.

comment:3 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Cleanup/optimization

Changed 5 years ago by Claude Paroz

Attachment: 13760-3.diff added

Rebased against trunk (once again)

comment:4 Changed 5 years ago by Claude Paroz

Easy pickings: unset
Has patch: set
Triage Stage: AcceptedReady for checkin
UI/UX: unset

comment:5 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: newclosed

In [17028]:

Fixed #13760 -- Cleaned up unnecessary default option handling in a bunch of management commands. Thanks to Paul McMillan for the report and to adamv and Claude Paroz for the patch.

comment:6 Changed 5 years ago by Ramiro Morales

In [17678]:

Fixed #17820 -- Fixed more occurrences of redundant handling of management commands options.

They had been missen in the first attempts or had been introduced afterwards.

Refs #13760, #10080, #17799.

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