Skip to content

Make CharField(blank=True) not be considered nullable #39

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

Merged
merged 3 commits into from
Mar 5, 2019

Conversation

Naddiseo
Copy link
Contributor

@Naddiseo Naddiseo commented Mar 4, 2019

The documentation on blank says that it "will allow the entry of an empty value", which for a string is just a 0-length string. This patch allows CharField(blank=True,...) to no longer be considered Optional.

closes #38

Naddiseo and others added 2 commits March 4, 2019 12:57
The documentation on [blank](https://docs.djangoproject.com/en/2.1/ref/models/fields/#blank) says that it "will allow the entry of an empty value", which for a string is just a 0-length string. This patch allows `CharField(blank=True,...)` to no longer be considered `Optional`.

closes typeddjango#38
@Naddiseo
Copy link
Contributor Author

Naddiseo commented Mar 4, 2019

Hmm, from my understanding, the django test that failed /should/ fail type checking...
@mkurnikov, not sure what to do about that one.

@mkurnikov
Copy link
Member

mkurnikov commented Mar 4, 2019

https://github.com/django/django/blob/master/django/db/models/fields/__init__.py#L620
https://github.com/django/django/blob/master/django/core/validators.py#L13

I don't know.

From my understanding of the code, blank=False should disable nullability, if null=True. But there's nothing about blank=True.
You can just add this line to IGNORED_ERRORS for this specific module.

@Naddiseo
Copy link
Contributor Author

Naddiseo commented Mar 4, 2019

Hmm, interesting. However, allowing a field that doesn't have null=True to be None can cause the database to throw an IntegrityError, eg:

class MyModel(models.Model):
   field = models.CharField(max_length=1, blank=True)
MyModel(field=None).save()
# raises IntegrityError (in mysql): (1048, "Column 'field' cannot be null")

@mkurnikov
Copy link
Member

mkurnikov commented Mar 4, 2019

I think blank=True should give nullability in the __init__ method, and fail in create(). And reveal_type should have non-optional type.
So, your PR is almost right, could you add special casing in extract_expected_types similar to https://github.com/mkurnikov/django-stubs/blob/master/mypy_django_plugin/transformers/init_create.py#L169
and add test for that?

Then, there will be no need for additional line in IGNORED_ERRORS, and behaviour you mentioned will be achieved.

@Naddiseo
Copy link
Contributor Author

Naddiseo commented Mar 5, 2019

Let me know if you want me to format it differently, or move the test cases into say model_init.test or nullable_fields.test, but I think code-wise it should do what we're expecting.

@mkurnikov
Copy link
Member

Looks good! The only thing, could be make two tests out of those three - one for blank=True, null=False, and one for blank=True, null=True?
Tests are very slow, trying to keep their numbers low.

@mkurnikov
Copy link
Member

Thanks!

@mkurnikov mkurnikov merged commit f7dfbef into typeddjango:master Mar 5, 2019
@TonyRippy TonyRippy mentioned this pull request Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CharField with blank=True should not be considered nullable
3 participants