-
Notifications
You must be signed in to change notification settings - Fork 44
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
Don't set null on M2M fields #28
Conversation
# check for M2M fields setting if we set null to True. Prevent | ||
# those warnings by setting null only for non-M2M fields. | ||
if django.VERSION < (1, 8) or not field.many_to_many: | ||
field.null = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting many to many fields null had no effect before Django 1.8, correct? Maybe just make this if not field.many_to_many:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting many to many fields null had no effect before Django 1.8, correct?
That is correct and this PR leaves the Django < 1.8 behavior the same. The reason for the Django version test is that many_to_many
is an attribute that was added to django.db.models.fields.Field
in Django 1.8. I'm hoping Django 1.7 is not long for this world so hopefully this can merge as is and then the Django version conditional can be removed when 1.7 support is dropped.
@craigds Any thoughts on this change? |
sorry. looks good. I think the addition to the docs is probably unnecessary, since null=True makes no sense for M2M fields anyway. |
Sounds good to me, reverted the doc change in 6a6c750 |
Don't set null on M2M fields. Thanks @naegelyd and @lucaswiman
Thanks for the review and merge @craigds! |
thank you :) |
This PR changes the behavior of
django-typed-models
so that thenull
attribute is not set on M2M fields. Currently,django-typed-models
wants to setnull=True
for all fields. Looking at the README, we see this is marked as a limitation:Django, however, recognizes that setting
null=True
has no effect on M2M fields. When an M2M field hasnull=True
, the Django system check will generate warnings of the form:To avoid these warnings in applications that use
django-typed-model
, this PR updates the original behavior to ignore M2M fields. For all non-M2M fields,null
is still set to true. If, however, a field is M2M, thenull
attribute is left alone.