-
Notifications
You must be signed in to change notification settings - Fork 12
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
Sphere class added as primitive geometry #31
Conversation
Stores key configuration via constructor: origin : Point3D Centered origin of the ``Sphere``. radius: float Radius of ``Sphere``.
|
||
def __ne__(self, other) -> bool: | ||
"""Not equals operator for ``Sphere``.""" | ||
return not self.__eq__(other) |
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.
Nitpick:
There's really no reason to use dunder methods (even internally). IMO, the only reason to ever directly call out a dunder method when calling the base method super().__eq__
.
return not self.__eq__(other) | |
return not self == other |
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.
This will be across all open PRs, can address them all based upon to be consistent.
def __eq__(self, other: object) -> bool: | ||
"""Equals operator for ``Sphere``.""" | ||
if not isinstance(other, Sphere): | ||
raise ValueError(f"Comparison of {self} against {other} is not possible.") |
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.
These should be type errors.
raise ValueError(f"Comparison of {self} against {other} is not possible.") | |
raise TypeError(f"Comparison against {type(other)} is not possible. Should be of type {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.
This will be across all open PRs, can address them all based upon to be consistent.
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.
Or put this in your base class.
self._radius = radius | ||
|
||
@property | ||
def origin(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.
origin
and radius
need setters.
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.
How common do we implement classes with immutability in python/pyansys? I was hoping to discuss the possibility of making all geometry primitives immutable.
self._origin = origin | ||
self._radius = radius |
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.
Needs type checking.
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.
This will be across all open PRs, can address them all based upon to be consistent.
|
||
@property | ||
def radius(self): | ||
"""Return the radius of the ``Sphere``.""" |
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.
"""Return the radius of the ``Sphere``.""" | |
"""Radius of the ``Sphere``.""" |
Replaced by #57 |
Stores key configuration via constructor: