-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 #1231
base: master
Are you sure you want to change the base?
Solution #1231
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 project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the key name inconsistency in the Cafe
class and the logical issue in the go_to_cafe
function need to be resolved to ensure the code functions correctly. Additionally, correcting the typos in the error messages will improve clarity and professionalism. Once these changes are made, your solution will be in great shape! Keep up the great 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/cafe.py
Outdated
|
||
def visit_cafe(self, visitor: dict) -> str: | ||
if "vaccine" not in visitor: | ||
raise NotVaccinatedError("Visitor is not vaccined.") |
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.
There is a typo in the error message: 'vaccined' should be 'vaccinated'.
app/cafe.py
Outdated
raise OutdatedVaccineError("Vaccine expiration date is missing.") | ||
|
||
if expiration_date < datetime.now().date(): | ||
raise OutdatedVaccineError("Vaccine is expirited.") |
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.
There is a typo in the error message: 'expirited' should be 'expired'.
app/cafe.py
Outdated
if expiration_date < datetime.now().date(): | ||
raise OutdatedVaccineError("Vaccine is expirited.") | ||
|
||
if not visitor.get("wear masks", False): |
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 key 'wear masks' in the visitor dictionary should be consistent with the expected key. Consider using 'wear_mask' or 'wearing_mask' to avoid confusion.
try: | ||
cafe.visit_cafe(friend) | ||
except VaccineError: | ||
return "All friends should be vaccinated" |
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.
Returning immediately upon catching a VaccineError
means that if any friend is not vaccinated, the function will not check the rest of the friends. Consider accumulating results for all friends before deciding on the return value.
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.
Your code meets the task requirements and constraints, and it's clear you've put in a lot of effort. 🎉 However, there are a few critical issues that need attention:
-
Cafe
Class: Ensure thatexpiration_date
is adate
object before comparing it withdatetime.now().date()
. Ifexpiration_date
is a string, parse it into adate
object to avoid errors. -
Visitor Dictionary Check: When checking for the 'vaccine' key, ensure its value is not
None
to prevent potential errors. -
go_to_cafe
Function: The current logic may cause the function to exit prematurely if any friend is not vaccinated. Consider revising the logic to accumulate messages or use a flag, and return after the loop to ensure all friends are checked.
Your implementation of error classes and the overall structure is well done. Keep up the good work, and make sure to address these issues in future projects to enhance your coding skills. Keep learning and improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
self.name = name | ||
|
||
def visit_cafe(self, visitor: dict) -> str: | ||
if "vaccine" not in visitor: |
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.
Consider checking if the 'vaccine' key exists and its value is not None to avoid potential errors.
|
||
expiration_date = visitor["vaccine"].get("expiration_date") | ||
|
||
if expiration_date < datetime.now().date(): |
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.
Ensure that expiration_date
is a date
object before comparing it with datetime.now().date()
. If expiration_date
is a string, you should parse it into a date
object.
try: | ||
cafe.visit_cafe(friend) | ||
except VaccineError: | ||
return "All friends should be vaccinated" |
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.
Returning immediately upon catching a VaccineError
will stop the loop and prevent checking other friends. Consider accumulating a message or a flag and returning after the loop to ensure all friends are processed.
No description provided.