Solution #2313
Solution #2313
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the Distance class! The core functionality is solid, and you've correctly implemented most of the magic methods. However, a few changes are needed to fully align with the project's requirements and Python best practices.
Here are the key areas to address:
- Incorrect Error Handling: The
__truediv__method inapp/main.pycurrently returnsNonefor unsupported types. The standard Python behavior is to raise aTypeError. Please update the method to raise this exception, and adjust the test intests/test_main.pyto check for it usingpytest.raises(TypeError). - Type Annotations: The type hints need to be more accurate as per the checklist. Please add
from __future__ import annotationsat the top ofapp/main.pyand use union types likeother: Distance | int | floatfor methods that accept multiple types. - Unnecessary
elif: In the comparison methods, you can simplify the logic by replacingelifwithif, since the precedingifblock already contains areturnstatement.
Once these changes are made, your solution will be perfect. Keep up the excellent effort!
✨ 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 test_distance_class_truediv_distance(): | ||
| distance1 = Distance(30) | ||
| distance2 = Distance(3) | ||
| try: | ||
| result = distance1 / distance2 | ||
| except TypeError: | ||
| pass | ||
| else: | ||
| assert result is None, ( | ||
| "'__truediv__' method should not accept Distance instance" | ||
| ) |
There was a problem hiding this comment.
This test validates an incorrect behavior. The standard way to handle an operation with unsupported types in Python is to raise a TypeError, not to return None. The test should be updated to assert that a TypeError is raised, for example by using with pytest.raises(TypeError):. This will also require fixing the __truediv__ method to raise this exception or return NotImplemented.
app/main.py
Outdated
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
This violates the checklist requirements for type annotations.
- The type for
othershould beDistance | int | floatto reflect that it can be aDistanceinstance or a number. - The use of string forward-references like
"Distance"is discouraged. To fix this and avoid aNameError, you should addfrom __future__ import annotationsat the very top of your file.
This feedback applies to __iadd__ and all comparison methods as well.
app/main.py
Outdated
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: float) -> "Distance": |
There was a problem hiding this comment.
The type annotation for other should be int | float to accurately reflect that this method accepts both integers and floats. This also applies to the __truediv__ method.
app/main.py
Outdated
| def __lt__(self, other: "Distance") -> bool: | ||
| if isinstance(other, (int, float)): | ||
| return self.km < other | ||
| elif isinstance(other, Distance): |
There was a problem hiding this comment.
This elif is unnecessary. According to the checklist, you should avoid else (or elif) blocks when the preceding if block already has a return statement. You can simply change this to a regular if. This applies to all other comparison methods (__gt__, __eq__, etc.).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the Distance class! You've addressed several key points from the last review. However, a few critical issues need to be resolved before this can be approved.
The most important issue is in app/main.py: the __truediv__ method still returns None for unsupported types instead of raising a TypeError. This was a high-priority requirement from the previous review.
Additionally, the type hints for other in __mul__ and __truediv__ are incorrect. They should only accept int | float, not Distance, as the requirements state. [CHECKLIST ITEM #Support correct types.1]
Lastly, please update the tests for these unsupported operations in tests/test_main.py to use with pytest.raises(TypeError): for a more robust and standard way to check for exceptions. You're very close to the finish line!
✨ 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
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: Distance | int | float) -> "Distance": |
There was a problem hiding this comment.
This type hint violates a requirement. According to the checklist, the __mul__ method should only accept an int or float as the other argument, not a Distance instance.
This violates checklist item #Support correct types.1: "__truediv__ and __mul__ should not accept Distance instance as argument."
| def __mul__(self, other: Distance | int | float) -> "Distance": | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: Distance | int | float) -> Distance | None: |
There was a problem hiding this comment.
The type hint for other is incorrect here. As with __mul__, the checklist specifies that this method should only operate on numbers, not other Distance instances.
This violates checklist item #Support correct types.1: "__truediv__ and __mul__ should not accept Distance instance as argument."
| raise ZeroDivisionError | ||
| value = round(self.km / other, 2) | ||
| return Distance(value) | ||
| return None |
There was a problem hiding this comment.
This was a high-priority issue from the last review. Instead of returning None for unsupported types, this method should raise a TypeError. This is the standard behavior in Python for operations with unsupported operands.
| return Distance(value) | ||
| return None | ||
|
|
||
| def __lt__(self, other: "Distance") -> bool: |
There was a problem hiding this comment.
The type hint for other is incomplete. Your implementation correctly handles int and float types, but the annotation only specifies "Distance". To be accurate, it should be Distance | int | float, just like the other comparison methods.
| def test_distance_class_mul_distance(): | ||
| distance1 = Distance(5) | ||
| distance2 = Distance(3) | ||
| try: | ||
| result = distance1 * distance2 | ||
| except TypeError: | ||
| pass | ||
| else: | ||
| assert result is None, ( | ||
| "'__mul__' method should not accept Distance instance" | ||
| ) |
There was a problem hiding this comment.
This test can be simplified and made more explicit by using pytest.raises. This context manager clearly states that the code within the block is expected to raise a TypeError.
For example:
def test_distance_class_mul_distance():
distance1 = Distance(5)
distance2 = Distance(3)
with pytest.raises(TypeError):
distance1 * distance2This is the standard way to test for expected exceptions in pytest.
| def test_distance_class_truediv_distance(): | ||
| distance1 = Distance(30) | ||
| distance2 = Distance(3) | ||
| try: | ||
| result = distance1 / distance2 | ||
| except TypeError: | ||
| pass | ||
| else: | ||
| assert result is None, ( | ||
| "'__truediv__' method should not accept Distance instance" | ||
| ) |
There was a problem hiding this comment.
Similar to the __mul__ test, this can be improved by using pytest.raises(TypeError). The current try...except...else structure is a bit lenient as it allows the test to pass if a TypeError is raised OR if the operation returns None. The test should be specific about the expected behavior, which is raising a TypeError.
Please refactor this to use with pytest.raises(TypeError):.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override to help you progress, but there are a few key requirements that still need to be addressed. You've done an excellent job with the overall structure and test coverage!
The most critical issue is that the __truediv__ method in app/main.py should raise TypeError for unsupported operations instead of returning None. Consequently, the tests for multiplication and division by another Distance instance in tests/test_main.py must use with pytest.raises(TypeError): to correctly assert that an exception is raised. Finally, please adjust the type hints in __mul__ and __truediv__ to only accept int | float for the other parameter, as these operations are not supported for two Distance instances.
Keep up the great effort, and pay close attention to these error-handling details in future tasks!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| def test_distance_class_mul_distance(): | ||
| distance1 = Distance(5) | ||
| distance2 = Distance(3) | ||
| try: | ||
| result = distance1 * distance2 | ||
| except TypeError: | ||
| pass | ||
| else: | ||
| assert result is None, ( | ||
| "'__mul__' method should not accept Distance instance" | ||
| ) |
There was a problem hiding this comment.
This test should be updated to use with pytest.raises(TypeError): to check that multiplying two Distance instances raises an exception, as required. The current try-except block is not the standard way to test for exceptions with pytest and can be misleading.
| def test_distance_class_truediv_distance(): | ||
| distance1 = Distance(30) | ||
| distance2 = Distance(3) | ||
| try: | ||
| result = distance1 / distance2 | ||
| except TypeError: | ||
| pass | ||
| else: | ||
| assert result is None, ( | ||
| "'__truediv__' method should not accept Distance instance" | ||
| ) |
There was a problem hiding this comment.
This test should use with pytest.raises(TypeError): to verify that a TypeError is raised when dividing a Distance by another Distance. The current implementation incorrectly tests that the operation returns None, which confirms the incorrect behavior in the __truediv__ method. The method should raise a TypeError for unsupported operands, not return None.
| f"'Distance({kilometers}) <= {kilometers2}' should equal to {result}" | ||
| ) | ||
|
|
||
| pytest.raises(TypeError) |
There was a problem hiding this comment.
This pytest.raises call is outside of any test function and has no effect. It should be removed. To test for an exception, the call that is expected to raise it must be placed inside a with pytest.raises(...): block within a test function.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: Distance | int | float) -> "Distance": |
There was a problem hiding this comment.
The type hint for other is incorrect. According to checklist item #Support correct types.1, this method should only accept int or float as arguments, not another Distance instance.
| def __mul__(self, other: Distance | int | float) -> "Distance": | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: Distance | int | float) -> Distance | None: |
There was a problem hiding this comment.
The type hints here need correction. The other parameter should only accept int | float according to the checklist. Consequently, the return type should just be Distance, not Distance | None, because the method should raise an exception for invalid types rather than returning None.
| raise ZeroDivisionError | ||
| value = round(self.km / other, 2) | ||
| return Distance(value) | ||
| return None |
There was a problem hiding this comment.
This line should raise a TypeError for unsupported operand types, as required by the task and mentioned in the previous review. Returning None is incorrect.
| return Distance(value) | ||
| return None | ||
|
|
||
| def __lt__(self, other: "Distance") -> bool: |
There was a problem hiding this comment.
The type hint for other is incomplete. Your implementation correctly handles int and float in addition to Distance instances, so the type hint should be Distance | int | float to match the logic and be consistent with the other comparison methods.
2 participants
No description provided.