feat: add wireup.instance() helper for registering existing instances#105
Conversation
maldoinc
left a comment
There was a problem hiding this comment.
👋 Hi there! Thanks for the PR! This is a great addition and fills a gap in the API nicely. The implementation looks solid and the tests cover the key scenarios well.
I pulled this locally and ran the tests. Everything functions as expected! I just have a couple of small items to address to get the CI green and the code fully compliant with the project's standards.
1. Formatting
It looks like the new files need to be formatted to match the project style. The CI failed on the formatting check. You can run:
make format
# or
uv run ruff format .This will fix the make lint failure.
2. Typing
There is a mypy error in wireup/_instance.py because we're attaching an attribute to a function, which strict typing forbids by default for Callable.
wireup/_instance.py:35: error: "Callable[[], T]" has no attribute "__wireup_registration__" [attr-defined]
You can fix this by adding a type ignore, similar to how it's done in wireup/_annotations.py:
_instance_provider.__wireup_registration__ = InjectableDeclaration( # type: ignore[attr-defined]
obj=_instance_provider,
...
)3. Verification
I verified that this works even with Generic types (like List[int]), which is awesome.
# verified locally
container = create_sync_container(injectables=[
instance([1, 2], as_type=List[int])
])Great work! looking forward to merging this.
There was a problem hiding this comment.
Since this is clearly vibecoded I decided to automate the review which you can see is just as terrible as the implementation.
When I create these issues on GitHub it's not because I can't write slop fast enough but rather to encourage new contributors with good first tickets and for them to learn and get some more practical experience. I'm not against using this per se but at least do take some time to go through the repo and get an understanding.
| from wireup import instance | ||
| from wireup._annotations import InjectableDeclaration | ||
|
|
||
| class TestInstanceProvider(unittest.TestCase): |
| # The container requires a return type annotation to determine what is being provided. | ||
| _instance_provider.__annotations__["return"] = as_type | ||
|
|
||
| _instance_provider.__wireup_registration__ = InjectableDeclaration( |
There was a problem hiding this comment.
No need to actually do this here, you can just add @injectable in the factory you create and the decorator will create the registration.
|
|
||
| def instance( | ||
| obj: T, | ||
| as_type: type[Any] | None = None, |
There was a problem hiding this comment.
If this is required then it shouldn't be made optional with a default value.
|
|
||
|
|
||
| def instance( | ||
| obj: T, |
There was a problem hiding this comment.
Anything other than obj should be keyword only to make the api more readable in this case and make the registration more explicit.
|
Thanks for the context. That makes sense, and I appreciate the goal of keeping issues beginner-friendly. I’m still actively reading through the docs and taking time to understand the repo structure and design decisions. That said, I do have a reasonably high-level understanding of the project wire-up and the dependency injection flow, which is what I relied on here. I was careful not to push anything that could introduce bad or unsafe code. The AI edits were only used for small refinements, not as a substitute for understanding, and I intentionally held off on making any changes I wasn’t confident about. I’ll keep digging deeper into the codebase and overall context before proposing further changes. Thanks for the feedback. @maldoinc |
|
@DarshanCode2005 let me know if you still are interested in taking this to the finish line |
|
Sure, I’m interested in completing this. I was a bit tied up with some other work, but I’ll try to finish it by today. Thanks! |
There was a problem hiding this comment.
Thanks for this. This is almost there, there's just a tiny issue.
Due to the created factory returning a TypeVar, wireup cannot do it's usual type checks on whether as_type matches obj and as_type.
See this example to repro
class A: pass
class B: pass
wireup.create_sync_container(injectables=[instance(A(), as_type=B)])You can fix it, by updating both __signature__ and __annotations__["return"] after creating it. Then the existing registry validation machinery will work unchanged. Let's also add the above example as a test asserting it should raise type mismatch error.
| @@ -0,0 +1,70 @@ | |||
| from typing import Annotated | |||
|
@DarshanCode2005 you can run |
|
Thank you @DarshanCode2005 |
Closes #102
Summary
This PR introduces a new helper,
wireup.instance(...), which allows direct registration of pre-existing objects (e.g. database connections, settings, legacy singletons) with the Wireup container—without requiring a factory function.What’s included
wireup.instance(obj, as_type, qualifier=None)Behavior & guarantees
as_typeis required and validated(type, qualifier)registrations failTests added
All tests are passing.
Related discussion
Implements the feature discussed in #94 (comment).