Re: [Django] #35154: QuerySet implements `contains` but not `__contains__`

[Django]

#35154: QuerySet implements `contains` but not `__contains__`
-+-
 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
-+-
Changes (by Natalia Bidart):

 * resolution:   => wontfix
 * status:  new => closed

Comment:

 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018eec5255e3-6428d4f8-a307-40bd-b4ca-43b1232f3e4c-00%40eu-central-1.amazonses.com.

Re: [Django] #35154: QuerySet implements `contains` but not `__contains__`

[Django]

#35154: QuerySet implements `contains` but not `__contains__`
-+-
 Reporter:  fidoriel |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  5.0
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 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
-+-
Changes (by Anders Kaseorg):

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018eea5bc8ba-1ae5a1db-9382-46e6-9e62-e24d0d26311c-00%40eu-central-1.amazonses.com.

Re: [Django] #35154: QuerySet implements `contains` but not `__contains__`

[Django]

#35154: QuerySet implements `contains` but not `__contains__`
-+-
 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
-+-
Comment (by Richard Ebeling):

 (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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" 

Re: [Django] #35154: QuerySet implements `contains` but not `__contains__`

[Django]

#35154: QuerySet implements `contains` but not `__contains__`
-+-
 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
-+-
Description changed by Tim Graham:

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018d5a4f7fa6-4c473ddd-f5da-409b-869d-cc82d1905612-00%40eu-central-1.amazonses.com.

Re: [Django] #35154: QuerySet implements `contains` but not `__contains__`

[Django]

#35154: QuerySet implements `contains` but not `__contains__`
-+-
 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
-+-
Changes (by Mariusz Felisiak):

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018d594e4980-50adc9ea-fd15-4c41-9d92-4c6f466187c5-00%40eu-central-1.amazonses.com.