[Django] #32317: Clean up loaddata
Django
-------------------------------------+-------------------------------------
Reporter: William | Owner: William Schwartz
Schwartz |
Type: | Status: assigned
Cleanup/optimization |
Component: Core | Version: master
(Management commands) |
Severity: Normal | Keywords: loaddata
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
[https://www.python.org/dev/peps/pep-0008/#programming-recommendations
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 #XXX 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32317>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: loaddata | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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
> [https://www.python.org/dev/peps/pep-0008/#programming-recommendations
> 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 #XXX 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.
New description:
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
[https://www.python.org/dev/peps/pep-0008/#programming-recommendations
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 [https://github.com/django/django/pull/13842 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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Looks good to me, awesome work!
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: loaddata | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by William Schwartz):
Thanks, Alexandr. Would you be able to review the PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
commands) |
Severity: Normal | Resolution:
Keywords: loaddata | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: loaddata | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"1557778121e5d04a80f1f7c81eeb4b47400939e4" 1557778]:
{{{
#!CommitTicketReference repository=""
revision="1557778121e5d04a80f1f7c81eeb4b47400939e4"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: loaddata | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"1e655d35ad7540ea557c532ebc72a9acc407df50" 1e655d35]:
{{{
#!CommitTicketReference repository=""
revision="1e655d35ad7540ea557c532ebc72a9acc407df50"
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'.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Component: Core (Management | Version: dev
commands) |
Keywords: loaddata | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"de32fe83a2e4a20887972c69a0693b94eb25a88b" de32fe83]:
{{{
#!CommitTicketReference repository=""
revision="de32fe83a2e4a20887972c69a0693b94eb25a88b"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32317#comment:8>