Fix BooleanField's allow_null behavior by math-a3k · Pull Request #8614 · encode/django-rest-framework (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

math-a3k

@math-a3k

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

adamchainz

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, right now if you want to migrate and preserve the initial value being None, you need NullBooleanField(allow_null=True, initial=None) ? I don't think we need to change BooleanField for this.

I don't quite follow why the default_empty_html change is needed - it doesn't look like NullBooleanField ever had that:

class NullBooleanField(BooleanField):
initial = None
def __init__(self, **kwargs):
warnings.warn(
"The `NullBooleanField` is deprecated and will be removed starting "
"with 3.14. Instead use the `BooleanField` field and set "
"`allow_null=True` which does the same thing.",
RemovedInDRF314Warning, stacklevel=2
)
assert 'allow_null' not in kwargs, '`allow_null` is not a valid option.'
kwargs['allow_null'] = True
super().__init__(**kwargs)
@@ -720,6 +720,12 @@ class BooleanField(Field):
}
NULL_VALUES = {'null', 'Null', 'NULL', '', None}
def __init__(self, **kwargs):
if 'allow_null' in kwargs:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if allow_null=False?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it should be if it's there and it's true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, right now if you want to migrate and preserve the initial value being None, you need NullBooleanField(allow_null=True, initial=None) ? I don't think we need to change BooleanField for this.

That's the problem, NullBooleanField (NBF) is deprecated but the current implementation of BooleanField does not fully support null values. NBF will be removed from Django eventually and DRF won't be aligned.

I don't quite follow why the default_empty_html change is needed - it doesn't look like NullBooleanField ever had that:

Because otherwise it will be treated as False instead of None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I meant to write:

As I understand, right now if you want to migrate and preserve the initial value being None, you need BooleanField(allow_null=True, initial=None) ? I don't think we need to change BooleanField for this.

That, is, to migrate from NullBooleanField precisely, it seems like you can use just BooleanField(allow_null=True, initial=None).

NBF will be removed from Django eventually and DRF won't be aligned.

NullBooleanField was already removed, in version 3.14: b658915#diff-7768b980611e24de782acc70ad64cd57b9d169d42f80b200cb7c6ef277fd74dbL715

Because otherwise it will be treated as False instead of None

I understnad this, but default_empty_html was never set from the default in NullBooleanField. See the above linked removal. To set it in BooleanField would actually be backwards incompatible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@math-a3k

@math-a3k

auvipy

@@ -690,6 +690,12 @@ class BooleanField(Field):
}
NULL_VALUES = {'null', 'Null', 'NULL', '', None}
def __init__(self, **kwargs):
if kwargs.get('allow_null', False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allow null false or true? can you elaborate please

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML checkboxes do not send any value and should be treated
as None by BooleanField if allow_null is True. from a comment of the tests in this PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If allow_null is true (false if not set) then set those default values, that was the problem,. If not set, It was consideres as false on booleanfield, It needs another widget to have the there values (none, true, false). As nullboolean field addesed that but It has been deprecated, the current booleanfield treats unexistant as false, the fix correcta that as show in the tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for compatibility reason we can consider this PR. just have to carefully handle any breaking or compatibility breaking changes. may be we can just mention that on release notes later.

This was referenced

Dec 4, 2022

@math-a3k

Great! There is no compatibility issues that i detected as all testsuite pass and its a fix for an intended behaviour, im surprised anyone hasnt stumble upon It so far El dom., 4 dic. 2022 13:52, Asif Saif Uddin ***@***.***> escribió:

…

***@***.**** commented on this pull request. ------------------------------ In rest_framework/fields.py <#8614 (comment)> : > @@ -690,6 +690,12 @@ class BooleanField(Field): } NULL_VALUES = {'null', 'Null', 'NULL', '', None} + def __init__(self, **kwargs): + if kwargs.get('allow_null', False): I think for compatibility reason we can consider this PR. just have to carefully handle any breaking or compatibility breaking changes. may be we can just mention that on release notes later. — Reply to this email directly, view it on GitHub <#8614 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHQPBRQ32XXPUHX5YYXKPTDWLTD3FANCNFSM565ARHZQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

auvipy

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I am convinced with the suggested change

@auvipy

Great! There is no compatibility issues that i detected as all testsuite pass and its a fix for an intended behaviour, im surprised anyone hasnt stumble upon It so far El dom., 4 dic. 2022 13:52, Asif Saif Uddin @.> escribió: … @.* commented on this pull request. ------------------------------ In rest_framework/fields.py <#8614 (comment)> : > @@ -690,6 +690,12 @@ class BooleanField(Field): } NULL_VALUES = {'null', 'Null', 'NULL', '', None} + def init(self, kwargs): + if kwargs.get('allow_null', False): I think for compatibility reason we can consider this PR. just have to carefully handle any breaking or compatibility breaking changes. may be we can just mention that on release notes later. — Reply to this email directly, view it on GitHub <#8614 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHQPBRQ32XXPUHX5YYXKPTDWLTD3FANCNFSM565ARHZQ . You are receiving this because you authored the thread.Message ID: @.*>

Hi Rodrigo, if you think we should document the change here let me know or feel free to come with a PR.

ml-taehoon-choi added a commit to banhala/django-rest-framework that referenced this pull request

May 12, 2023

@ml-taehoon-choi