Opened 4 years ago

Closed 3 years ago

#32317 closed Cleanup/optimization (fixed)

Clean up loaddata

Reported by: William Schwartz Owned by: William Schwartz
Component: Core (Management commands) Version: dev
Severity: Normal Keywords: loaddata
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 (last modified by William Schwartz)

In django.core.management.commands.Command, load_label is 66 lines long with maximum indentation of 9 levels (the row starts at column 37). find_fixtures is 56 lines. These monolith methods are hard to read, hard to override, and, in some places, violate PEP-8's recommendation to keep try blocks small.

The reason I care about this is that an app I'm working on may need to customize how loaddata finds fixtures, and, unlike templates, there isn't a nice loader API to hook into. So that leaves me with overriding find_fixtures. My initial attempt was a mess because of how giant the parent class's method is.

I am submitting PR GH-13842 with my proposed refactoring. The commits will need to be squashed before merging the PR, but I wanted to include commit message to justify some of the less obvious-looking changes.

Change History (8)

comment:1 by William Schwartz, 4 years ago

Description: modified (diff)

comment:2 by Alexandr Tatarinov, 4 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

Looks good to me, awesome work!

comment:3 by William Schwartz, 4 years ago

Thanks, Alexandr. Would you be able to review the PR?

comment:4 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 1e655d35:

Refs #32317 -- Cleaned up try/except blocks in loaddata command.

This moves code unable to trigger relevant exceptions outside of
try/except blocks, and changes 'objects' to 'objects_in_fixture'
which is equal to the length of 'objects'.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 1557778:

Refs #32317 -- Simplified find_fixtures() in loaddata command.

This always replaces 'fixture_name' with its base name, which preserves
the previous behavior, because os.path.basename() was not called only on
relative paths without os.path.sep i.e. when base name was equal to the
file name.

This also changes os.path.dirname() and os.path.basename() calls to the
equivalent os.path.split() call.

comment:7 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In de32fe83:

Fixed #32317 -- Refactored loaddata command to make it extensible.

Moved deeply nested blocks out of inner loops to improve readability
and maintainability.

Thanks to Mariusz Felisiak, Shreyas Ravi, and Paolo Melchiorre for
feedback.

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