-
Notifications
You must be signed in to change notification settings - Fork 20
add support for unknown dependencies that provide a default value #99
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
Conversation
maldoinc
left a 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.
Hi @iverberk, thanks for this! Just a few minor comments but functionality-wise this is ready to go.
|
|
||
| class MyService: | ||
| pass | ||
| def test_register_factory_with_unknown_dependency_with_default() -> 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.
Would be good to have some tests that capture the same behavior for configuration injection via Inject(param=)
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'll add 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.
Are the tests satisfactory?
7ef67f4 to
8de5f22
Compare
|
Looks good, thanks! Just need to sort out the linting checks. You can run |
|
linting errors are resolved, at least locally. |
|
The build breaks on Python < 3.10 due to the pydantic_settings library requirements. What would you propose in this case? I think I needed to add the Python >= 3.10 requirement, otherwise Poetry would fail to resolve the dependencies. |
|
Let's skip the pydantic test case for versions older than py3.10. |
|
Not sure what is going on with the 3.9 tests? It works locally with 3.9.25. |
wireup/ioc/service_registry.py
Outdated
| elif not self.is_type_with_qualifier_known(parameter.klass, qualifier=parameter.qualifier_value): | ||
| msg = ( | ||
| f"Parameter '{name}' of {stringify_type(target)} " | ||
| f"depends on an unknown service {stringify_type(parameter.klass)} " |
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.
The error comes from the stringify_type function when called with an optional type. You can get the raw type directly like this: stringify_type(analyze_type(parameter.klass).raw_type)
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.
Do you want me to update all the stringify_type(target) instances to the new form stringify_type(analyze_type(parameter.klass).raw_type)? I'm unable to reproduce this locally with Python 3.9.25, which makes it a bit hard to verify.
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.
Hmm, strange. I tested it with the same version locally and I can reporoduce it. The failing test cases is test_unknown_service_with_default_value
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.
As expected, a configuration error on my system. I installed a a second venv for 3.9 but unbeknownst to me, it was still defaulting to the 3.13 one.
|
Should we skip the Pydantic check for 3.14 also? Pydantic seems to have a compatibility problem with 3.14 at the moment. |
|
Let's skip that for now. |
|
Some edge case we have not addressed here: @service
class Thing:
def __init__(self,
foo: Foo | None,
x = 1,
some_config: Annotated[str | None, Inject(param="test_config")] = None,
*args: Any,
**kwargs: Any
) -> None:
self.foo = foo
self.bar = bar |
|
I'm not sure I follow what I need to do here. Is your example something that wouldn't work with the new changes? I put it in a testcase and it was happy to provide the dependency. Can you make it a little more specific what is currently missing from the code? Thanks! |
|
Yeah, the above will fail since some parameters have no type at all, and stuff like args and kwargs that also have "default" values are validated. We can skip these kind of parameters outright during dependency registration. |
e7ce18a to
9b09f62
Compare
9b09f62 to
41c0275
Compare
|
Looks good, thanks. Just py3.8 lint failing. |
|
Always something :-) Pushed a new commit to fix. |
|
Thank you @iverberk! |
This PR adds support for dependencies of an unknown type that provide a default value, allowing them to be instantiated even if the type is not known in the registry.
Fixes #94