implement stricter 1-to-1 checking #26
Conversation
jab
commented
Nov 28, 2015
jab
commented
- raise ValueExistsException on insert new key associated with existing value
- stifled by forceput just like CollapseException was
- ValueExistsException replaces CollapseException since it's more general
- rename collapsingbidict loosebidict now that ValueExistsException has replaced CollapseException
- add forceupdate method for bulk forceput
- update docs and tests
- raise ValueExistsException on insert new key associated with existing value - stifled by forceput just like CollapseException - rename collapsingbidict loosebidict now that it's loose in both cases - add forceupdate method for bulk forceput - ValueExistsException and CollapseException inherit from new base class BidictException
- replace CollapseException with ValueExistsException, which trumps it - configure py.test to run doctests in docs dir as well as tests dir
|
@lordmauve thinking of merging this, but will wait if you're still interested in giving feedback first. |
|
Thanks. Yes, I'd be interested in looking through it before you merge
it. I'll have a look tomorrow.
|
|
Great, thanks for taking a look, and looking forward to hearing what you think. |
| A mutable bidict which always uses forcing put operations | ||
| so that it never raises :class:`ValueExistsException`. | ||
| """ | ||
| def _put(self, key, val): |
There was a problem hiding this comment.
Tiny performance optimisation, but
_put = bidict.forceput
would remove function call overhead without increasing code duplication. I imagine _put() will get called a lot in loops, so this adds up.
There was a problem hiding this comment.
Originally had that but changed it in favor of dynamic method lookup for more extensibility. Worth it?
There was a problem hiding this comment.
You're right. We shouldn't make changes like this unless we've profiled what the possible speedups might be.
|
Generally, this looks superb! |
|
@lordmauve Thanks for reviewing and for all the great additional suggestions. (And apologies for not responding sooner, some more time-sensitive work came up.) The latest version of this branch incorporates all your feedback. Would you like to take another look before I merge? |
|
https://bidict.readthedocs.org/en/strict/ has also been updated with the latest. |
|
btw I think I also improved the code in the course of implementing these changes (see the new kwargs and factoring of |
|
Hey @lordmauve, would like to merge this and get a release out tomorrow if possible. But can wait till you can 👍 if you think you can get to it this week? |
|
Looking now... |
| return | ||
| if oldval is not _missing: | ||
| if overwrite_val: | ||
| del self._bwd[oldval] |
There was a problem hiding this comment.
Isn't there the potential here that this del will happen, and then the ValueExistsException gets raised in the other leg, leaving the map inconsistent?
There was a problem hiding this comment.
Excellent catch @lordmauve, thank you. Added tests to catch this and fixed this bug (and a couple others the tests turned up!) in 3d8bb18.
<3 u hypothesis /cc @DRMacIver ;)
|
@lordmauve I think this is finally ready to merge! Thanks so much for all your helpful review. I just rebased the entire branch onto master as a single patch, new PR in #28. Will merge and cut a new release tomorrow. If you'd like to take one last look before I do, would love your final 👍. And thanks again for all your great suggestions, this next release of bidict will definitely be the best yet. |
|
Great work! I'll have a look properly later. |
|
Awesome, thanks @lordmauve! Looking forward. |
