Code

Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#13760 closed Cleanup/optimization (fixed)

Management commands should not specify the default value twice

Reported by: PaulM 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 adamv 4 years ago.
Remove duplicated default option values.
remove-duplicate-defaults.diff (12.2 KB) - added by adamv 4 years ago.
Rebased against trunk.
13760-3.diff (12.4 KB) - added by claudep 3 years ago.
Rebased against trunk (once again)

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 4 years ago by adamv

Remove duplicated default option values.

Changed 4 years ago by adamv

Rebased against trunk.

comment:2 Changed 3 years ago by ramiro

See also #10080 and #14909.

comment:3 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

Changed 3 years ago by claudep

Rebased against trunk (once again)

comment:4 Changed 3 years ago by claudep

  • Easy pickings unset
  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

comment:5 Changed 3 years ago by julien

  • Resolution set to fixed
  • Status changed from new to closed

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 2 years ago by ramiro

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.