[Django] #31720: Better documentation and defaults for BoolOr and BoolAnd
Django
--------------------------------------------+------------------------
Reporter: Alex Scott | Owner: (none)
Type: Uncategorized | Status: new
Component: contrib.postgres | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
The only documentation available (that I found at least) for Postgres DB
functions is:
https://docs.djangoproject.com/en/3.0/ref/contrib/postgres/aggregates/
It would be really nice to have an example or two for each of those.
One issue I ran into with `BoolOr` when I was doing something like
`BoolOr(Q('someCharField') == Value('someVal'))` (which in itself would be
a great example to show), is an error:
`'Cannot resolve expression type, unknown output_field'`
Adding `output_field=BooleanField()` seemed to solve it but this wasn't
intuitive. Shouldn't the default output for `BoolOr` be boolean? If so,
it would be good to set it as such I think.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Alex Scott | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: (none) => nobody
* needs_docs: 0 => 1
* type: Uncategorized => Cleanup/optimization
* component: contrib.postgres => Database layer (models, ORM)
* stage: Unreviewed => Accepted
Comment:
A patch for both documentation and explicit `output_field` for `BoolOr`
and `BoolAnd` would be welcome. The issue you're experiencing here is
likely due to the fact `Q` and `WhereNode` are marked `conditional=True`
but don't set `output_field = models.BooleanField()` and thus the default
behavior of `Expression._resolve_output_field` cannot be leveraged.
Are you interested in submitting a PR with these improvements?
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:1>
Django
Reporter: Alex Scott | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alex Scott):
Thanks Simon for the quick response. I'm a bit of a n00b and have never
really contributed a PR to an open source project but maybe this could be
the first. Do you think setting the default is all that needs to be done?
I can try to take a look over this next week (and read about how to
contribute) and let you know if I think I'm capable.
Replying to [comment:1 Simon Charette]:
> A patch for both documentation and explicit `output_field` for `BoolOr`
and `BoolAnd` would be welcome. The issue you're experiencing here is
likely due to the fact `Q` and `WhereNode` are marked `conditional=True`
but don't set `output_field = models.BooleanField()` and thus the default
behavior of `Expression._resolve_output_field` cannot be leveraged.
>
> Are you interested in submitting a PR with these improvements?
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:2>
Django
Reporter: Alex Scott | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
> Do you think setting the default is all that needs to be done?
yeah, defining `BoolOr.output_field = models.BooleanField()` at the class
level (for `BoolAnd` as well) should be enough.
For the tests an extra method in
[https://github.com/django/django/blob/a8473b4d348776d823b7a83c1795279279cf3ab5/tests/postgres_tests/test_aggregates.py#L157
tests/postgres_tests/test_aggregates.py] for usage of `Q` objects for both
functions should be enough for each aggregate function.
e.g.
{{{#!python
def test_bool_or_q_object(self):
values =
AggregateTestModel.objects.aggregate(boolor=BoolOr(Q(integer_field__gt=2))
self.assertEqual(values, {'boolor': False})
}}}
I think examples in
[https://github.com/django/django/blob/a8473b4d348776d823b7a83c1795279279cf3ab5/docs/ref/contrib/postgres/aggregates.txt#L70-L84
the documentation] would also be welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:3>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Chorpash):
* owner: nobody => David Chorpash
* status: new => assigned
Comment:
I am interested in working this ticket. I know plenty of time has passed
since this was last modified, but I'll give it up to Alex if he wants to
give it a shot within the next day or so.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:4>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alex Scott):
Thanks David! Please do. Day job work got in the way of me taking a stab
at this and I'll continue to be busy for the next few weeks. So happy for
you to take it.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:5>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Chorpash):
* has_patch: 0 => 1
Comment:
Alex, I understand how that goes!
Simon, I submitted a [https://github.com/django/django/pull/13209/ PR] and
I wanted to make sure I set it up where I'm merging to the correct branch.
This is the first time submitting a PR for a ticket where the version was
**not** set to master.
Since the version is set to 3.0, I'm going to be merging into the
stable/3.1.x. Is that correct?
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:6>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Thanks for your PR Alex!
I left some comments on Github but you'll want to target the `master`
branch instead of `stable/3.1.x`, mergers will take care of the backport
if necessary.
I also think it might be worth documenting the usage of `Q` objects for
these aggregates and not solely boolean fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:7>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by David Chorpash):
Thank you for the feedback, Simon!
I fixed the PR so that it's targeting `master`. I also made adjustments
based on the comments and added documentation for `Q` objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:8>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_docs: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:9>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a2e621b14e85836362b7fc0e6b1bf7d7ff98e42b" a2e621b]:
{{{
#!CommitTicketReference repository=""
revision="a2e621b14e85836362b7fc0e6b1bf7d7ff98e42b"
Refs #31720 -- Added examples to BoolAnd() and BoolOr() documentation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:10>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6ec5eb5d74fd18a91256010706ab8b5b583526a9" 6ec5eb5]:
{{{
#!CommitTicketReference repository=""
revision="6ec5eb5d74fd18a91256010706ab8b5b583526a9"
Refs #31720 -- Defined default output_field of BoolAnd() and BoolOr()
aggregate functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:11>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"5360f002f8822ef2c43e4f3ecb1859765423ca61" 5360f00]:
{{{
#!CommitTicketReference repository=""
revision="5360f002f8822ef2c43e4f3ecb1859765423ca61"
[3.1.x] Refs #31720 -- Added examples to BoolAnd() and BoolOr()
documentation.
Backport of a2e621b14e85836362b7fc0e6b1bf7d7ff98e42b from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:12>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0a3c1272f2213ccd2b36081c8d587b3f33237a5d" 0a3c1272]:
{{{
#!CommitTicketReference repository=""
revision="0a3c1272f2213ccd2b36081c8d587b3f33237a5d"
[3.0.x] Refs #31720 -- Added examples to BoolAnd() and BoolOr()
documentation.
Backport of a2e621b14e85836362b7fc0e6b1bf7d7ff98e42b from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:13>
Django
Reporter: Alex Scott | Owner: David
Type: | Chorpash
Component: Database layer | Version: 3.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/31720#comment:14>