Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-16 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Ian Foote):

 Replying to [comment:9 jonathan-golorry]:
 > Unfortunately we can't use kwargs for Q objects with multiple children.
 >
 > `(Q(x=1) &  Q(x=2)).deconstruct()` would lose an argument.

 Excellent point!

-- 
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/074.fbd9caebd1bdc587212b6cef5c9278b3%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-16 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by jonathan-golorry):

 Unfortunately we can't use kwargs for Q objects with multiple children.

 `(Q(x=1) &  Q(x=2)).deconstruct()` would lose an argument.

-- 
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/074.69358cd0ffcc71a9bb56f9e6ba5994f7%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-16 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Ian Foote):

 I like the consistency and simplicity of the reworked {{{deconstruct}}}
 method. I think removing weird edge-cases in {{{Q}}} is a good thing.

 I think I would personally prefer a deconstruct api that always uses
 kwargs where possible, rather than args:

 {{{('django.db.models.Q', (), {'x': 1, 'y': 2})}}} looks nicer than
 {{{('django.db.models.Q', (('x', 1), ('y', 2)), {})}}} to me.

 I don't know how much harder this would be to implement though, and it's a
 machine facing interface, so human readability isn't the highest priority.

-- 
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/074.b73b4982df08b8287030adbf410740b4%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 Ian, can I ask for your opinion? We need another pair of eyes, I really
 don't see why the current format of `deconstruct()` is problematic 路.

-- 
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/074.73a5a5b6e20640620300a53c204a%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by jonathan-golorry):

 I suppose it's a semantics argument whether `Q(Exists...)` is untested if
 there's a test that runs that exact expression, but isn't solely checking
 that functionality.

 My point is that `Q(("x", 1))` and `Q(x=1)` are equivalent, so it's
 impossible for the deconstruct to correctly recreate the original args and
 kwargs in all cases. Therefore, unless there's an argument for keeping the
 special case, it's better to consistently use args for both `Q(x=1)` and
 `Q(x=1, y=2)`.

 I point out `Q(Exists...) | Q(Q())` to show that the fragility of the
 special case is problematic and hard to catch. An internal optimization
 for nested empty Q objects can cause conditional expression combination to
 fail. That's why I'd like this patch to be focused on removing the special
 case and making Q objects more robust for all inputs, rather than only
 adding support for expressions. Both would make my future work on Q
 objects possible, but the current patch would put django in a better
 position for future development.

-- 
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/074.139c4de4b81d4247699295297f5a12fd%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 > Django already passes conditional expressions to Q objects internally.
 https://github.com/django/django/blob/main/django/db/models/expressions.py#L113
 >
 > Tested here
 https://github.com/django/django/blob/main/tests/expressions/tests.py#L827

 These are example of combining conditional expressions. Again,
 initializing `Q` objects with conditional expressions is undocumented and
 untested.

 > That test is only succeeding because `Q(...) | Q(Q())` is treated
 differently from `Q(...) | Q()`.

 I'm not sure how it's related with the ticket 樂

 > As for the form of `.deconstruct()`, is there any reason for keeping the
 special case? It's:
 >
 > 1. Inconsistent: `Q(x=1).deconstruct()` vs `Q(x=1, y=2).deconstruct()`

 First is a single condition without a connector.

 > 2. Fragile: Unsupported inputs like `Q(False)` sometimes (but not
 always!) lead to "not subscriptable" errors.
 > 3. Incorrect: `Q(("x", 1)).deconstruct()` incorrectly puts the condition
 in  kwargs instead of args.

 I wouldn't say that is incorrect `Q(('x', 1))` is equivalent to the
 `Q(x=1)` so I don't see anything wrong with this behavior.

-- 
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/074.32e143d9c227c539100be5321ac1fb75%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by jonathan-golorry):

 Django already passes conditional expressions to Q objects internally.
 https://github.com/django/django/blob/main/django/db/models/expressions.py#L113

 Tested here
 https://github.com/django/django/blob/main/tests/expressions/tests.py#L827

 That test is only succeeding because `Q(...) | Q(Q())` is treated
 differently from `Q(...) | Q()`.

 As for the form of `.deconstruct()`, is there any reason for keeping the
 special case? It's:

 1. Inconsistent: `Q(x=1).deconstruct()` vs `Q(x=1, y=2).deconstruct()`
 2. Fragile: Unsupported inputs like `Q(False)` sometimes (but not always!)
 lead to "not subscriptable" errors.
 3. Incorrect: `Q(("x", 1)).deconstruct()` incorrectly puts the condition
 in  kwargs instead of args.

-- 
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/074.a56d84f208baaab6154bd3a07b92214a%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q().

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Nick Pope):

 As `Q()` has `.conditional` set to `True`, we can amend Mariusz' example
 above to the following:
 {{{#!diff
 diff --git a/django/db/models/query_utils.py
 b/django/db/models/query_utils.py
 index ae0f886107..5dc71ad619 100644
 --- a/django/db/models/query_utils.py
 +++ b/django/db/models/query_utils.py
 @@ -85,7 +85,7 @@ class Q(tree.Node):
  if path.startswith('django.db.models.query_utils'):
  path = path.replace('django.db.models.query_utils',
 'django.db.models')
  args, kwargs = (), {}
 -if len(self.children) == 1 and not isinstance(self.children[0],
 Q):
 +if len(self.children) == 1 and getattr(self.children[0],
 'conditional', False) is False:
  child = self.children[0]
  kwargs = {child[0]: child[1]}
  else:
 }}}

-- 
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/074.a601f6a198d8841363a21305d91f7765%40djangoproject.com.

Re: [Django] #32548: Support passing conditional expressions to Q(). (was: Remove special kwarg deconstruct case for Q objects with 1 child)

2021-03-15 Thread Django 
#32548: Support passing conditional expressions to Q().
-+-
 Reporter:  jonathan-golorry |Owner:  jonathan-
 Type:   |  golorry
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Q objects,   | Triage Stage:  Accepted
  deconstruct|
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak):

 * status:  new => assigned
 * cc: Ian Foote, Matthew Schinckel (added)
 * needs_better_patch:  0 => 1
 * needs_tests:  0 => 1
 * owner:  nobody => jonathan-golorry
 * stage:  Unreviewed => Accepted


Comment:

 Conditional expressions can be combined together, so it's not necessary to
 encapsulate `Exists()` with `Q()`. Moreover it's an undocumented and
 untested to pass conditional expressions to `Q()`. Nevertheless I think it
 makes sense to support this.

 There is no need to change the current format of `deconstruct()`, it
 should be enough to handle conditional expressions, e.g.
 {{{
 diff --git a/django/db/models/query_utils.py
 b/django/db/models/query_utils.py
 index ae0f886107..5dc71ad619 100644
 --- a/django/db/models/query_utils.py
 +++ b/django/db/models/query_utils.py
 @@ -85,7 +85,7 @@ class Q(tree.Node):
  if path.startswith('django.db.models.query_utils'):
  path = path.replace('django.db.models.query_utils',
 'django.db.models')
  args, kwargs = (), {}
 -if len(self.children) == 1 and not isinstance(self.children[0],
 Q):
 +if len(self.children) == 1 and not isinstance(self.children[0],
 Q) and getattr(self.children[0], 'conditional', False) is False:
  child = self.children[0]
  kwargs = {child[0]: child[1]}
  else:
 }}}

-- 
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/074.7da03ab99a820a068c9a4154ac29d429%40djangoproject.com.