Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refs #23130 -- Allowed BooleanField to be null=True #9016

Closed
wants to merge 62 commits into from

Conversation

coilysiren
Copy link

@coilysiren coilysiren commented Sep 3, 2017

https://code.djangoproject.com/ticket/23130

Picks up were @timgraham left off in #8467

refs:

details:

  • BooleanField model field can take null=True
  • NullBooleanField is now just BooleanField(null=True)
  • depreciates NullBooleanField (this decision was made in @timgraham's PR)
  • normalizes boolean type casting, and incorporates distutils.util.strtobool
  • collects all the different boolean casting special cases into a single file django/utils/typecasting.py

still TODO:

  • there's at least a dozen tests failing <<< resolved
  • determine if this is a large enough PR to get added to AUTHORS ^_^ <<< call: no. the PR is much simpler without the typecasting.py change
  • check for NullBooleanField isinstance calls to turn into null=True
  • typecasting.py entrypoint functions need better names
  • evaluate typecasting.py nullable=[ True | False ] handling
  • add a few more cases to test_typecasting.py

timgraham and others added 30 commits June 9, 2017 12:26
@coilysiren
Copy link
Author

coilysiren commented Sep 3, 2017

✔️ tests passing

Solve the issue with Oracle check constraints. I think the appropriate check constraint for BooleanField(null=True) won't be generated because Field.get_internal_type() returns BooleanField rather than NullBooleanField. A possible fix is...

Implemented that fix! But also I feel this becomes less relevant if #9018 is merged first

As needed, continue adding tests for BooleanField(null=True) where NullBooleanField is tested.

So ^ is the last thing to do

Although I'm not sure to what extent adding those tests would be overkill, since NullBooleanField is now just

https://github.com/lynncyrin/django/blob/254d3149cb50016ae6e0746488b1042f5a05d8ec/django/forms/fields.py#L742-L748

class NullBooleanField(BooleanField):
    """
    A BooleanField that forces an optional NullBooleanSelect widget
    """

    def __init__(self, **kwargs):
        super().__init__(null=True, **kwargs)

@coilysiren coilysiren changed the title [WIP] Refs #23130 -- Allowed BooleanField to be null=True Refs #23130 -- Allowed BooleanField to be null=True Sep 3, 2017
@timgraham
Copy link
Member

  1. Yes, I think we can omit the change to get_internal_type() considering Unified NullBooleanField's and BooleanField's check constraints on Oracle. #9018.
  2. I don't think the form field changes to put NullBooleanField's logic in BooleanField is much of a simplification. Let's focus on the model field and leave the form fields alone for now.
  3. The idea with the additional tests is about the model field. For example, in tests/bulk_create/models.py there's a usage of NullBooleanField. A similar test for BooleanField(null=True) should be added there.

@coilysiren
Copy link
Author

coilysiren commented Sep 9, 2017

omit the change to get_internal_type() considering #9018

✔️

focus on the model field and leave the form fields alone for now

✔️

additional tests [for] the model field

WIP. I added the bulk_create one (it was obvious what to do there). I still need to appraise what adequate tests for be for:

  • postgres_tests
  • test_promises
  • serializers
  • test_booleanfield
  • managers_regress
  • generic_relations_regress
  • expressions_case
  • annotations

@coilysiren
Copy link
Author

In case I forget to make them, some optimization ticket ideas spawned from this PR are:

  • make all boolean type casting utilize distutils.util.strtobool, and collect form field and model field special cases into a single boolean type casting file
  • make NullBoolean model field BooleanField(null=True)
  • make NullBoolean form field BooleanField(null=True)

@timgraham
Copy link
Member

I incorporated some of these updates in #8467 which should be ready for review now. Thanks!

@timgraham timgraham closed this Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants