-
Notifications
You must be signed in to change notification settings - Fork 95
fix: Multibind scopes #284
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
Changes from all commits
11042c7
bdbf66c
1032e80
efc62e1
4d57f11
b4d9fcb
37e071e
11b52e2
2d45015
9faa32a
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 |
|---|---|---|
|
|
@@ -22,24 +22,25 @@ | |
| import threading | ||
| import types | ||
| from abc import ABCMeta, abstractmethod | ||
| from collections import namedtuple | ||
| from dataclasses import dataclass | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Any, | ||
| Callable, | ||
| cast, | ||
| Dict, | ||
| Generator, | ||
| Generic, | ||
| get_args, | ||
| Iterable, | ||
| List, | ||
| Optional, | ||
| overload, | ||
| Set, | ||
| Tuple, | ||
| Type, | ||
| TypeVar, | ||
| TYPE_CHECKING, | ||
| Union, | ||
| cast, | ||
| get_args, | ||
| overload, | ||
| ) | ||
|
|
||
| try: | ||
|
|
@@ -52,13 +53,13 @@ | |
| # canonical. Since this typing_extensions import is only for mypy it'll work even without | ||
| # typing_extensions actually installed so all's good. | ||
| if TYPE_CHECKING: | ||
| from typing_extensions import _AnnotatedAlias, Annotated, get_type_hints | ||
| from typing_extensions import Annotated, _AnnotatedAlias, get_type_hints | ||
| else: | ||
| # Ignoring errors here as typing_extensions stub doesn't know about those things yet | ||
| try: | ||
| from typing import _AnnotatedAlias, Annotated, get_type_hints | ||
| from typing import Annotated, _AnnotatedAlias, get_type_hints | ||
| except ImportError: | ||
| from typing_extensions import _AnnotatedAlias, Annotated, get_type_hints | ||
| from typing_extensions import Annotated, _AnnotatedAlias, get_type_hints | ||
|
|
||
|
|
||
| __author__ = 'Alec Thomas <[email protected]>' | ||
|
|
@@ -340,39 +341,60 @@ def __repr__(self) -> str: | |
|
|
||
|
|
||
| @private | ||
| class ListOfProviders(Provider, Generic[T]): | ||
| class MultiBinder(Provider, Generic[T]): | ||
| """Provide a list of instances via other Providers.""" | ||
|
|
||
| _providers: List[Provider[T]] | ||
| _multi_bindings: List['Binding'] | ||
|
|
||
| def __init__(self) -> None: | ||
| self._providers = [] | ||
| def __init__(self, parent: 'Binder') -> None: | ||
| self._multi_bindings = [] | ||
| self._binder = Binder(parent.injector, auto_bind=False, parent=parent) | ||
|
|
||
| def append(self, provider: Provider[T]) -> None: | ||
| self._providers.append(provider) | ||
| def append(self, provider: Provider[T], scope: Type['Scope']) -> None: | ||
| # HACK: generate a pseudo-type for this element in the list. | ||
| # This is needed for scopes to work properly. Some, like the Singleton scope, | ||
| # key instances by type, so we need one that is unique to this binding. | ||
| pseudo_type = type(f"multibind-type-{id(provider)}", (provider.__class__,), {}) | ||
|
Comment on lines
+354
to
+357
Collaborator
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. Could you please elaborate a bit on in what case this is needed? Is it required for the case when multiple different types are bound with the singleton scope in the same multibound type? Just like
Collaborator
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. I quickly tried to reproduce the problem without the pseudo-type, by instead passing the resolved type, which is
Collaborator
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. After digging in the code for I while, I rather think this is the reason why the hack is needed. Could the reason be that the scope is applied to the binding of
Contributor
Author
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.
My intention was to generate a key that is unique to a particular multi-binding, that is, for each call to binder.multibind(list[Plugin], to=PluginA, scope=singleton)
binder.multibind(list[Plugin], to=PluginA) # bind to the same class againThen my expectation is that I get two instances of the same class. If I request the list again, then the first instance is a singleton but the second is not. injector = Injector([configure])
list1 = injector.get(List[Plugin])
list2 = injector.get(List[Plugin])
assert list1[0] is list2[0]
assert list1[1] is not list2[1]I get why types have been used as scope keys, because that makes sense for regular bindings. But multi-bindings are a different beast, so we're kinda reduced to ramming a square peg into a round hole by fabricating a type for each multibind call.
Collaborator
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. Right! If I recall correctly, the testsst doing that with lists still passed without the pseudo type, but not for dicts. I believe creating a new injector (or binder) for each |
||
| self._multi_bindings.append(Binding(pseudo_type, provider, scope)) | ||
|
|
||
| def get_scoped_providers(self, injector: 'Injector') -> Generator[Provider[T], None, None]: | ||
| for binding in self._multi_bindings: | ||
| if ( | ||
| isinstance(binding.provider, ClassProvider) | ||
| and binding.scope is NoScope | ||
| and self._binder.parent | ||
| and self._binder.parent.has_explicit_binding_for(binding.provider._cls) | ||
|
Collaborator
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. I'm planning on removing the Do you have any argument against doing this? All tests still pass.
Collaborator
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. Sorry, but I have changed my mind. The scope applied to a binding should only be applied for that binding. This is how it works for regular bindings, so this is how it should work for multibinds as well. The special case checking for explicit bindings (above) should be removed. However,
Collaborator
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. I have removed the logic above and merged it already, and also added a failing test case for the I would have really liked to get a release out today, but I don't think I can do it, and it might very well take a while before I have a larger chunk of time to spend on this again. |
||
| ): | ||
| parent_binding, _ = self._binder.parent.get_binding(binding.provider._cls) | ||
davidparsson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| scope_binding, _ = self._binder.parent.get_binding(parent_binding.scope) | ||
| else: | ||
| scope_binding, _ = self._binder.get_binding(binding.scope) | ||
| scope_instance: Scope = scope_binding.provider.get(injector) | ||
| provider_instance = scope_instance.get(binding.interface, binding.provider) | ||
| yield provider_instance | ||
|
|
||
| def __repr__(self) -> str: | ||
| return '%s(%r)' % (type(self).__name__, self._providers) | ||
| return '%s(%r)' % (type(self).__name__, self._multi_bindings) | ||
|
|
||
|
|
||
| class MultiBindProvider(ListOfProviders[List[T]]): | ||
| class MultiBindProvider(MultiBinder[List[T]]): | ||
| """Used by :meth:`Binder.multibind` to flatten results of providers that | ||
| return sequences.""" | ||
|
|
||
| def get(self, injector: 'Injector') -> List[T]: | ||
| result: List[T] = [] | ||
| for provider in self._providers: | ||
| for provider in self.get_scoped_providers(injector): | ||
| instances: List[T] = _ensure_iterable(provider.get(injector)) | ||
| result.extend(instances) | ||
| return result | ||
|
|
||
|
|
||
| class MapBindProvider(ListOfProviders[Dict[str, T]]): | ||
| class MapBindProvider(MultiBinder[Dict[str, T]]): | ||
| """A provider for map bindings.""" | ||
|
|
||
| def get(self, injector: 'Injector') -> Dict[str, T]: | ||
| map: Dict[str, T] = {} | ||
| for provider in self._providers: | ||
| for provider in self.get_scoped_providers(injector): | ||
| map.update(provider.get(injector)) | ||
| return map | ||
|
|
||
|
|
@@ -387,7 +409,11 @@ def get(self, injector: 'Injector') -> Dict[str, T]: | |
| return {self._key: self._provider.get(injector)} | ||
|
|
||
|
|
||
| _BindingBase = namedtuple('_BindingBase', 'interface provider scope') | ||
| @dataclass | ||
| class _BindingBase: | ||
| interface: type | ||
| provider: Provider | ||
| scope: Type['Scope'] | ||
davidparsson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| @private | ||
|
|
@@ -531,44 +557,51 @@ def multibind( | |
|
|
||
| :param scope: Optional Scope in which to bind. | ||
| """ | ||
| if interface not in self._bindings: | ||
| provider: ListOfProviders | ||
| if ( | ||
| isinstance(interface, dict) | ||
| or isinstance(interface, type) | ||
| and issubclass(interface, dict) | ||
| or _get_origin(_punch_through_alias(interface)) is dict | ||
| ): | ||
| provider = MapBindProvider() | ||
| else: | ||
| provider = MultiBindProvider() | ||
| binding = self.create_binding(interface, provider, scope) | ||
| self._bindings[interface] = binding | ||
| else: | ||
| binding = self._bindings[interface] | ||
| provider = binding.provider | ||
| assert isinstance(provider, ListOfProviders) | ||
|
|
||
| if isinstance(provider, MultiBindProvider) and isinstance(to, list): | ||
| multi_binder = self._get_multi_binder(interface) | ||
| if isinstance(multi_binder, MultiBindProvider) and isinstance(to, list): | ||
| try: | ||
| element_type = get_args(_punch_through_alias(interface))[0] | ||
| except IndexError: | ||
| raise InvalidInterface( | ||
| f"Use typing.List[T] or list[T] to specify the element type of the list" | ||
| ) | ||
| for element in to: | ||
| provider.append(self.provider_for(element_type, element)) | ||
| elif isinstance(provider, MapBindProvider) and isinstance(to, dict): | ||
| element_binding = self.create_binding(element_type, element, scope) | ||
| multi_binder.append(element_binding.provider, element_binding.scope) | ||
| elif isinstance(multi_binder, MapBindProvider) and isinstance(to, dict): | ||
| try: | ||
| value_type = get_args(_punch_through_alias(interface))[1] | ||
| except IndexError: | ||
| raise InvalidInterface( | ||
| f"Use typing.Dict[K, V] or dict[K, V] to specify the value type of the dict" | ||
| ) | ||
| for key, value in to.items(): | ||
| provider.append(KeyValueProvider(key, self.provider_for(value_type, value))) | ||
| element_binding = self.create_binding(value_type, value, scope) | ||
| multi_binder.append(KeyValueProvider(key, element_binding.provider), element_binding.scope) | ||
| else: | ||
| provider.append(self.provider_for(interface, to)) | ||
| element_binding = self.create_binding(interface, to, scope) | ||
| multi_binder.append(element_binding.provider, element_binding.scope) | ||
|
|
||
| def _get_multi_binder(self, interface: type) -> MultiBinder: | ||
| multi_binder: MultiBinder | ||
| if interface not in self._bindings: | ||
| if ( | ||
| isinstance(interface, dict) | ||
| or isinstance(interface, type) | ||
| and issubclass(interface, dict) | ||
| or _get_origin(_punch_through_alias(interface)) is dict | ||
| ): | ||
| multi_binder = MapBindProvider(self) | ||
| else: | ||
| multi_binder = MultiBindProvider(self) | ||
| binding = self.create_binding(interface, multi_binder) | ||
| self._bindings[interface] = binding | ||
| else: | ||
| binding = self._bindings[interface] | ||
| assert isinstance(binding.provider, MultiBinder) | ||
| multi_binder = binding.provider | ||
|
|
||
| return multi_binder | ||
|
|
||
| def install(self, module: _InstallableModuleType) -> None: | ||
| """Install a module into this binder. | ||
|
|
@@ -611,10 +644,10 @@ def create_binding( | |
| self, interface: type, to: Any = None, scope: Union['ScopeDecorator', Type['Scope'], None] = None | ||
| ) -> Binding: | ||
| provider = self.provider_for(interface, to) | ||
| scope = scope or getattr(to or interface, '__scope__', NoScope) | ||
| scope = scope or getattr(to or interface, '__scope__', None) | ||
| if isinstance(scope, ScopeDecorator): | ||
| scope = scope.scope | ||
| return Binding(interface, provider, scope) | ||
| return Binding(interface, provider, scope or NoScope) | ||
|
|
||
| def provider_for(self, interface: Any, to: Any = None) -> Provider: | ||
| base_type = _punch_through_alias(interface) | ||
|
|
@@ -696,7 +729,7 @@ def get_binding(self, interface: type) -> Tuple[Binding, 'Binder']: | |
| # The special interface is added here so that requesting a special | ||
| # interface with auto_bind disabled works | ||
| if self._auto_bind or self._is_special_interface(interface): | ||
| binding = ImplicitBinding(*self.create_binding(interface)) | ||
| binding = ImplicitBinding(**self.create_binding(interface).__dict__) | ||
| self._bindings[interface] = binding | ||
| return binding, self | ||
|
|
||
|
|
@@ -817,7 +850,7 @@ def __repr__(self) -> str: | |
| class NoScope(Scope): | ||
| """An unscoped provider.""" | ||
|
|
||
| def get(self, unused_key: Type[T], provider: Provider[T]) -> Provider[T]: | ||
| def get(self, key: Type[T], provider: Provider[T]) -> Provider[T]: | ||
davidparsson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return provider | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.