Re: [Django] #33724: Changing from list to set in `exclude` raises errors, and is not documented.
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):
Replying to [comment:1 Mariusz Felisiak]:
> ...
> > What is the best written code to change the line `exclude +=
['username', 'slug']` in my code? Is it exclude `|= set(['username',
'slug'])` or `exclude |= {'username', 'slug'}`? Or should I convert to
list and then back to set?
>
> Personally I would use `exclude |= {'username', 'slug'}` or
> {{{
> exclude.add('username')
> exclude.add('slug')
> }}}
Thank you. Should I assume `exclude` is a set from Django 4.1, or should I
convert it to set? Especially since in Django 4.0 it's a list.
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> Thank you. Should I assume `exclude` is a set from Django 4.1, or should
I convert it to set? Especially since in Django 4.0 it's a list.
`exclude` can be a list or set, it depends from where `clean_fields()` is
called, so it's safer to handle both types, e.g.
{{{#!python
def clean_fields(self, exclude=None):
if exclude is None:
exclude = set()
else:
exclude = set(exclude)
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:3>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):
Replying to [comment:3 Mariusz Felisiak]:
> `exclude` can be a list or set, it depends from where `clean_fields()`
is called, so it's safer to handle both types, e.g.
> {{{#!python
> def clean_fields(self, exclude=None):
> if exclude is None:
> exclude = set()
> else:
> exclude = set(exclude)
> ...
> }}}
Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:4>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):
Hi, just to let you know - if I either convert `exclude` to a set or
convert it to a list, and then add my values, then it works with all
versions of Django. So the problem was just that I assumed it's a list,
instead of converting it to a set or list. Maybe it's better to recommend
it in the documentation - convert `exclude` to a set or list if it's not
`None`.
Commit - https://github.com/speedy-net/speedy-
net/commit/c76c82bc0b40ed06f11a27cd6f0e4deab2a70d47
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:5>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Still I wonder if the performance gain is worth the loss of consistency. I
wouldn't mind if exclude was defined to always be a set, but having to
code defensively by expecting list or set is a loss in my point of view.
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:6>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):
Replying to [comment:6 Claude Paroz]:
> Still I wonder if the performance gain is worth the loss of consistency.
I wouldn't mind if exclude was defined to always be a set, but having to
code defensively by expecting list or set is a loss in my point of view.
I think logically it should be a set, because we don't need the same value
twice and the order doesn't matter. But we should decide how to handle old
code where it was a list. Maybe define it a set from now on, and write in
the documentation that it must be a set. But I don't see any problem with
including the following 4 lines every time we use `exclude`:
{{{
if exclude is None:
exclude = set()
else:
exclude = set(exclude)
}}}
By the way, since these 4 lines are repeated, maybe we can define a util
function that takes `exclude` as an argument, and return a set. Either
`set()` (if `exclude is None`) or `set(exclude)` otherwise.
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:7>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):
Commit - https://github.com/speedy-net/speedy-
net/commit/d0cd6791b717a9e3c874fe6ea489032830617030
I defined a `convert_to_set` util function. If you want you can take this
function and use it in Django. Or maybe add it to the documentation that
users can use it to convert `exclude` to a set (with one line).
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:8>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
Comment:
Replying to [comment:6 Claude Paroz]:
> Still I wonder if the performance gain is worth the loss of consistency.
I wouldn't mind if exclude was defined to always be a set, but having to
code defensively by expecting list or set is a loss in my point of view.
All built-in methods do exactly the same number of conversion as before
1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9 just using `set(...)` instead of
`list(...)`. IMO we can document `set` as the preferable option, e.g.
{{{#!diff
diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt
index 9aa9486f4a..8601a19e64 100644
--- a/docs/ref/models/instances.txt
+++ b/docs/ref/models/instances.txt
@@ -252,9 +252,9 @@ The first step ``full_clean()`` performs is to clean
each individual field.
.. method:: Model.clean_fields(exclude=None)
This method will validate all fields on your model. The optional
``exclude``
-argument lets you provide a list of field names to exclude from
validation. It
-will raise a :exc:`~django.core.exceptions.ValidationError` if any fields
fail
-validation.
+argument lets you provide a ``set`` of field names to exclude from
validation.
+It will raise a :exc:`~django.core.exceptions.ValidationError` if any
fields
+fail validation.
The second step ``full_clean()`` performs is to call
:meth:`Model.clean()`.
This method should be overridden to perform custom validation on your
model.
@@ -355,8 +355,8 @@ uniqueness constraints defined via
:attr:`.Field.unique`,
:attr:`.Field.unique_for_date`, :attr:`.Field.unique_for_month`,
:attr:`.Field.unique_for_year`, or :attr:`Meta.unique_together
<django.db.models.Options.unique_together>` on your model instead of
individual
-field values. The optional ``exclude`` argument allows you to provide a
list of
-field names to exclude from validation. It will raise a
+field values. The optional ``exclude`` argument allows you to provide a
``set``
+of field names to exclude from validation. It will raise a
:exc:`~django.core.exceptions.ValidationError` if any fields fail
validation.
:class:`~django.db.models.UniqueConstraint`\s defined in the
@@ -380,7 +380,7 @@ Finally, ``full_clean()`` will check any other
constraints on your model.
This method validates all constraints defined in
:attr:`Meta.constraints <django.db.models.Options.constraints>`. The
-optional ``exclude`` argument allows you to provide a list of field names
to
+optional ``exclude`` argument allows you to provide a ``set`` of field
names to
exclude from validation. It will raise a
:exc:`~django.core.exceptions.ValidationError` if any constraints fail
validation.
diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt
index 7b922256ec..a1ec53e2ac 100644
--- a/docs/releases/4.1.txt
+++ b/docs/releases/4.1.txt
@@ -561,6 +561,10 @@ Miscellaneous
``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are
moved to ``django.contrib.admindocs.utils``.
+* Custom :meth:`.Model.clean_fields`, :meth:`.Model.validate_unique`, and
+ :meth:`.Model.validate_constraints` methods now convert an ``exclude``
+ value to the ``set``.
+
.. _deprecated-features-4.1:
}}}
Also, passing lists still works, but would not be documented anymore. What
do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:9>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
> All built-in methods do exactly the same number of conversion as before
1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9 just using set(...) instead of
list(...).
The problem is not in Django code, the problem is in user code when you
have to do smart custom things with exclude, and you don't know if you get
a list or a set, so you are forced to cast the variable each time before
starting to manipulate it. If different sort of types in the same category
(typically iterables) can be seen as a feature, I don't like the
`list`/`set` potential mix.
> IMO we can document set as the preferable option, e.g.
+1, I think this is going in the right direction.
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:10>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Yes, agreed +1 (since you CC'd me :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:11>
Django
documented.
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mariusz Felisiak
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15726 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:12>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:13>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: Mariusz
| Felisiak
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"90aabd730a2a434c227faf8a927b0e2ccd67e291" 90aabd73]:
{{{
#!CommitTicketReference repository=""
revision="90aabd730a2a434c227faf8a927b0e2ccd67e291"
Fixed #33724 -- Doc'd exclude argument changes in model validation.
Thanks אורי for the report.
Follow up to 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:14>
Django
documented.
-------------------------------------+-------------------------------------
Reporter: אורי | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3d4bab28de16c56e7fcd94a26a570f5bc730e691" 3d4bab2]:
{{{
#!CommitTicketReference repository=""
revision="3d4bab28de16c56e7fcd94a26a570f5bc730e691"
[4.1.x] Fixed #33724 -- Doc'd exclude argument changes in model
validation.
Thanks אורי for the report.
Follow up to 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9.
Backport of 90aabd730a2a434c227faf8a927b0e2ccd67e291 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33724#comment:15>