Solution to review – TO MASTER - #1
Conversation
Sn1kls
left a comment
Sn1kls
left a comment
There was a problem hiding this comment.
Nice work – the logic correctly covers all three scenarios from the README. Key fixes before approval: inherit from Exception, use parenthesised imports, compare to None with is, and add type hints (which also clears the unused import). The rest are minor style nits. Requesting changes – you're close)))
| @@ -0,0 +1,19 @@ | |||
| from app.errors import NotVaccinatedError, \ | |||
| OutdatedVaccineError, \ | |||
| NotWearingMaskError | |||
There was a problem hiding this comment.
Use parentheses for multi-line imports instead of backslash continuation. You already do this in main.py (lines 2–4), so it's mainly about keeping one consistent style across the project. It's also the approach PEP 8 prefers for wrapping long lines.
| from app.errors import NotVaccinatedError, \ | ||
| OutdatedVaccineError, \ | ||
| NotWearingMaskError | ||
| import datetime |
There was a problem hiding this comment.
Imports should be grouped – standard library first, then third-party, then local app – with a blank line between groups (PEP 8 – Imports). So datetime (standard library) belongs above app.errors (local), separated by a blank line.
Note: default flake8 doesn't check import order (isort does)
| def __init__(self, name: str) -> None: | ||
| self.name = name | ||
|
|
||
| def visit_cafe(self, vstr: dict) -> str: |
There was a problem hiding this comment.
vstr is hard to read – the README calls this parameter visitor. A descriptive name makes the method self-documenting, so please rename it.
| self.name = name | ||
|
|
||
| def visit_cafe(self, vstr: dict) -> str: | ||
| if "vaccine" not in vstr.keys() or vstr["vaccine"] == None: |
There was a problem hiding this comment.
Two small fixes: drop .keys() ("vaccine" not in visitor is enough), and compare to None with is, not ==. PEP 8 is explicit: "Comparisons to singletons like None should always be done with is or is not, never the equality operators." (flake8 flags this as E711)
| elif not vstr["wearing_a_mask"]: | ||
| raise NotWearingMaskError("not wearing mask") | ||
| else: | ||
| return "Welcome to " + self.name |
There was a problem hiding this comment.
Prefer an f-string for readability and consistency: return f"Welcome to {self.name}" (PEP 498)
| @@ -0,0 +1,14 @@ | |||
| class NotWearingMaskError(BaseException): | |||
There was a problem hiding this comment.
Custom exceptions should inherit from Exception. BaseException is reserved for system-exiting signals like SystemExit and KeyboardInterrupt, so an error derived from it won't be caught by a normal except Exception – which is surprising and easy to break later. The Python tutorial puts it directly: "Exceptions should typically be derived from the Exception class, either directly or indirectly."
|
|
||
|
|
||
| class OutdatedVaccineError(VaccineError): | ||
| pass No newline at end of file |
There was a problem hiding this comment.
The file should end with a newline after the last line (flake8 W292)
| OutdatedVaccineError) | ||
|
|
||
|
|
||
| def go_to_cafe(friends, cafe) -> str: |
There was a problem hiding this comment.
The parameters have no type hints – checklist point 5.
Annotating cafe: Cafe also puts the Cafe import on line 1 to use – it's currently unused and flagged as flake8 F401, so this is cleaner than deleting the import.
| def go_to_cafe(friends, cafe) -> str: | ||
| total = 0 | ||
|
|
||
| for i in range(len(friends)): |
There was a problem hiding this comment.
You can loop over the list directly – for friend in friends: instead of indexing with range(len(...)). It's the more idiomatic and readable form (Python for statements).
| if total: | ||
| return f'Friends should buy {total} masks' | ||
| else: | ||
| return f'Friends can go to {cafe.name}' |
There was a problem hiding this comment.
Same else after return as in cafe.py – safe to drop (checklist point 4). Renaming total to something like masks_to_buy would also make the intent clearer.
1 participant
No description provided.