Opened 11 years ago
Closed 9 years ago
#22341 closed Cleanup/optimization (fixed)
Split django.db.models.fields.related into multiple modules.
Reported by: | loic84 | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
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
The django.db.models.fields.related
module is very large and pretty hard to work with.
It contains a lot of similar concepts with only slight differences, and one thing can easily be mistaken for its exact opposite, which makes navigating the file very error-prone. This is made worse by the fact that some class name are borderline wrong, (e.g. ReverseSingleRelatedObjectDescriptor
which actually is the forward FK descriptor).
Quoting akaariai: "fields/related.py is a brain melting machine".
This ticket proposes that we turn related.py
into a package with the following modules: related_field.py
, many_to_one.py
, one_to_one.py
and many_to_many.py
.
Change History (14)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
@timo, very good point, clearly this needs to wait for the 1.7 release.
The latest effort regarding this ticket is tracked at https://github.com/django/django/pull/2411.
comment:3 by , 10 years ago
I'm planning to resume work on this.
https://github.com/django/django/pull/2411#issuecomment-56624379 shows a summary of where we were.
I wonder if ForeignObject
shouldn't move to many_to_one.py
as well.
comment:4 by , 10 years ago
Keywords: | 1.8-alpha added |
---|
I'm tagging this ticket 1.8-alpha so we can try to make this change close to that date (currently scheduled for January 12) before we fork stable/1.8.x.
comment:5 by , 10 years ago
@loic hi. this ticket isn't assigned to anybody. are you planing to resume on this ticker or i can start to do it?
comment:6 by , 10 years ago
Hi @e0ne, I'm still planning to tackle this once I return from holiday. I discussed this recently on IRC and it'll go further than just splitting the files:
- Refactor
ForeignObject
to not depend onReverseSingleRelatedObjectDescriptor
, which includes factoring out the relevant bits ofReverseSingleRelatedObjectDescriptor
into a new descriptor. - Move the existing
ReverseSingleRelatedObjectDescriptor
tomany_to_one.py
but deprecate it and introduce a new better named descriptor, since this is actually the forward side of a FK.
comment:7 by , 10 years ago
Has patch: | set |
---|
comment:8 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:9 by , 10 years ago
Keywords: | 1.8-alpha removed |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
from Loic on the PR: "I think we are going to postpone this refactor some more."
comment:10 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 9 years ago
The discussions on the previous attempt show that grouping code by the type of relation (one-to-one, one-to-many, many-to-many) doesn't work very well. It raises hard questions about where code would end up after plausible refactorings.
I'd like to suggest a different approach: group code by the type of objects defined:
- descriptors
- managers
- "rel objects" (these don't have a good name)
- etc.
This is straightforward, reasonably future proof — if we introduce a new kind of object it's easy to add a module.
It groups similar code nicely and allows for module docstrings explaining the role of each layer.
comment:13 by , 9 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
I suggest we do it after 1.7 is released so backporting any bug fixes in this area in the meantime is not so painful.