#31369 closed Cleanup/optimization (fixed)
Deprecate the model NullBooleanField.
Reported by: | David Smith | Owned by: | Hristiyan Ivanov |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.0 |
Severity: | Normal | Keywords: | |
Cc: | Timothy Schilling | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
#29227 allowed BooleanField
to be null=true, a step towards deprecating NullBooleanField
. #27917 also mentions an aim to deprecate this field.
Comments on the PR discussed deprecating NullBooleanField
with the conclusion being that we should wait a while. Now that we are almost 2 years on, and with Django 4.0 being sometime away I suggest now is a good time to revisit this decision.
This ticket therefore proposes we deprecate NullBooleanField
. It would be good to also tidy up the associated fields / widgets e.g. #23681 discussed deprecation of NullBooleanSelect
Note: I was slightly unsure if this is best raised as a ticket or as a topic on the mailing list.
Change History (15)
comment:1 by , 5 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 5 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Support on the mailing list, so let's progress. Thanks David.
comment:3 by , 5 years ago
Has patch: | set |
---|
comment:4 by , 5 years ago
Has patch: | unset |
---|
comment:5 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 5 years ago
"Deprecate NullBooleanField" is a bit of a broad naming. If I understand correctly, for now just adding a "NullBooleanField has been deprecated." message should be enough, and later on removing the field in favor of the BooleanField.
system_check_deprecated_details = { 'msg': ( 'NullBooleanField has been deprecated. Support ' 'for it (except in historical migrations) will be removed ' 'in Django 4.0.' ), 'hint': ( 'Use BooleanField(null=true) instead.' ), 'id': 'fields.W903', }
comment:7 by , 5 years ago
Hi hristiy4n,
Thanks for picking this up.
I had a brief look at the code before raising the issue but thought the principle was important to agree before lots of detailed work. I agree a deprecation message is key to this. I also think a bit more thought at this stage would be beneficial in the long run. I'd suggest that 'just' issuing a deprecation warning at this stage we may uncover something later which is tricky to resolve.
Here are my more detailed notes having spent some time looking at it in more detail.
There are three elements which we should consider deprecating. Is it just the first one at this stage, the top two (I think this is the intent), or a bit more of a wider tidy up and go for all three?
- There is nullboolean model field
- and nullbooleanfield field
- and the associated widget. Note this widget is currently used when
null=True
My main concern about just adding a deprecation notice is that the two BooleanField
s work slightly differently.
The models.fields.BooleanField
acts more like the forms.fields.NullBooleanField
than forms.fields.BooleanField
. Whilst I think the preference should be to deprecate both NullBooleanField
s do we need to change the behaviour of forms.fields.BooleanField
to mirror models.fields.BooleanField
when null=True
.
I hope my code sample below, explains it more clearly that what I've written out above!
>>> from django.forms.fields import NullBooleanField >>> f = NullBooleanField() >>> f.clean("True") True >>> f.clean("") # returns null >>> f.clean(False) False >>> from django.forms.fields import BooleanField >>> f = BooleanField() >>> f.required = False >>> f.clean(True) True >>> f.clean("") False >>> f.clean(False) False >>> from django.db.models import fields >>> f = fields.BooleanField(null=True) >>> f.blank = True >>> f.clean(True, "test") True >>> f.clean("", "test") # returns null >>> f.clean(False, "test") False
comment:8 by , 5 years ago
I had a quick look at the code myself and I think I understand the problem you are raising. The behavior of the BooleanField
s is indeed different, which is due to forms.BooleanField
's to_python()
method as it uses bool(value)
if the value is not False.
I do agree that if models.NullBooleanField
is getting deprecated, forms.NullBooleanField
should also be deprecated and that just adding the deprecation message will introduce work later on, as the forms.BooleanField
will need to be changed.
I will have a look at the widgets that the two form fields are using to see if there is any substantial difference and if not, I guess we can alter the widgets.CheckboxInput
and also deprecate widgets.NullBooleanSelect
, as well as improve the to_python()
method of forms.BooleanField
and therefor deprecate all three NullBooleanField
related items.
comment:9 by , 5 years ago
Cc: | added |
---|---|
Has patch: | set |
I made an attempt at the deprecation of both NullBooleanField
's. I left NullBooleanSelect
alone for the time being as it seems like a much larger change. If that's a show stopper, I can take a swing at that as well.
comment:13 by , 5 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Summary: | Deprecate NullBooleanField → Deprecate the model NullBooleanField. |
We agree on the mailing list that we should deprecate only the model NullBooleanField
.
I've posted to the mailing list to get feedback. https://groups.google.com/d/topic/django-developers/pSvFXcUUkRs/discussion
I'm inclined to Accept, but let's see what folks say.
(I'll close as needsinfo for the moment.)