Opened 14 years ago

Closed 12 years ago

Last modified 12 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 13 years ago.
Remove duplicated default option values.
remove-duplicate-defaults.diff (12.2 KB ) - added by Adam Vandenberg 13 years ago.
Rebased against trunk.
13760-3.diff (12.4 KB ) - added by Claude Paroz 12 years ago.
Rebased against trunk (once again)

Download all attachments as: .zip

Change History (9)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

by Adam Vandenberg, 13 years ago

Attachment: remove-dupe-defaults.diff added

Remove duplicated default option values.

by Adam Vandenberg, 13 years ago

Rebased against trunk.

comment:2 by Ramiro Morales, 13 years ago

See also #10080 and #14909.

comment:3 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

by Claude Paroz, 12 years ago

Attachment: 13760-3.diff added

Rebased against trunk (once again)

comment:4 by Claude Paroz, 12 years ago

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

comment:5 by Julien Phalip, 12 years ago

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 by Ramiro Morales, 12 years ago

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