[Django] #24863: Make `django.db.models.Manager.from_queryset` copy over properties and not just methods
Django
just methods
----------------------------------------------+--------------------
Reporter: coolRR | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
When I define a `QuerySet` subclass, I like defining not only methods but
properties on it.
The function `django.db.models.Manager.from_queryset` was introduced so we
could succinctly create a `Manager` subclass that gets the same
functionality that we defined on our `QuerySet` subclass. It copies over
all the methods defind in the `QuerySet` class to the `Manager` class. But
it doesn't copy properties, so they remain inaccessible from the `Manager`
class.
I want `django.db.models.Manager.from_queryset` to copy over the
properties as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/24863>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
just methods
Reporter: coolRR | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: loic (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Loic, what do you think about this?
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:1>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by m_sha):
* owner: nobody => m_sha
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:2>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* cc: mjtamlyn (added)
* stage: Unreviewed => Accepted
Comment:
Queryset at the moment has only two properties, both of which make no
sense on the manager (`ordered` and `db`).
I'm tentatively accepting this ticket, but I would like someone to explain
a concrete use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:3>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I assume the OP is referring to dynamic properties (e.g. using `@property`
or `@cached_property` - so basically methods but called implicitly), as
that's the only use case for this I can imagine. Copying over static
attributes doesn't make much sense to me.
I'm not strongly opposed to this, but if the implementation turns out to
be complex or difficult, I'd be perfectly happy to close it wontfix and
say "just use methods instead."
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:4>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coolRR):
Carl: Yes, that's my intention.
mjtamlyn: Okay, here's a concrete use case. I have a model `CreditCard`
and a corresponding queryset `CreditCardQuerySet`. I want to have a
property `expired` on `CreditCardQuerySet` so I could filter to expired
credit cards like `CreditCard.objects.expired`. (Assuming there isn't a
straightforward query to do that.) I could do it as a method but I always
prefer using a `property` when there aren't any arguments.
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:5>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
I have no idea how we'd be able to only take those properties and no
attributes or other properties, except for requiring an attribute on the
property. But I think just using a function without any arguments is just
fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:6>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coolRR):
I'm not really understanding what the implementation problem is. If you're
going over all the members of the `QuerySet` class, you can see which are
properties and then copy them. So what's the problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:7>
Django
just methods
Reporter: coolRR | Owner: m_sha
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
Stylistically, I'd say a property is wrong for that use case, I don't like
properties which do round trips to the database as when reading the code
it looks like an attribute. That said...
The implementation is straightforwards, we can use
https://docs.python.org/3/library/inspect.html#inspect.isdatadescriptor
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:8>
Django
just methods
Reporter: coolRR | Owner:
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by m_sha):
* owner: m_sha =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:9>
Django
just methods
Reporter: coolRR | Owner:
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Would this solve #25201? That is, `use_for_related_fields` could be set on
the `QuerySet` and then copied to the manager when using `as_manager()`?
Would it be better to have something like
`.as_manager(use_for_related_fields=True)`?
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:10>
Django
just methods
Reporter: Ram Rachum | Owner:
| KaratasFurkan
Type: New feature | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by KaratasFurkan):
* owner: (none) => KaratasFurkan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:11>
Django
just methods
Reporter: Ram Rachum | Owner: Furkan
| Karataş
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Furkan Karataş):
* has_patch: 0 => 1
Comment:
PR is opened: https://github.com/django/django/pull/16919
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:12>
Django
just methods
Reporter: Ram Rachum | Owner: Furkan
| Karataş
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => wontfix
* stage: Accepted => Unreviewed
Comment:
I agree with
[https://github.com/django/django/pull/16919/files#r1217066741 Lily], Mark
and Carl weren't convinced either. Using properties in `Queryset` to
perform queries is confusing, it gets even more puzzling when we realize
that some of them (e.g. `ordered`, `query`) have to be omitted in a
potential implementation. IMO, it's not worth the additional complexity.
--
Ticket URL: <https://code.djangoproject.com/ticket/24863#comment:13>