-
Notifications
You must be signed in to change notification settings - Fork 663
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
Aqua park #625
base: master
Are you sure you want to change the base?
Aqua park #625
Conversation
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.
Thank you for your hard work on this pull request! There are a few critical issues that need to be addressed before we can approve it:
-
IntegerRange
Descriptor Usage: TheIntegerRange
class is defined with descriptor methods but is not utilized as a descriptor in theSlideLimitationValidator
class. If the goal was to use it for automatic validation, it should be applied to theage_range
,weight_range
, andheight_range
attributes. -
validate
Method Return Type: In theAdultSlideLimitationValidator
, thevalidate
method should return a boolean value. Currently, it lacks an explicit return statement, resulting inNone
being returned. Please ensure the method returns a boolean and update the method signature to-> bool
. -
Type Checking in
IntegerRange
: The__set__()
method in theIntegerRange
class should validate the type of the value before setting it. The current implementation only checks if the value is within the range. Make sure to raise aTypeError
orValueError
if the type is incorrect. -
SlideLimitationValidator
Abstract Method: TheSlideLimitationValidator
class should not have an abstractvalidate
method, as no additional abstract methods are required for this class. -
Slide
Class Instantiation: TheSlide
class should store thelimitation_class
itself, not an instance of it. The current implementation instantiates thelimitation_class
in the constructor, which is against the task requirements.
Once these issues are addressed, your code will be in great shape! Keep up the excellent work! 🌟
✨ 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
height_range=IntegerRange(120, 220) | ||
) | ||
|
||
def validate(self, visitor: "Visitor") -> None: |
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 validate
method in AdultSlideLimitationValidator
should return a boolean value. Currently, it returns None
because there is no explicit return statement. Add -> bool
to the method signature and ensure it returns a boolean value.
app/main.py
Outdated
self.age_range = age_range | ||
self.weight_range = weight_range | ||
self.height_range = height_range |
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 IntegerRange
class is defined with descriptor methods (__get__
, __set__
, __set_name__
), but it is not used as a descriptor in the SlideLimitationValidator
class. If the intention was to use it for automatic validation, consider using it as a descriptor for the age_range
, weight_range
, and height_range
attributes.
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.
Review Summary
Your code meets most of the task requirements, and I appreciate your efforts in implementing the solution. However, there are a few critical issues that need attention:
-
SlideLimitationValidator
Class: Ensure that this class includes an__init__
method that takesage
,weight
, andheight
as parameters, as specified in the task description. -
Instantiation of
limitation_class
inSlide
Class: TheSlide
class should store thelimitation_class
itself, not an instance. This means you should store the class and instantiate it when needed in thecan_access
method. -
validate
Method inAdultSlideLimitationValidator
: This method is missing and should be implemented to return a boolean value, which is crucial for the functionality of your code.
Despite these issues, I am approving your Pull Request to help you move forward with your learning. Please review the comments and address these issues to improve your code. Remember, addressing these critical issues will help you avoid similar problems in the future. Keep up the good work, and continue to learn from your mistakes! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
limitation_class: SlideLimitationValidator | ||
) -> None: | ||
self.name = name | ||
self.limitation_class = limitation_class |
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 Slide
class should store the limitation_class
itself, not an instance of it. You should store the class and instantiate it when needed in the can_access
method.
) -> None: | ||
self.age = age | ||
self.weight = weight | ||
self.height = height | ||
|
||
|
||
class AdultSlideLimitationValidator(SlideLimitationValidator): |
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 AdultSlideLimitationValidator
class should implement a validate
method that returns a boolean value. Currently, this method is missing, which is against the task requirements.
No description provided.