Opened 16 months ago

Last modified 7 months ago

#22341 new Cleanup/optimization

Split django.db.models.fields.related into multiple modules.

Reported by: loic84 Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (9)

comment:1 Changed 16 months ago by timo

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

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 Changed 16 months ago by loic84

@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 Changed 10 months ago by loic

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 Changed 9 months ago by timgraham

  • 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 Changed 7 months ago by e0ne

@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 Changed 7 months ago by loic

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 7 months ago by loic (previous) (diff)

comment:7 Changed 7 months ago by timgraham

  • Has patch set

comment:8 Changed 7 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 7 months ago by timgraham

  • Keywords 1.8-alpha removed
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

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