[Django] #22341: Split django.db.models.fields.related into multiple modules.
Django
----------------------------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:1>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:2>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:3>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* keywords: => 1.8-alpha
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:4>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: 1.8-alpha | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:5>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: 1.8-alpha | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:6>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: 1.8-alpha | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:7>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:8>
Django
Reporter: loic84 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* keywords: 1.8-alpha =>
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
from Loic on the PR: "I think we are going to postpone this refactor some
more."
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:9>
Django
Reporter: loic84 | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by aaugustin):
* status: new => assigned
* owner: nobody => aaugustin
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:10>
Django
Reporter: loic84 | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:11>
Django
Reporter: loic84 | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by aaugustin):
See https://github.com/django/django/pull/5318.
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:12>
Django
Reporter: loic84 | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:13>
Django
Reporter: loic84 | Owner: aaugustin
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Aymeric Augustin <aymeric.augustin@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"005c9fc45f99d03344fa9fae3dd984c2ab87f1ea" 005c9fc]:
{{{
#!CommitTicketReference repository=""
revision="005c9fc45f99d03344fa9fae3dd984c2ab87f1ea"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22341#comment:14>