[Django] #32971: Internal Field.check() methods don't need to construct and return lists
Django
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I noticed that many of the internal "check()" methods used by `Field`
subclasses return lists (including the empty list) when they can just
yield items (or yield nothing). This is because the concrete `check()`
implementations unpack the return value when constructing a new list. See
here for one example of such a `check()` method:
https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L196-L205
The proposed change, then, would be to change methods like the following
to yield their items instead of creating and returning a list:
https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L243-L254
--
Ticket URL: <https://code.djangoproject.com/ticket/32971>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(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
-------------------------------------+-------------------------------------
Old description:
> I noticed that many of the internal "check()" methods used by `Field`
> subclasses return lists (including the empty list) when they can just
> yield items (or yield nothing). This is because the concrete `check()`
> implementations unpack the return value when constructing a new list. See
> here for one example of such a `check()` method:
> https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L196-L205
>
> The proposed change, then, would be to change methods like the following
> to yield their items instead of creating and returning a list:
> https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L243-L254
New description:
I noticed that many of the internal "check()" methods used by `Field`
subclasses return lists (including the empty list) when they can just
yield items (or yield nothing). This is because the concrete `check()`
implementations unpack the return value when constructing a new list. See
here for one example of such a `check()` method:
https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L196-L205
The proposed change, then, would be to change methods like the following
to yield their items instead of creating and returning a list:
https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L243-L254
It looks like there are 23 methods in that file with a name starting with
`_check...`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Actually, it looks like this observation may apply to the entire system
check framework. Currently, it looks like lists are constructed at every
layer in nested fashion, when it appears that it would work equally well
for items to be yielded up the stack.
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:2>
Django
every layer
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* component: Database layer (models, ORM) => Core (System checks)
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:3>
Django
every layer
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Core (System | Version: dev
checks) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Thanks for this proposition, however it's
[https://docs.djangoproject.com/en/3.2/topics/checks/ documented] that
check function must return a **list** of messages. Also, folks can extend
checks at any level, see
[https://docs.djangoproject.com/en/3.2/topics/checks/#field-model-manager-
and-database-checks examples]. I don't think it's worth backward
compatibility concerns.
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:4>
Django
every layer
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
> Thanks for this proposition, however it's documented that check
function must return a list of messages. Also, folks can extend checks at
any level, see examples. I don't think it's worth backward compatibility
concerns.
I was talking also about the internal check methods (e.g. the ones whose
names start with an underscore), which are called by the public methods.
Here is an example:
https://github.com/django/django/blob/f331eba6d576752dd79c4b37c41d981daa537fe6/django/db/models/fields/__init__.py#L198-L204
You can see the public method calling several internal methods --
unpacking each of their results to create the new list. Each internal
layer adds unnecessary list creations.
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:5>
Django
every layer
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:5 Chris Jerdonek]:
> You can see the public method calling several internal methods --
unpacking each of their results to create the new list. Each internal
layer adds unnecessary list creations.
True, but I don't think that's a big issue. I'd like to keep internal
methods consistent with what we have in docs (we recommend creating
internal `_check...` methods which use this pattern).
--
Ticket URL: <https://code.djangoproject.com/ticket/32971#comment:6>