-
Notifications
You must be signed in to change notification settings - Fork 82
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 optional uniqueness checks for Binder
#235
base: master
Are you sure you want to change the base?
Changes from 5 commits
cda04a3
212d38c
28522ca
3cbe776
8e2fffc
cc8cf6f
9e59d15
5784937
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
Inject, | ||
Injector, | ||
NoInject, | ||
NonUniqueBinding, | ||
Scope, | ||
InstanceProvider, | ||
ClassProvider, | ||
|
@@ -62,12 +63,12 @@ def __init__(self, b: EmptyClass): | |
self.b = b | ||
|
||
|
||
def prepare_nested_injectors(): | ||
def prepare_nested_injectors(unique=False): | ||
def configure(binder): | ||
binder.bind(str, to='asd') | ||
|
||
parent = Injector(configure) | ||
child = parent.create_child_injector() | ||
child = parent.create_child_injector(unique=unique) | ||
return parent, child | ||
|
||
|
||
|
@@ -1583,6 +1584,30 @@ def test_binder_has_implicit_binding_for_implicitly_bound_type(): | |
assert not injector.binder.has_explicit_binding_for(int) | ||
|
||
|
||
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) | ||
Comment on lines
+1587
to
+1593
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want a test for the opposite case, where overriding works without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cswartzvi bump? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @craig8 apologies, I completely forgot about this PR. I will try and push the requested changes this weekend. |
||
|
||
|
||
def test_child_injector_with_uniqueness_checking_overrides_parent_bindings(): | ||
parent, child = prepare_nested_injectors(unique=True) | ||
child.binder.bind(str, to='qwe') | ||
|
||
assert (parent.get(str), child.get(str)) == ('asd', 'qwe') | ||
|
||
|
||
def test_child_injector_with_uniqueness_checking_raises_error(): | ||
_, child = prepare_nested_injectors(unique=True) | ||
child.binder.bind(str, to='qwe') | ||
|
||
with pytest.raises(NonUniqueBinding, match="Binding for 'str' already exists"): | ||
child.binder.bind(str, to='zxc') | ||
|
||
|
||
def test_get_bindings(): | ||
def function1(a: int) -> None: | ||
pass | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That crossed my mind as well, and yes, I think that would be reasonable.