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 #1326

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

Solution #1326

wants to merge 3 commits into from

Conversation

asynyshyn
Copy link

No description provided.

@asynyshyn asynyshyn marked this pull request as draft October 27, 2024 03:14
@asynyshyn asynyshyn marked this pull request as ready for review October 27, 2024 03:14
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


Thank you for your submission! Your code is well-structured and meets most of the task requirements. However, there are a couple of critical issues that need to be addressed before we can approve the pull request:

  1. Type Safety in bite Method: The bite method should accept a parameter specifically of type Herbivore instead of Animal. This change will ensure type safety and align with the method's logic, which is designed to interact with Herbivore instances.

  2. __str__ Method Implementation: The __str__ method is typically used as an instance method to provide a string representation of an object. In your code, it seems to be used as a class method. Consider renaming it to something more descriptive, like list_alive_animals, or converting it to an instance method if it should represent an instance.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and feel free to reach out if you have any questions. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

app/main.py Outdated
Comment on lines 21 to 23
@classmethod
def __str__(cls) -> str:
return "[" + ", ".join(repr(animal) for animal in cls.alive) + "]"

Choose a reason for hiding this comment

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

The __str__ method is typically an instance method, not a class method. Consider renaming this method to something more appropriate for a class method, like list_alive_animals, or converting it to an instance method if it should represent an instance.

app/main.py Outdated


class Carnivore(Animal):
def bite(self, herbivore: Animal) -> None:

Choose a reason for hiding this comment

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

The bite method should ideally accept a parameter of type Herbivore instead of Animal to ensure type safety, as the method logic specifically checks for Herbivore instances.

@asynyshyn asynyshyn marked this pull request as draft November 1, 2024 00:09
@asynyshyn asynyshyn marked this pull request as ready for review November 1, 2024 00:09
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.

2 participants