-
Notifications
You must be signed in to change notification settings - Fork 24
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
PoC Injectable type hint #31
Conversation
@@ -203,4 +205,12 @@ def _decorator(_service: ServiceDefinition) -> ServiceResult: | |||
return _decorator(_service) | |||
|
|||
|
|||
__all__ = ["inject"] | |||
class _Injectable: |
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.
This is the clue of this PR.
@@ -203,4 +205,12 @@ def _decorator(_service: ServiceDefinition) -> ServiceResult: | |||
return _decorator(_service) | |||
|
|||
|
|||
__all__ = ["inject"] | |||
class _Injectable: | |||
def __getitem__(self, item: Type[I]) -> I: |
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.
I am not really happy about it, but tried __class_getitem__
and generic class, but IDE is not able to recognize it properly. mypy is throwing errors, and pylint is exploding :D. So it is what it is.
kink/inject.py
Outdated
return item | ||
|
||
|
||
Injectable: TypeAlias = _Injectable() |
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.
TypeAlias
is required by mypy, otherwise, it will start complaining that Injectable is not a valid type.
from kink import Injectable, inject, di | ||
|
||
|
||
def test_injectable() -> None: |
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.
I have used pretty much the simplified scenario from the issue.
@inject | ||
class Service: | ||
@inject() | ||
def run(self, value: str, injected: Injectable[A]) -> str: |
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.
Here we have Injectable
usage.
Awesome! Thanks @dkraczkowski! I did some testing and, for some reason, mypy fails to properly see def foo(x: Injectable[str]):
pass
reveal_type(foo)
# Revealed type is "def (x: Any) -> Any" When I was doing some local testing few days ago, I made my own from typing import _SpecialForm
@_SpecialForm
def Injectable(self, parameters):
# Stolen from `typing.Optional`
arg = _type_check(parameters, f"{self} requires a single type.")
return arg
def foo(x: Injectable[str]):
pass
Revealed type is "def (x: Injectable?[builtins.str]) -> Any" ...however this makes mypy complain that Any thoughts on that? |
Update: I've removed reveal_type(Injectable[IRepository])
# Any Possibly relevant: python/mypy#11501 |
Update 2: seems like some metaclass magic could do the trick: class M(type):
def __getitem__(self, typ: Type[I]) -> I:
return typ
class Injectable(Generic[I], metaclass=M): pass
reveal_type(Injectable[X])
# Revealed type is "def () -> kink.inject.Injectable[1.IRepository]" That still has some issues, trying to find a workaround for it. Reference: python/mypy@e0e9c2e#diff-1c26dc582ed5f36723414080dfb646bc71cf20736b73ae1ccff9b90a5d3befaf |
Okay, I think I've finally found a way to do this: from typing import Generic, TypeVar
from typing_extensions import reveal_type
import inspect
class Foo:
pass
T = TypeVar('T')
class M(type):
def __getitem__(self, item: T) -> T:
return item
class Injectable(Generic[T], metaclass=M):
pass
def foo(a: str, b: Injectable[Foo]):
pass
# mypy:
reveal_type(foo) # Revealed type is "def (a: builtins.str, b: test.Injectable[test.Foo]) -> Any"
# runtime:
print(Injectable[Foo]) # <class '__main__.Foo'>
print(inspect.signature(foo)) # (a: str, b: __main__.Foo) |
I think I finally managed to make it work. Some changes are required though: I = TypeVar('I')
if TYPE_CHECKING:
# Give mypy a generic version of `Injectable[I]` that also inherits `I`.
# Do not implement `__getattr__` to prevent downgrading `Injectable[I]` to `I`.
class Injectable(I, Generic[I]):
pass
else:
# Give python a working version of `Injectable[I]` that actually returns `I`.
# This will convert `Injectable[I]` to `I` during runtime which is compatible with inject logic.
# (Taken from your PR)
class _Injectable:
def __getitem__(self, item: Type[I]) -> I: # FIXME: Shouldn't this be Type[I] -> Type[I]?
return item # type: ignore
Injectable: TypeAlias = _Injectable() # type: ignore[misc]
# Alternative version using metaclasses:
class M(type):
def __getitem__(self, item: Type[I]) -> Type[I]:
return item
class Injectable(metaclass=M):
pass Corresponding plugin code: from mypy.nodes import ARG_OPT
from mypy.plugin import FunctionContext, Plugin
from mypy.types import Instance
# Set this to contain full names of corresponding items
INJECT_FULLNAME = 'kink.inject.inject'
INJECTABLE_FULLNAME = 'kink.inject.Injectable'
class KinkPlugin(Plugin):
def get_function_hook(self, fullname: str):
if fullname == INJECT_FULLNAME:
return self._inject_hook
return super().get_function_hook(fullname)
def _inject_hook(self, ctx: FunctionContext):
try:
func = ctx.arg_types[0][0]
except IndexError:
# FIXME: This is not an `@inject` form, but `@inject()`.
# Do nothing since we don't have the function signature yet.
return ctx.default_return_type
for i, (arg_name, arg_type) in enumerate(zip(func.arg_names, func.arg_types)):
# Check if argument is of type `Injectable[T]`
if (
arg_name not in ('self', 'cls')
and isinstance(arg_type, Instance)
and arg_type.type.fullname == INJECTABLE_FULLNAME
):
# Mark as optional & replace with inner arg
func.arg_kinds[i] = ARG_OPT
# func.arg_types[i] = arg_type.args[0] # Not necessary since Injectable[T] implements T
return func
def plugin(version: str):
return KinkPlugin This still doesn't work with What do you think? |
@dkraczkowski I just gave it some more thought, and I think there's a more universal solution for this. However, I was thinking of adding a different way to mark injected values, but with the help of T = TypeVar('T')
def injected(typ: Type[T]) -> T:
# Stub for setting default values of injected arguments to make checkers happy.
# This eliminates "missing argument" errors.
# This is totally optional - most users will not use this, it's for those who want
# to make mypy & pylint work properly.
pass
# ...
class UserService:
@inject()
def register_new_user(self, username, repo: IRepository = injected(IRepository)):
user = User(username)
repo.save_user(user) This passes Pros:
Cons:
|
@and3rson For some reason, GitHub didn't notify me about your comment o_0. What if we use your first solution with a minor tweak: from typing import _SpecialForm
@_SpecialForm
def _Injectable(self, parameters):
# Stolen from `typing.Optional`
arg = _type_check(parameters, f"{self} requires a single type.")
return arg
Injectable: TypeAlias = _Injectable # type: ignore
def foo(x: Injectable[str]):
pass I think this should sort out mypy and work how we want it too. From my perspective, assigning a default value is not the best idea (not from my perspective, maybe I would need to get used to it). Other explorations were fascinating (thank you for that). I also had a bit of a problem myself sorting it out, so it is practical and not disturbing. Ping me in the issue if I don't respond, Thanks ;) |
OK. I have checked the solution and seems like IDE is not going to regonize it and mypy is complaining a lot. I will think how to solve it differently. |
@dkraczkowski JFYI - I've been using Some docs I've found:
Furthermore, having P.S. I'm not in any way pushing you to accept my proposal with Thanks for the time & efforts you're putting into this! Kink has become our go-to library for dependency injection in one of our new healthcare projects and is going to production next week! |
@and3rson Thank you for the links I will have a look in my spare time. I am really happy to see another projects in production using kink, this is really an awesome news and I wish you all the best with deployment ;). |
A couple of things: |
@dkraczkowski @and3rson Any progress on this? Willing to help but don't want to duplicate effort and need to get this fixed. |
@adamlogan73 I am happy to share the effort, lately I did not had much time to look into issues that the community is facing. |
No description provided.