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

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

Solution #652

wants to merge 2 commits into from

Conversation

Jejemira
Copy link

No description provided.

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 hard work on this task! Your implementation is mostly correct and aligns well with the task requirements. However, there are a couple of critical issues that need to be addressed before we can approve the pull request:

  1. Instantiation of Limitation Class: In the Slide class, the limitation_class should be instantiated with the visitor's attributes to properly check the constraints. This ensures that the slide's limitations are correctly applied to each visitor.

  2. __get__ Method in IntegerRange Class: The __get__ method should return the actual value of the attribute, not self. Returning self will cause unexpected behavior when accessing the attribute, which is crucial for the correct functionality of your code.

Once these issues are resolved, your code will be ready for approval. Keep up the great work, and don't hesitate 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
objtype: None | Any = None
) -> int | str:
if obj is None:
return self

Choose a reason for hiding this comment

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

There is a mistake here. The __get__ method should return the value of the attribute, not self. Returning self is incorrect and will lead to unexpected behavior when accessing the attribute.

visitor: Any
) -> bool:
try:
self.limitation_class(

Choose a reason for hiding this comment

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

The limitation_class is expected to be a class, not an instance. You should instantiate it here with the visitor's attributes to check the constraints. Consider using self.limitation_class(visitor.age, visitor.height, visitor.weight) instead.

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.

Review Summary

Great job on your implementation! 🎉 Your code in main.py is mostly correct and aligns well with the task requirements. However, there are a couple of critical issues that need your attention:

  1. Instantiation of Limitation Class: In the Slide class, it's crucial to instantiate the limitation_class with the visitor's attributes. This ensures that the slide's limitations are correctly applied to each visitor. Make sure to review this part to enhance the functionality of your code.

  2. __get__ Method in IntegerRange Class: The __get__ method should return the actual value of the attribute, not self. Returning self can lead to unexpected behavior when accessing the attribute. Ensure that you return the value variable instead for correct functionality.

Despite these issues, your code meets the task requirements and constraints as specified. The decision to approve your pull request is made to help you move forward with your learning journey. Please take the time to review the comments and improve your code. Addressing these critical issues will help you avoid similar problems in the future. Keep up the good work and continue learning from your experiences! 🚀

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

objtype: None | Any = None
) -> int | str:
value = getattr(obj, self.private_name, None)
return value

Choose a reason for hiding this comment

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

The __get__ method should return the actual value of the attribute, not self. Returning self will cause unexpected behavior when accessing the attribute. You should return the value variable instead.

Comment on lines +117 to +121
self.limitation_class(
age=visitor.age,
height=visitor.height,
weight=visitor.weight
)

Choose a reason for hiding this comment

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

In the Slide class, the limitation_class should be instantiated with the visitor's attributes to properly check the constraints. This ensures that the slide's limitations are correctly applied to each visitor.

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