-
Notifications
You must be signed in to change notification settings - Fork 722
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
implement py-api-views #674
base: master
Are you sure you want to change the base?
Conversation
cinema/serializers.py
Outdated
title = serializers.CharField(max_length=255) | ||
description = serializers.CharField() | ||
duration = serializers.IntegerField() | ||
name = serializers.CharField(max_length=255) |
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.
Do you need to define these fields if you use ModelSerializer?
db.sqlite3
Outdated
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.
You don't need to push this file.
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.
You stille have this file here.
cinema/serializers.py
Outdated
|
||
|
||
class MovieSerializer(serializers.Serializer): | ||
class MovieSerializer(serializers.ModelSerializer): | ||
actors = serializers.PrimaryKeyRelatedField( |
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.
Using PrimaryKeyRelatedField you will have N+1 query problem.
cinema/views.py
Outdated
|
||
class MovieViewSet(viewsets.ModelViewSet): | ||
queryset = Movie.objects.all() |
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.
Using PrimaryKeyRelatedField in your serializer you will have N+1 query problem for genres and actors.
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.
Use prefetch_related
to solve N+1 problem.
cinema/views.py
Outdated
|
||
class GenreList(APIView): | ||
def get(self, request: HttpRequest) -> Response: | ||
genre = Genre.objects.all() |
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.
name it in plural - genres
cinema/urls.py
Outdated
path("actors/<int:pk>/", ActorDetail.as_view(), name="actor-detail"), | ||
path("genres/", GenreList.as_view(), name="actor-list"), | ||
path("genres/<int:pk>/", GenreDetail.as_view(), name="actor-detail"), | ||
path( |
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.
You can register CinemaHallViewSet in router with the correct prefix. It will look better
cinema/urls.py
Outdated
GenreList, | ||
CinemaHallViewSet | ||
) | ||
from .views import MovieViewSet |
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.
Add this to the import above
cinema/urls.py
Outdated
path("", include(router.urls)), | ||
path("actors/", ActorList.as_view(), name="actor-list"), | ||
path("actors/<int:pk>/", ActorDetail.as_view(), name="actor-detail"), | ||
path("genres/", GenreList.as_view(), name="actor-list"), |
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.
the name here should be "genre-list". See also below.
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.
In general, great job. But you need to do some small fixes.
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.
Great job, but still you need to fix N+1 problem and delete db.sqlite3 file from your PR.
description = serializers.CharField() | ||
duration = serializers.IntegerField() | ||
class MovieSerializer(serializers.ModelSerializer): | ||
genres = serializers.SerializerMethodField() |
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.
Not necessary in this task
Create serializers for all these models. Do not use related serializers for ManyToMany relations.
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.
Good job!
No description provided.