-
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
created system of determines visitors #618
base: master
Are you sure you want to change the base?
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.
Looks good!
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 on the pull request! 🎉 While there are a few areas where you can improve, such as ensuring proper validation with the limitation_class
and adding informative messages to exceptions, these are minor and don't impact the core functionality. Keep up the good work and consider these suggestions for future improvements!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
def __set__(self, instance: type, value: int) -> None: | ||
if not isinstance(value, int): | ||
raise TypeError |
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.
This line raises a TypeError
without a message. It's a good practice to provide a message to make debugging easier.
if not isinstance(value, int): | ||
raise TypeError | ||
if not (self.min_amount <= value <= self.max_amount): | ||
raise ValueError |
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.
This line raises a ValueError
without a message. Consider adding a message to provide more context about the error.
|
||
def can_access(self, visitor: Visitor) -> bool: | ||
try: | ||
self.limitation_class(visitor.age, visitor.weight, visitor.height) |
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 limitation_class
is being instantiated with visitor
attributes, but it seems like the intention is to validate the visitor against the slide's limitations. Ensure that the limitation_class
is used correctly to validate the visitor.
No description provided.