Conversation
jstasiak
left a comment
There was a problem hiding this comment.
Hey, thanks for the patch. Can you provide a unit test for this?
netaddr/eui/__init__.py
Outdated
| raise NotRegisteredError('OUI %r not registered!' % (oui,)) | ||
|
|
||
| def __hash__(self): | ||
| """:return: hash of this OUI object suitable for dict keys, sets etc""" |
There was a problem hiding this comment.
Let's drop this comment, the meaning of the value returned by __hash__() is rather standard.
There was a problem hiding this comment.
Will do. That was a copy-and-paste from the EUI.__hash__.
netaddr/tests/eui/test_eui.py
Outdated
| def test_oui_constructor(): | ||
| oui = OUI(524336) | ||
| assert str(oui) == '08-00-30' | ||
| assert hash(oui) == 524336 |
There was a problem hiding this comment.
We need a real-world hash-capability demonstration here. If the original issue is about using OUI objects as dictionary keys let's use that to check if it works, we shouldn't be hardcoding specific hash values (those can't be depended on etc.).
There was a problem hiding this comment.
Fair enough, let me know if you want me to drop the hash asserts in the new one. Broke it out into a different func to not clutter the test_oui_constructor.
There was a problem hiding this comment.
Thank you! Yes, please drop those two asserts and I'm happy to merge this.
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
==========================================
+ Coverage 86.74% 89.78% +3.03%
==========================================
Files 48 44 -4
Lines 4558 3190 -1368
Branches 659 381 -278
==========================================
- Hits 3954 2864 -1090
+ Misses 424 254 -170
+ Partials 180 72 -108
Continue to review full report at Codecov.
|
|
Thank you! |
Closes #224