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 Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by loic84, 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 Loic Bistuer, 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 Tim Graham, 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 Ivan Kolodyazhny, 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 Loic Bistuer, 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 on ReverseSingleRelatedObjectDescriptor, which includes factoring out the relevant bits of ReverseSingleRelatedObjectDescriptor into a new descriptor.
  • Move the existing ReverseSingleRelatedObjectDescriptor to many_to_one.py but deprecate it and introduce a new better named descriptor, since this is actually the forward side of a FK.
Last edited 10 years ago by Loic Bistuer (previous) (diff)

comment:7 by Tim Graham, 10 years ago

Has patch: set

comment:8 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham, 10 years ago

Keywords: 1.8-alpha removed
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

from Loic on the PR: "I think we are going to postpone this refactor some more."

comment:10 by Aymeric Augustin, 9 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:11 by Aymeric Augustin, 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 Tim Graham, 9 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:14 by Aymeric Augustin <aymeric.augustin@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 005c9fc:

Fixed #22341 -- Split django.db.models.fields.related.

At 2800 lines it was the largest module in the django package. This
commit brings it down to a more manageable 1620 lines.

Very small changes were performed to uniformize import style.

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