-
Notifications
You must be signed in to change notification settings - Fork 12
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
#98 Fixed overriding variables. #110
base: main
Are you sure you want to change the base?
Conversation
alexanderlazarev0
commented
Jul 9, 2024
Can you, please, revert |
@Lancetnik reset formatting, I had run |
@@ -23,6 +47,7 @@ async def some_func(b: int, c=Depends(dep_func)) -> int: | |||
assert (await some_func("2")) == 7 | |||
|
|||
|
|||
@pytest.mark.skip |
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.
Why are you skipping these tests?
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 skipped them since they fail (as they should if overriding is supported). You can also delete them.
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.
But they should works. Your function signature should be accumulation of the function and all dependencies signature
So,
def dep(a): ...
def another_dep(a, b): ...
def func(a, c, dep1 = Depends(dep1), dep2 = Depends(another_dep)): ...
Has (a, b, c)
signature
In my opinion, your test test_override_depends_with_by_passing_positional
doesn't look correctly due the function has ()
- empty signature
I think, we should allow overrides dependencies by explicit keyword overriding only
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.
Okay just so we are clear we should allow:
def dep(a: int) -> int:
return a + 1
@inject
def func(b: int = Depends(dep)):
return b
func(a=3) # 4
func(b=3) # 3
func(3) # ValidationError
Although in this case func
will still have signature (a,)
and we are passing (b,)
?
From what I can tell, one should allow both signatures -> overloading. I don't see another way to fix #98.
This will come with the side effect that with deeper nesting, for example:
def dep_1_1(d): ...
def dep_1_2(e): ...
@inject
def dep_1(b = Depends(dep_1_1), c = Depends(dep_1_2)): ...
@inject
def func(a = Depends(dep_1)): ...
One would need to allow the signatures : [(a),(b, c), (d, c), (b, e), (d, e)]
, so the number of signatures will grow "exponentially".
Unless I am misunderstanding your comment.
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.
Nope, we don't create a new signature for each depends, we are including it to the original one, so for your last case signature will be (d, e)
only.
I think, we should works this way:
def dep(a: int) -> int:
return a + 1
@inject
def func(b: int = Depends(dep)):
return b
func(a=3) # 4
func(b=3) # 3
func(3) # 4 - the same with `a=3` due our real signature is `(a,)`