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

Solution #393

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Solution #393

wants to merge 7 commits into from

Conversation

prochigor
Copy link

No description provided.

Comment on lines +15 to +16
def __str__(self) -> str:
return str(self.name)

Choose a reason for hiding this comment

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

Do you really need here str()?

Copy link
Author

Choose a reason for hiding this comment

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

This is because I add an annotation type, and it has an underline without it

Comment on lines +24 to +25
def __str__(self) -> str:
return f"{self.name} rows: {self.rows} seats: {self.seats_in_row}"

Choose a reason for hiding this comment

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

Perhaps it is more logical to return the name of the hall?

Copy link
Author

Choose a reason for hiding this comment

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

I don't agree, it depends on the task you have, but here is not business logic about that.

Comment on lines 33 to 51
class GenreSerializer(serializers.ModelSerializer):

class Meta:
model = Genre
fields = ["id", "name"]


class ActorSerializer(serializers.ModelSerializer):

class Meta:
model = Actor
fields = ["id", "first_name", "last_name"]


class CinemaHallSerializer(serializers.ModelSerializer):

class Meta:
model = CinemaHall
fields = ["id", "name", "rows", "seats_in_row"]

Choose a reason for hiding this comment

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

You can use fields = "all"

Copy link
Author

Choose a reason for hiding this comment

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

it is a good point

cinema/views.py Outdated
Comment on lines 36 to 38
class GenreDetail(APIView):
def get(self, request, pk):
genre = get_object_or_404(Genre, pk=pk)
Copy link

@petrykivd petrykivd Nov 28, 2023

Choose a reason for hiding this comment

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

You can create get_object method, It will be more readable.

cinema/views.py Outdated
if request.method == "PUT":
serializer = MovieSerializer(movie, data=request.data)
def put(self, request, pk):
genre = get_object_or_404(Genre, pk=pk)

Choose a reason for hiding this comment

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

Same here

cinema/views.py Outdated

def patch(self, request, pk):
genre = get_object_or_404(Genre, pk=pk)

Choose a reason for hiding this comment

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

and next

Copy link
Contributor

@Y-Havryliv Y-Havryliv left a comment

Choose a reason for hiding this comment

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

tests should pass

@prochigor
Copy link
Author

I have just pushed that(


def update(self, instance, validated_data):
instance.title = validated_data.get("title", instance.title)
instance.description = validated_data.get(
"description", instance.description
)
instance.duration = validated_data.get("duration", instance.duration)

actors_data = validated_data.pop("actors", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to use an empty list instead of string

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

Successfully merging this pull request may close these issues.

3 participants