openedx / openedx-learning

GNU Affero General Public License v3.0
5 stars 8 forks source link

Nullable MultiCollationTextField type-checking #152

Open kdmccormick opened 5 months ago

kdmccormick commented 5 months ago

Context: https://github.com/openedx/openedx-learning/pull/149#issuecomment-1930203807

Setting null=True on a Django TextField or CharField successfully tells mypy that the field's value can be None:

class MyModel(models.Model):
    text = TextField(..., null=True)

foo = MyModel(text=None)  # all good

But setting null=True this repo's custom MultiCollation(Text|Char)Field, which is a subclass of Django's (Text|Char)Field, does not lead mypy to the same conclusion:

class MyModel(models.Model):
    text = MultiCollationTextField(..., null=True)

foo = MyModel(text=None)  # error: Incompatible type for "text" of "Content" (got "None", expected "Union[str, Combinable]") [misc]

It's ugly, but we can work around this currently by explicitly annotating the type of text:

class MyModel(models.Model):
    text: models.TextField[str | None, str | None] = MultiCollationTextField(..., null=True)

foo = MyModel(text=None)  # all good

Side note: We could work around this now using # type: ignore wherever we set text=None, but it's not great, because .text should in fact have the type str|None, and thus it'd be valuable for mypy to yell at us if we try to use it as if it's type is just str.

I believe that the solution involves some wrangling of generic type arguments between MultiCollationTextField and TextField. We also might end up needing two separate sets of classes: MultiCollation(Text|Char)Field and NullableMultiCollation(Text|Char)Field.