Added typing to package code. - #102
Open
noamkush wants to merge 1 commit into
Open
Conversation
Reviewer's GuideThis PR applies comprehensive type annotations across the generic views, mixins, and viewsets, upgrades the minimum Python and dependency versions, integrates strict MyPy configuration, and updates CI/CD and tests to match the new behavior. Class diagram for updated generic views and mixins with type annotationsclassDiagram
class GenericAPIView {
read_serializer_class: type[BaseSerializer[_MT_co]] | None
write_serializer_class: type[BaseSerializer[_MT_co]] | None
get_serializer_class() type[BaseSerializer[_MT_co]]
get_read_serializer(*args: Any, **kwargs: Any) BaseSerializer[_MT_co]
get_read_serializer_class(default_to_serializer_class: bool = False) type[BaseSerializer[_MT_co]]
get_write_serializer(*args: Any, **kwargs: Any) BaseSerializer[_MT_co]
get_write_serializer_class(default_to_serializer_class: bool = False) type[BaseSerializer[_MT_co]]
}
class CreateAPIView {
post(request: Request, *args: Any, **kwargs: Any) Response
}
class UpdateAPIView {
put(request: Request, *args: Any, **kwargs: Any) Response
patch(request: Request, *args: Any, **kwargs: Any) Response
}
class ListAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
}
class RetrieveAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
}
class ListCreateAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
post(request: Request, *args: Any, **kwargs: Any) Response
}
class RetrieveDestroyAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
delete(request: Request, *args: Any, **kwargs: Any) Response
}
class RetrieveUpdateAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
put(request: Request, *args: Any, **kwargs: Any) Response
patch(request: Request, *args: Any, **kwargs: Any) Response
}
class RetrieveUpdateDestroyAPIView {
get(request: Request, *args: Any, **kwargs: Any) Response
put(request: Request, *args: Any, **kwargs: Any) Response
patch(request: Request, *args: Any, **kwargs: Any) Response
delete(request: Request, *args: Any, **kwargs: Any) Response
}
GenericAPIView <|-- CreateAPIView
GenericAPIView <|-- UpdateAPIView
GenericAPIView <|-- ListAPIView
GenericAPIView <|-- RetrieveAPIView
GenericAPIView <|-- ListCreateAPIView
GenericAPIView <|-- RetrieveDestroyAPIView
GenericAPIView <|-- RetrieveUpdateAPIView
GenericAPIView <|-- RetrieveUpdateDestroyAPIView
class UpdateModelMixin {
update(request: Request, *args: Any, **kwargs: Any) Response
}
class CreateModelMixin {
create(request: Request, *args: Any, **kwargs: Any) Response
}
class ListModelMixin {
list(request: Request, *args: Any, **kwargs: Any) Response
}
class RetrieveModelMixin {
retrieve(request: Request, *args: Any, **kwargs: Any) Response
}
Class diagram for updated viewsets with type annotationsclassDiagram
class GenericViewSet {
}
class ModelViewSet {
}
class ReadOnlyModelViewSet {
}
GenericAPIView <|-- GenericViewSet
GenericViewSet <|-- ModelViewSet
GenericViewSet <|-- ReadOnlyModelViewSet
ModelViewSet <|-- ReadOnlyModelViewSet
note for GenericViewSet "Now inherits GenericAPIView[_MT_co] and viewsets.GenericViewSet[_MT_co]"
note for ModelViewSet "Now inherits GenericViewSet[_MT_co]"
note for ReadOnlyModelViewSet "Now inherits GenericViewSet[_MT_co]"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There are many nearly identical HTTP method implementations across the viewset classes—consider abstracting that pattern (e.g. via a base mixin or factory) to reduce duplication.
- The large number of
# type: ignoreannotations suggests mypy isn’t recognizing your customget_read_serializer/get_write_serializermethods—introducing a protocol or adjusting your generics might help remove those ignores. - This PR bundles typing additions, version bumps, and a behavioral change in
get_serializer_class; it may be clearer to split these concerns into separate PRs to isolate the breaking change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are many nearly identical HTTP method implementations across the viewset classes—consider abstracting that pattern (e.g. via a base mixin or factory) to reduce duplication.
- The large number of `# type: ignore` annotations suggests mypy isn’t recognizing your custom `get_read_serializer`/`get_write_serializer` methods—introducing a protocol or adjusting your generics might help remove those ignores.
- This PR bundles typing additions, version bumps, and a behavioral change in `get_serializer_class`; it may be clearer to split these concerns into separate PRs to isolate the breaking change.
## Individual Comments
### Comment 1
<location> `drf_rw_serializers/generics.py:22-23` </location>
<code_context>
+
+
+class GenericAPIView(generics.GenericAPIView[_MT_co]):
+ read_serializer_class: type[BaseSerializer[_MT_co]] | None = None
+ write_serializer_class: type[BaseSerializer[_MT_co]] | None = None
-class GenericAPIView(generics.GenericAPIView):
</code_context>
<issue_to_address>
**issue:** Type annotations using 'type[...] | None' may not be compatible with Python <3.10.
To maintain compatibility with Python 3.9, use 'Optional[type[BaseSerializer[_MT_co]]]' instead.
</issue_to_address>
### Comment 2
<location> `example_app/urls.py:22` </location>
<code_context>
)
-urlpatterns = [
+urlpatterns: list[URLResolver | URLPattern] = [
path(
"orders-list-without-read-serializer/",
</code_context>
<issue_to_address>
**issue:** Type annotation using 'list[URLResolver | URLPattern]' may not be compatible with Python <3.10.
For Python 3.9 support, use 'List[Union[URLResolver, URLPattern]]' instead.
</issue_to_address>
### Comment 3
<location> `drf_rw_serializers/mixins.py:12` </location>
<code_context>
partial = kwargs.pop("partial", False)
- instance = self.get_object()
- write_serializer = self.get_write_serializer(instance, data=request.data, partial=partial)
+ instance = self.get_object() # type: ignore[attr-defined]
+ write_serializer = self.get_write_serializer( # type: ignore[attr-defined]
+ instance,
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing all inline type ignore comments with a single module-level or class-level ignore directive to reduce clutter.
Here’s a quick way to get rid of all those inline `# type: ignore[...]` comments and still keep your annotations:
1. At the top of the module (or right on each mixin class), add a single `type: ignore` for the “attr-defined” and “misc” errors you’re silencing:
```python
# -*- coding: utf-8 -*-
# mypy: ignore-errors
# type: ignore[attr-defined, misc]
from typing import Any
from rest_framework import mixins, status
from rest_framework.request import Request
from rest_framework.response import Response
```
2. Remove all the per-call ignores. For example, your `UpdateModelMixin` becomes:
```python
class UpdateModelMixin(mixins.UpdateModelMixin):
def update(self, request: Request, *args: Any, **kwargs: Any) -> Response:
partial = kwargs.pop("partial", False)
instance = self.get_object()
write_serializer = self.get_write_serializer(
instance,
data=request.data,
partial=partial,
)
write_serializer.is_valid(raise_exception=True)
self.perform_update(write_serializer)
# pylint: disable=protected-access
if getattr(instance, "_prefetched_objects_cache", None) is not None:
instance._prefetched_objects_cache = {}
# pylint: enable=protected-access
read_serializer = self.get_read_serializer(instance)
return Response(read_serializer.data)
```
Repeat for your other mixins (Create/List/Retrieve).
By silencing the errors once at the top (or on each class), you preserve all your type-hints and remove the noise of dozens of inline ignores.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description: This adds annotations to the code in
drf_rw_serializers/Dependencies: Fixes #58
Merge checklist:
Post merge:
finished.
Author concerns: This does break the current behavior of
get_serializer_classas it may no longer return aNonein the rare case where there's no request and no serializer configured. This is more compatible with rest framework, as the base implementation doesn't allowNoneto be returned andget_serializerassumes it didn't get aNone.Additionally, I had to bump the minimal python version and some package versions as it was very hard to keep Python 3.8 support.
Summary by Sourcery
Add comprehensive typing support and update project configuration for newer Python and dependency versions
Enhancements:
Build:
CI:
Documentation:
Tests: