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

Allow None return form model Field.formfield() #2363

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

adamchainz
Copy link
Contributor

@adamchainz adamchainz requested a review from intgr September 9, 2024 13:52
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! TIL

@sobolevn sobolevn merged commit 0efa35d into master Sep 9, 2024
36 checks passed
@sobolevn sobolevn deleted the formfield_may_return_none branch September 9, 2024 14:08
@@ -205,7 +205,7 @@ class Field(RegisterLookupMixin, Generic[_ST, _GT]):
form_class: type[forms.Field] | None = None,
choices_form_class: type[forms.ChoiceField] | None = None,
**kwargs: Any,
) -> forms.Field: ...
) -> forms.Field | None: ...

Choose a reason for hiding this comment

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

hmmm so this is triggering a bunch of false positives after upgrading django-stubs

it seems to me that django.db.models.fields.Field.formfield always returns a Field so this annotation is wrong (?) and subclasses should override it to be | None if applicable (?) (even though it would violate substitutability -- but such is django)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you read the associated ticket and docs change? Any Field subclass may return None from formfield() to remove it from forms.

In Django itself, AutoFieldMixin and OneToOneField use this feature. I have used this feature at least once in my MySQL package: https://github.com/adamchainz/django-mysql/blob/1ba0471c06bf7e3b47a1ee2c544fc3738020611e/src/django_mysql/models/fields/dynamic.py#L295-L299

Adding the None option the base class fixes substitutability, no?

Choose a reason for hiding this comment

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

yes but fields.Field does not return None, and my subclass of fields.Field inherits the same implementation which does not return None -- and now the type checker is mad about that

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.. I'm happy adding assert formfield is not None to such tests. That's just type-hinted life, in my experience.

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.

3 participants