-
-
Notifications
You must be signed in to change notification settings - Fork 46.6k
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
compare-method added to Vector class in lib.py #12448
Conversation
linear_algebra/src/lib.py
Outdated
@@ -183,6 +182,16 @@ def angle(self, other: Vector, deg: bool = False) -> float: | |||
else: | |||
return math.acos(num / den) | |||
|
|||
def __eq__(self, vector: object) -> bool: |
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.
Could you move the method higher up in the file, before all the public methods? Having this one dunder method within all the public methods, rather than having it alongside all the other dunder methods, makes it harder to find.
linear_algebra/src/lib.py
Outdated
def __eq__(self, vector: object) -> bool: | ||
""" | ||
performs the comparison between two vectors | ||
""" | ||
if not isinstance(vector, Vector): | ||
return NotImplemented |
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.
def __eq__(self, vector: object) -> bool: | |
""" | |
performs the comparison between two vectors | |
""" | |
if not isinstance(vector, Vector): | |
return NotImplemented | |
def __eq__(self, vector: Vector) -> bool: | |
""" | |
performs the comparison between two vectors | |
""" |
I don't think it makes sense to allow comparison with any object, only with other Vector
s.
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.
I tried writing Vector insted of object but it gave me an error that saying: Argument 1 of "eq" is incompatible with supertype "object"; supertype defines the argument type as "object" [override]
This violates the Liskov substitution principle
Sorry for the inconvenience, I hadn't realized that |
ok, i went back to the previous version |
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.
LGTM, thanks for your changes!
Describe your change:
Checklist: