-
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
Add the ability to diff #286
Conversation
src/picosvg/geometric_types.py
Outdated
|
||
def __add__(self, other: "Vector") -> "Point": | ||
"""Return Point translated by other Vector""" | ||
if isinstance(other, Vector): | ||
return self.__class__(self.x + other.x, self.y + other.y) | ||
return NotImplemented | ||
raise ValueError(f"Other must be a Vector, not a {type(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.
Changes here are due to typechecker complaining; I don't want the return type to be Point or 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.
but the canonical way to implement math operator overrides is exactly to return NotImplemented when the other type isn't among the ones which the current method is capable of handling, this enables the interpreter to check if the same operator is implemented by the other side of the expression, see https://docs.python.org/3/library/numbers.html#implementing-the-arithmetic-operations
I'm surprised that pytype doesn't automatically support this..
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.
there's even an explicit test for this in pytype test suite:
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.
Huh. TIL. I will revert this part.
"Add the ability to diff" -- before reading the code, I thought it was about diffing two svg files.. |
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.
by the way, what are you hacking on?
I wanted to colorize Ewert. Horrible hackery @ https://github.com/rsheeter/colorize. Haven't slapped gradients on it yet but I still quite like the result: |
Handy when hacking on things