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

django-stubs breaks type checking of DataclassSerializer.validated_data #91

Closed
johnthagen opened this issue Oct 31, 2023 · 6 comments
Closed

Comments

@johnthagen
Copy link

johnthagen commented Oct 31, 2023

With this example code:

from dataclasses import dataclass

from rest_framework_dataclasses.serializers import DataclassSerializer


@dataclass
class Person:
    name: str
    age: int


class PersonSerializer(DataclassSerializer[Person]):
    class Meta:
        dataclass = Person


serializer = PersonSerializer(data={"name": "Tony", "age": 39})
serializer.is_valid()
person = serializer.validated_data
print(f"{person.name}")

With the following Mypy configuration:

[tool.mypy]
strict = true

Mypy produces a false positive error:

(venv) $ mypy main.py
main.py:20: error: "T" has no attribute "name"  [attr-defined]

When django-stubs is installed. I haven't been able to piece together why this is, but given how prevalent usage of django-stubs is for those type checking Django projects, I wanted to share this up.

Environment

  • Python 3.10.11
Package                         Version
------------------------------- ------------
asgiref                         3.7.2
Django                          4.2.6
django-stubs                    4.2.6
django-stubs-ext                4.2.5
djangorestframework             3.14.0
djangorestframework-dataclasses 1.3.1
mypy                            1.6.1
mypy-extensions                 1.0.0
pip                             23.1.2
pytz                            2023.3.post1
setuptools                      68.1.2
sqlparse                        0.4.4
tomli                           2.0.1
types-pytz                      2023.3.1.1
types-PyYAML                    6.0.12.12
typing_extensions               4.8.0
wheel                           0.40.0

These combinations also trigger the error:

  • Mypy 1.6.1, django-stubs 4.2.5
  • Mypy 1.5.1, django-stubs 4.2.4
  • Mypy 1.4.1, django-stubs 4.2.3
  • Mypy 1.6.1, django-stubs 4.2.2
  • Mypy 1.3.0, django-stubs 4.2.1
  • Mypy 1.2.0, django-stubs 4.2.0
@oxan
Copy link
Owner

oxan commented Oct 31, 2023

This smells a lot like a bug in either mypy or django-stubs. If you replace rest_framework_dataclasses/serializers.py in your venv with the following stripped version, you should still get the same error from mypy:

from __future__ import annotations

from typing import Any, Generic, TypeVar
from django.utils.functional import cached_property
import rest_framework.serializers

T = TypeVar('T')

class DataclassSerializer(rest_framework.serializers.Serializer, Generic[T]):
    def __init__(self, *args: Any, **kwargs: Any):
        pass

    @cached_property
    def validated_data(self) -> T:
        pass

Now, comment out the constructor, and it starts working.

@oxan
Copy link
Owner

oxan commented Oct 31, 2023

Also, if you replace this line:

serializer = PersonSerializer(data={"name": "Tony", "age": 39})

with this line

serializer = DataclassSerializer[Person](data={"name": "Tony", "age": 39})

it works.

Until someone has evidence to the contrary, I'm going to consider this to be a bug in something else and not something that can or should be fixed in djangorestframework-dataclasses.

@johnthagen
Copy link
Author

@oxan Thank you for the reply. I agree this is bizarre and could very well not be a problem with this package. I reported this to django-stubs (who also has someone on the Mypy team on their team) to see what they think

@johnthagen
Copy link
Author

johnthagen commented Nov 1, 2023

@oxan As explained here:

The underlying issue is that for type checking to work properly,djangorestframework-dataclasses has an implicit typing dependency on djangorestframework-stubs.

To help this situation, one idea would be to add a new extras to this package such that if someone:

pip install djangorestframework-dataclasses[typing]

This brings in djangorestframework-stubs as well. This would help make this relationship explicit.

Related to this, I think it would be useful in the README to show an example of how to use this library with type checkers (e.g. adding the generic type)

class PersonSerializer(DataclassSerializer[Person]):
    class Meta:
        dataclass = Person

I didn't realize that the generic types could be used until I hit a Mypy error and then jumped into the djangorestframework-dataclasses source code. Perhaps a new Typing section in the README could address all of the above.

@oxan
Copy link
Owner

oxan commented Nov 5, 2023

I'm not too fond of creating a typing extras, as it still has a discoverability issue: a typing extras isn't common practice in either Django or DRF, so you stilll need to look up the existence and necessity of installing it in the documentation, at which point it's just as easy to recommend installing djangorestframework-stubs.

Adding some documentation/examples about usage with mypy sounds good. It might take a while until I get around to writing that though, so feel free to open a PR with it :)

@johnthagen
Copy link
Author

One advantage of uses extras is you could add a range of versions (e.g. perhaps that is tested in CI using Mypy) such as a lower bound to djangorestframework-stubs. But I agree in this case a simple note in the README could be sufficient.

@oxan oxan closed this as completed in c37616a Dec 25, 2023
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

No branches or pull requests

2 participants