[Django] #35154: QuerySet implements `contains` but not `__contains__`
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
Type: | Status: new
Uncategorized |
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords: queryset contains
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is a similar proposal to https://code.djangoproject.com/ticket/31561,
but it is not the same. Currently using
{{{
x in myQuerySet
}}}
results in python using the fallback solution:
https://docs.python.org/3/reference/expressions.html#membership-test-
details
Because https://groups.google.com/g/django-
developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ deiced to implement contains in
https://code.djangoproject.com/ticket/24141
I think it is only consistent to have the same behavior implemented in
__contains__. I would expect that, it is also a more efficient
implementation and unifies django behavior. Nevertheless, documentation is
needed why this inconsistency exists. I was not able to find a reason.
Because the mailing list agreed on adding contains, this is discussed
behavior. Why was __contains__ not added in the first place? To not have
breaking changes? I cannot see what would break.
As said in https://code.djangoproject.com/ticket/31561 a queryset could be
a collection to make typing easier. But this is not the intention of this
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/35154>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: queryset contains | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* type: Uncategorized => New feature
* resolution: => wontfix
Old description:
> This is a similar proposal to
> https://code.djangoproject.com/ticket/31561, but it is not the same.
> Currently using
> {{{
> x in myQuerySet
> }}}
> results in python using the fallback solution:
> https://docs.python.org/3/reference/expressions.html#membership-test-
> details
> Because https://groups.google.com/g/django-
> developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ deiced to implement contains in
> https://code.djangoproject.com/ticket/24141
> I think it is only consistent to have the same behavior implemented in
> __contains__. I would expect that, it is also a more efficient
> implementation and unifies django behavior. Nevertheless, documentation
> is needed why this inconsistency exists. I was not able to find a reason.
> Because the mailing list agreed on adding contains, this is discussed
> behavior. Why was __contains__ not added in the first place? To not have
> breaking changes? I cannot see what would break.
>
> As said in https://code.djangoproject.com/ticket/31561 a queryset could
> be a collection to make typing easier. But this is not the intention of
> this issue.
New description:
This is a similar proposal to https://code.djangoproject.com/ticket/31561,
but it is not the same. Currently using
{{{
x in myQuerySet
}}}
results in python using the fallback solution:
https://docs.python.org/3/reference/expressions.html#membership-test-
details
Because https://groups.google.com/g/django-
developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ deiced to implement contains in
https://code.djangoproject.com/ticket/24141
I think it is only consistent to have the same behavior implemented in
`__contains__`. I would expect that, it is also a more efficient
implementation and unifies django behavior. Nevertheless, documentation is
needed why this inconsistency exists. I was not able to find a reason.
Because the mailing list agreed on adding contains, this is discussed
behavior. Why was `__contains__` not added in the first place? To not have
breaking changes? I cannot see what would break.
As said in https://code.djangoproject.com/ticket/31561 a queryset could be
a collection to make typing easier. But this is not the intention of this
issue.
--
Comment:
> Why was `__contains__` not added in the first place?
Have you read the discussion that you mention in the ticket? or comments
in #24141? The entire discussion is about `__contains__` and there was a
consensus to add `contains()` instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: queryset contains | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> This is a similar proposal to
> https://code.djangoproject.com/ticket/31561, but it is not the same.
> Currently using
> {{{
> x in myQuerySet
> }}}
> results in python using the fallback solution:
> https://docs.python.org/3/reference/expressions.html#membership-test-
> details
> Because https://groups.google.com/g/django-
> developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ deiced to implement contains in
> https://code.djangoproject.com/ticket/24141
> I think it is only consistent to have the same behavior implemented in
> `__contains__`. I would expect that, it is also a more efficient
> implementation and unifies django behavior. Nevertheless, documentation
> is needed why this inconsistency exists. I was not able to find a reason.
> Because the mailing list agreed on adding contains, this is discussed
> behavior. Why was `__contains__` not added in the first place? To not
> have breaking changes? I cannot see what would break.
>
> As said in https://code.djangoproject.com/ticket/31561 a queryset could
> be a collection to make typing easier. But this is not the intention of
> this issue.
New description:
This is a similar proposal to #31561, but it is not the same. Currently
using `x in myQuerySet` results in Python using the fallback solution:
https://docs.python.org/3/reference/expressions.html#membership-test-
details because https://groups.google.com/g/django-
developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ decided to implement contains in
#24141.
I think it is only consistent to have the same behavior implemented in
`__contains__`. I would expect that, it is also a more efficient
implementation and unifies Django behavior. Nevertheless, documentation is
needed why this inconsistency exists. I was not able to find a reason.
Because the mailing list agreed on adding contains, this is discussed
behavior. Why was `__contains__` not added in the first place? To not have
breaking changes? I cannot see what would break.
As said in #31561 a queryset could be a collection to make typing easier.
But this is not the intention of this issue.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: queryset contains | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
(Collaborator of @fidoriel here)
=== Background: Type Annotation
One intention of this issue is to improve the usability of type-
annotations and static type checking with django code. Due to the missing
`__contains__` method, `QuerySet`s do not fulfil the typing requirements
to be a `Container`, so passing them to a method that expects a
`Container` will fail static type checking. In consequence, they also do
not meet the requirements of `Collection` and `Sequence`. This makes type-
annotating methods that take, e.g., a list or a queryset, verbose und
unnecessarily specific, since you always have to type as "takes a
Container[x] OR a QuerySet[x]".
One could argue that this is a problem with Python's abstract base classes
and the fact that most ABCs inherit from `Container`, when most users
don't really care if `__contains__` is present or not. If it isn't, an
`element in our_container_like_object` test still is valid using the
fallback mechanism from above. However, a QuerySet semantically fulfills
all the requirements of `Container`, so I don't think this is a reason to
not make it one.
=== Past Discussion
The discussion in #31561 revolves around `Set` not being a suitable base
class because `Set`s do not have an order, but `QuerySet`s do. From what I
see, it does not give any points against making `QuerySet` a
`collections.abc.Collection` (by adding `__contains__`) in general.
In #24141, Carl Meyer said
> I think the original PR (to implement `QuerySet.__contains__()` as
`.filter(pk=obj.pk).exists()` under the hood) is a non-starter
which I understand as criticism towards this specific implementation
(`__contains__` will be `.filter(pk=obj.pk).exists()` under the hood,
which always hits the database, even if objects are already cached -- this
is unexpected in comparison to other helper methods that use the object
cache). I don't see any arguments against having `__contains__`
implemented here.
The discussion in https://groups.google.com/g/django-
developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ rejects the idea of implementing
`.__contains__()` identically to the current implementation of
`.contains()`, i.e., not evaluating the queryset. I don't see an argument
against having _some_ implementation of `__contains__`.
The discussion in https://github.com/django/django/pull/3906 concludes
that `x in qs` should always evaluate an unevaluated queryset. I don't see
an argument to not have `__contains()__`, just a requirement towards its
behavior.
I didn't find arguments against implementing `__contains__` in the
discussion of https://github.com/django/django/pull/13038.
Unless I'm missing something here, there's not really a point against
having `__contains__` in general. There are the arguments that
* a `__contains__` implementation shouldn't always hit the database if
objects are already cached (agree)
* `len(qs)`, `bool(qs)`, `x in qs` should be consistent in whether they
evaluate a queryset or not (agree)
* `x in qs` should evaluate the queryset (I guess debatable depending on
perspective, but not our goal in this ticket)
=== Proposal
In general, it is pythonic to test membership by using `in`. Currently,
the django documentation does not say what happens in this case, you have
to be aware of the elementwise-iteration-and-comparison fallback in python
and conclude that this iteration implies that the queryset will be
evaluated.
If we implemented `__contains__` as `evalute if not evaluted, then return
contains()`, we wouldn't have any changes in behavior, but `QuerySet`
would be a `Container`. Additionally, the documentation could include
this, e.g. at https://docs.djangoproject.com/en/5.0/ref/models/querysets
/#when-querysets-are-evaluated. I think both are desirable.
Thoughts / Did I miss something?
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: wontfix =>
* status: closed => new
Comment:
The type annotation use case presented in comment:3 didn’t come up in
previous discussion and cannot be satisfied by the addition of
`.contains()` rather than `.__contains__()`. This argument seems
compelling, so I hope I’m not overstepping by reopening for further
consideration.
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
(models, ORM) |
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => wontfix
Hello Anders, thank for your interest in making Django better but any
ticket closed as `wontfix` should not be reopened without a proper
discussion and agreement with the Django community. This process is
described [https://docs.djangoproject.com/en/stable/internals/contributing
/triaging-tickets/#closing-tickets in these docs]:
> Always use the forum or mailing list to get a consensus before reopening
tickets closed as “wontfix”.
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:5>