test solution#2857
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still two checklist violations that count as critical for this task. First, in app/main.py your __mul__ and __truediv__ methods currently try to handle Distance arguments (e.g. isinstance(other, Distance) and setting other_km = None), which conflicts with the explicit requirement that these methods must only accept int | float and not Distance instances [CHECKLIST ITEM #1]. This is a behavioral constraint, not just style, so it must be corrected before the solution can be considered complete. Second, none of the methods in Distance have type annotations for parameters or return types (e.g. def __truediv__(self, other: int | float) -> Distance:), which violates the requirement to use proper type hints and is causing the flake8-annotations check to fail [CHECKLIST ITEM #4]. To fix this, restrict __mul__ and __truediv__ to numeric other values in both their logic and type signatures, and add appropriate type annotations to all methods (__init__, comparison operators, __str__, __repr__, arithmetic methods). Functionally you’ve implemented the core behavior well, so once these type and supported-argument issues are addressed, your solution should be in good shape.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| def __mul__(self, other): | ||
| other_km = other if not isinstance(other, Distance) else None |
There was a problem hiding this comment.
This violates checklist item "Support correct types – __truediv__ and __mul__ should not accept Distance instance as argument": in both __mul__ and __truediv__ you explicitly handle Distance by setting other_km to None, which means these methods are effectively written to accept Distance arguments rather than restricting to int | float only.
| def __init__(self, km): | ||
| self.km = km | ||
|
|
||
| # def instance_check(other): | ||
| # return other.km if isinstance(other, Distance) else other | ||
|
|
||
| def __str__(self): | ||
| return f"Distance: {self.km} kilometers." | ||
|
|
||
| def __repr__(self): | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return Distance( | ||
| km=self.km + other_km | ||
| ) | ||
|
|
||
| def __iadd__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| self.km = self.km + other_km | ||
| return self | ||
|
|
||
| def __mul__(self, other): | ||
| other_km = other if not isinstance(other, Distance) else None | ||
| return Distance(km=self.km * other_km) | ||
|
|
||
| def __truediv__(self, other): | ||
| other_km = other if not isinstance(other, Distance) else None | ||
| return Distance(km=round(self.km / other_km, 2)) | ||
|
|
||
| def __lt__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return self.km < other_km | ||
|
|
||
| def __gt__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return self.km > other_km | ||
|
|
||
| def __eq__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return self.km == other_km | ||
|
|
||
| def __le__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return self.km <= other_km | ||
|
|
||
| def __ge__(self, other): | ||
| other_km = other.km if isinstance(other, Distance) else other | ||
| return self.km >= other_km |
There was a problem hiding this comment.
This violates checklist item "Use proper type annotations for function arguments and return types": the methods in Distance have no type hints for parameters or return types (e.g. def __truediv__(self, other: int | float) -> Distance: as shown in the checklist).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical violation of the task checklist in your Distance class implementation. Specifically, both __mul__ and __truediv__ are annotated to accept only numeric arguments (int | float), but their bodies still contain isinstance(other, Distance) checks and return None when a Distance instance is passed, which directly conflicts with [CHECKLIST ITEM #1] that says these methods must not accept Distance as an argument. This is a blocker because it means the methods are trying to handle an unsupported operand type and introduce a None return path instead of always returning a valid Distance, which can break expected functionality and test behavior.
To fix this, update __mul__ (around the lines where you currently check isinstance(other, Distance)) so that it only handles numeric operands, e.g.:
- Remove the
isinstance(other, Distance)branch and anyreturn None. - Implement it as
return Distance(self.km * other)assumingotherisint | float.
Similarly, update __truediv__ so it only divides by numeric values and always returns a Distance, for example:
- Remove the
isinstance(other, Distance)branch and anyreturn None. - Implement it as something like
return Distance(round(self.km / other, 2))forint | floatdivisors.
Aside from this issue, the rest of your class (initialization, string/representation methods, addition, in-place addition, comparisons, and rounded division behavior) is well-aligned with the requirements, so once you restrict __mul__ and __truediv__ to numeric arguments in both signature and logic, your solution should be ready to pass. Keep going—you’re very close to a fully compliant implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| def __mul__(self: Distance, other: int | float) -> Distance: | ||
| return (None | ||
| if isinstance(other, Distance) | ||
| else Distance(self.km * other)) |
There was a problem hiding this comment.
This violates checklist item #1: __truediv__ and __mul__ should not accept Distance instance as argument. Even though the signature is other: int | float, the body still checks isinstance(other, Distance) and returns None, meaning the method is trying to handle Distance arguments. Remove the isinstance(other, Distance) branch and just return Distance(self.km * other) for numeric other.
| def __truediv__(self: Distance, other: int | float) -> Distance: | ||
| return (None | ||
| if isinstance(other, Distance) | ||
| else Distance(round(self.km / other, 2))) |
There was a problem hiding this comment.
This also violates checklist item #1: __truediv__ and __mul__ should not accept Distance instance as argument. The method is annotated for numeric other, but the implementation still checks isinstance(other, Distance) and returns None. You should only handle numeric divisors here, e.g. return Distance(round(self.km / other, 2)), without any special Distance handling.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved — the Distance class now cleanly meets the functional requirements and behaves correctly across initialization, string/representation methods, arithmetic, and comparisons. You correctly implemented __str__ as "Distance: <km> kilometers." and __repr__ as Distance(km=<km>), which makes debugging and logging clear and consistent. Arithmetic methods like __add__, __iadd__, __mul__, and __truediv__ all return Distance instances, handle both Distance and numeric operands where required, and your division correctly rounds to two decimals while restricting __mul__/__truediv__ to numeric types only. Your comparison methods sensibly use the km attribute for both Distance and numeric operands, matching the intent of the class and keeping behavior intuitive. Type annotations are modern and precise, using int | float and Distance | int | float where appropriate, and you avoided unnecessary branches or awkward patterns, which improves readability. Overall, this is a solid, well-structured implementation that shows good attention to both correctness and clarity; keep building on this level of care in future exercises.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.