Add optional uniqueness checks for Binder
#235
Conversation
davidparsson
left a comment
davidparsson
left a comment
There was a problem hiding this comment.
Thanks for your contribution. Your changes look fine to me, but please consider my comments below.
injector/__init__.py
Outdated
| """A dictionary that raises an exception when trying to add duplicate bindings.""" | ||
| def __setitem__(self, key: type, value: Binding) -> None: | ||
| if key in self.data: | ||
| raise NonUniqueBinding(key.__name__) |
There was a problem hiding this comment.
I would like to see a more human readable error message here. Our other error messages help the more novice users understad what is wrong.
There was a problem hiding this comment.
I completely agree! I will add a message that explains "Binding for 'X' already exists". Do you think that is detailed enough, or would you like to seeing some mention of the unique flag?
There was a problem hiding this comment.
I think the first message would be good enough, but that it would be even better to also mention the flag.
injector_test.py
Outdated
| def test_binder_with_uniqueness_checking_raises_error(): | ||
| def configure(binder): | ||
| binder.bind(int, to=123) | ||
| binder.bind(int, to=456) | ||
|
|
||
| with pytest.raises(NonUniqueBinding): | ||
| _ = Injector([configure], unique=True) |
There was a problem hiding this comment.
Good!
I would like to see how the unique flag interacts with parent/child injectors though, both to verify the functionality and as a form of documentation of the expected behavior. I would expect child injectors to be allowed to override binds in parent injectors (and I see no reason to why this should not work). What do you think of adding another test for that?
There was a problem hiding this comment.
No problem! It looks like there are a couple of tests keying on similar concepts - I will borrow from them.
|
|
||
| parent = Injector(configure) | ||
| child = parent.create_child_injector() | ||
| child = parent.create_child_injector(unique=unique) |
There was a problem hiding this comment.
Wouldn't it be even more interesting to add the unique flag to the parent injector as well, to see that they interact as expected, since it's an even stricter case? I think that it should be allowed and work with the current implementation.
There was a problem hiding this comment.
Yup, I agree. Also: I'm wondering if child injectors shouldn't inherit the unique value of the parent injector by default.
There was a problem hiding this comment.
That crossed my mind as well, and yes, I think that would be reasonable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #235 +/- ##
==========================================
+ Coverage 95.20% 95.37% +0.17%
==========================================
Files 1 1
Lines 563 584 +21
Branches 95 99 +4
==========================================
+ Hits 536 557 +21
Misses 20 20
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
davidparsson
left a comment
davidparsson
left a comment
There was a problem hiding this comment.
I'm ready to approve this, just a final question
| def test_injector_with_uniqueness_checking_raises_error(): | ||
| def configure(binder): | ||
| binder.bind(int, to=123) | ||
| binder.bind(int, to=456) | ||
|
|
||
| with pytest.raises(NonUniqueBinding, match="Binding for 'int' already exists"): | ||
| _ = Injector([configure], unique=True) |
There was a problem hiding this comment.
Do we want a test for the opposite case, where overriding works without unique=True?
There was a problem hiding this comment.
@craig8 apologies, I completely forgot about this PR. I will try and push the requested changes this weekend.
5 participants
This PR attempts to address issue #234 by introducing a unique key dictionary in the
Binderwhenunique=True.