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
Type check when comparing nodes #98
Conversation
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.
Good fix; thank you! I especially like the NotImplemented strategem. Here are the 2 little things I noticed.
@@ -90,6 +90,9 @@ def __str__(self): | |||
|
|||
def __eq__(self, other): | |||
"""Support by-value deep comparison with other nodes for testing.""" | |||
if not isinstance(other, Node): | |||
return NotImplemented | |||
|
|||
return (other is not None and |
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.
Since we're doing the isinstance() test above, we can lose the other is not None
clause.
|
||
def test_node_inequality(): | ||
node = Node('text', 'o hai', 0, 5) | ||
assert node != 5 |
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.
For consistency and to work properly under python -O
(rather than getting optimized out), let's import ok_
from nose.tools
and do ok_(node != 5)
. Since we're removing the explicit None
checking above, it might also not be a bad idea to test equality against None
if we're not doing it elsewhere.
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.
Absolutely! I am mostly familiar with pytest and not much with nose, so I just hacked away at it until I had a failing test.
It should be covered by the isinstance above
Thanks again for the good patch! :-D |
I didn't know about the |
This should close #95 by making the
Node
give up comparisons with things that are not of its class.I went with returning
NotImplemented
as that givesother
a chance to do the comparison.I thought about using
But I don't really know what is the preferred style, so I just went with the simpler.