-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implemented __eq__ function for Kovats and CubicSpline via ComputationMethod class #52
Conversation
def __eq__(self, o: object) -> bool: | ||
return isinstance(o, type(self)) |
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.
Just a note to consider - if in the future you will possibly use nested inheritance
(e.g. create a new Computation Method XY
from Kovats
, where you, for example, change only one of its methods).
class XY(Kovats):
...
Then, this kind of equality check Kovats() == XY()
returns True
, because isinstance
considers also inheritance. Is that desired behavior? (See this SO post for more detials)
A way to change this is to explicitelly compare their types, i.e.
def __eq__(self, o: object) -> bool:
return type(o) == type(self)
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.
Thanks for the comment! My linter complains about the type comparison with type(o) == type(self)
and I am aware of the subclassing issue.
I don't think there will be computation methods derived from any of the derived classes, but this is good to know. I might violate the linter and change the type check to this one which seems to be stricter.
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.
What is the linter's output?
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.
Using type() instead of isinstance() for a typecheck.
It is unclear to me what is the intended use of this equality comparator. Could you please elaborate a bit @hechth? |
The use is to be able to compare them using |
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.
The usage is unclear to me but it works fwiw. Be wary of what is actually being compared when using it though.
Implemented equality comparison in abstract base class based on class identity.
Fixes #50