Skip to content

feat: implement Distance class with magic methods#2850

Open
mxrpheusd wants to merge 4 commits into
mate-academy:masterfrom
mxrpheusd:develop/distance-logic
Open

feat: implement Distance class with magic methods#2850
mxrpheusd wants to merge 4 commits into
mate-academy:masterfrom
mxrpheusd:develop/distance-logic

Conversation

@mxrpheusd

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is a correctness issue that will cause runtime errors in multiple methods of your Distance class. In __add__, __iadd__, __mul__, __truediv__, and all comparison methods (__lt__, __gt__, __le__, __eq__, __ge__), you use patterns like isinstance(other, Distance | int | float) and isinstance(other, int | float), but the second argument to isinstance must be a type or a tuple of types, not a union expression. As written, these calls will raise TypeError at runtime, so operations like Distance() + 5, Distance() * 2, or comparisons such as Distance() < 10 will fail and break required functionality.

To fix this, update each check to use tuples, for example:

  • isinstance(other, (Distance, int, float)) wherever you currently use Distance | int | float (e.g., in __add__, __iadd__, and the comparison methods).
  • isinstance(other, (int, float)) wherever you currently use int | float (e.g., in __mul__ and __truediv__).
    Once you correct these isinstance calls, your existing logic (including rounding in division and support for both Distance and numeric operands) will work as intended and fully satisfy the task requirements. Aside from this, the structure and behavior of your class look solid, so you’re very close—fix these type checks and you should be good to go.

✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
return f"Distance(km={self.km})"

def __add__(self, other: Distance | int | float) -> Distance: # Виправили ім'я та букви
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isinstance(other, Distance | int | float) is invalid: the second argument to isinstance must be a type or a tuple of types. This will raise TypeError at runtime. Change it to isinstance(other, (Distance, int, float)) to satisfy the requirements while keeping explicit checks.
This violates correctness and must be fixed for __add__ to work.

Comment thread app/main.py Outdated
return NotImplemented

def __iadd__(self, other: Distance | int | float) -> Distance:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as in __add__: Distance | int | float cannot be used as the second argument of isinstance. Use a tuple of types, e.g. isinstance(other, (Distance, int, float)), to avoid TypeError and make in-place addition behave as required.

Comment thread app/main.py Outdated
return NotImplemented

def __mul__(self, other: int | float) -> Distance:
if isinstance(other, int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here isinstance(other, int | float) is also invalid; the union expression is not a type tuple and will cause TypeError. Replace with isinstance(other, (int, float)) so multiplication works correctly for numeric arguments as required.

Comment thread app/main.py Outdated
return NotImplemented

def __truediv__(self, other: int | float) -> Distance:
if isinstance(other, int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same invalid isinstance usage: int | float cannot be passed directly as the second parameter. Use isinstance(other, (int, float)) so division is safe and behaves as the spec expects.

Comment thread app/main.py Outdated
return NotImplemented

def __lt__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comparison methods repeat the same invalid pattern isinstance(other, Distance | int | float). Here in __lt__, switch to isinstance(other, (Distance, int, float)) so comparisons with both Distance and numbers work without raising TypeError. Apply the same fix to __gt__, __le__, __eq__, and __ge__.

@mxrpheusd mxrpheusd left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is still a critical runtime issue with your isinstance usage that breaks core functionality. In multiple methods in app/main.py—including __add__, __iadd__, __mul__, __truediv__, and all comparison methods (__lt__, __gt__, __le__, __eq__, __ge__)—you use patterns like isinstance(other, Distance | int | float) or isinstance(other, int | float). The second argument to isinstance must be a type or a tuple of types, not a union expression, so these calls will raise TypeError at runtime and prevent the class from working correctly with Distance and numeric values. To fix this, update each of these checks to use tuples, for example: isinstance(other, (Distance, int, float)) where you accept both Distance and numbers, and isinstance(other, (int, float)) where only numbers are allowed. Once those isinstance calls are corrected, your implementation should run without errors and will fully meet the task requirements and checklist expectations. Aside from this issue, your handling of initialization, string/representation methods, arithmetic, and comparisons is well done—this is a good solution that just needs this final correctness fix.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
self,
other: Distance | int | float
) -> Distance:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the previous review correction requirement: you still use isinstance(other, Distance | int | float), but the second argument to isinstance must be a type or a tuple of types, not a union expression. Update this to isinstance(other, (Distance, int, float)) so __add__ doesn’t raise TypeError at runtime.

Comment thread app/main.py Outdated
return NotImplemented

def __iadd__(self, other: Distance | int | float) -> Distance:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same isinstance issue as in __add__: Distance | int | float is an invalid second argument and will cause TypeError. Change it to a tuple like isinstance(other, (Distance, int, float)) to make __iadd__ work correctly.

Comment thread app/main.py Outdated
return NotImplemented

def __mul__(self, other: int | float) -> Distance:
if isinstance(other, int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you use isinstance(other, int | float), which is also invalid as a second argument to isinstance. Per the previous review correction requirement, change this to isinstance(other, (int, float)) so multiplication works without errors.

Comment thread app/main.py Outdated
return NotImplemented

def __truediv__(self, other: int | float) -> Distance:
if isinstance(other, int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same invalid isinstance pattern: int | float will cause TypeError. Replace it with isinstance(other, (int, float)) so division behaves correctly and meets the task requirements.

Comment thread app/main.py Outdated
return NotImplemented

def __lt__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comparison methods use the same incorrect isinstance(other, Distance | int | float) pattern. For __lt__, switch to isinstance(other, (Distance, int, float)) to avoid runtime errors and satisfy the requirement to fix invalid isinstance usage.

Comment thread app/main.py Outdated
return NotImplemented

def __gt__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__gt__ repeats the invalid isinstance(other, Distance | int | float) usage. Change this to isinstance(other, (Distance, int, float)) as required by the previous corrections so comparisons work reliably.

Comment thread app/main.py Outdated
return NotImplemented

def __le__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__le__ also uses Distance | int | float inside isinstance, which must be replaced by a tuple: isinstance(other, (Distance, int, float)). Otherwise, comparison with numbers or Distance will raise TypeError.

Comment thread app/main.py Outdated
return NotImplemented

def __eq__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In __eq__, you must fix isinstance(other, Distance | int | float) to isinstance(other, (Distance, int, float)) to comply with the requirement to use tuples of types in isinstance checks.

Comment thread app/main.py Outdated
return NotImplemented

def __ge__(self, other: Distance | int | float) -> bool:
if isinstance(other, Distance | int | float):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__ge__ again uses the invalid Distance | int | float union in isinstance. Update this to isinstance(other, (Distance, int, float)) so greater-or-equal comparisons work without errors and meet the task requirements.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution because your Distance class cleanly satisfies all the functional and style requirements: initialization, string representations, arithmetic, and comparisons all behave correctly for both Distance instances and numeric values. Your __str__ and __repr__ formats are clear and match the required output, and your arithmetic methods (__add__, __iadd__, __mul__, __truediv__) correctly handle mixed operands while keeping the logic easy to read. The comparison methods are implemented consistently and correctly use other.km when comparing to another Distance, while still supporting int and float inputs.

From a technical perspective, using from __future__ import annotations and proper type hints is a strong choice, and your use of isinstance(..., (Distance, int, float)) avoids the earlier union-type pitfall. The two-decimal rounding in __truediv__ aligns exactly with the requirement, and your control flow avoids unnecessary else blocks after return, which matches good Python style guidance. Overall, this is a solid, well-structured implementation that shows good attention to both correctness and readability—well done, and keep building on this style in future exercises.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants