feat: add conditional provider activation #286
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduce a conditional provider activation system into the DI layer, allowing providers to be registered with predicates and filtered at module build time based on an activation context, while keeping default behavior unchanged when no conditions are specified. Sequence diagram for module build with conditional provider filteringsequenceDiagram
actor Dev
participant WakuFactory
participant ModuleRegistryBuilder as RegistryBuilder
participant _ActivationBuilder as Builder
participant Module
participant ProviderFilter
participant ConditionalProvider as CondProv
participant Provider
Dev->>WakuFactory: create()
WakuFactory->>RegistryBuilder: new(root_module_type, context, provider_filter)
WakuFactory->>RegistryBuilder: build()
activate RegistryBuilder
RegistryBuilder->>RegistryBuilder: _collect_modules()
RegistryBuilder->>Builder: _build_type_registry(modules)
loop for each ModuleMetadata
alt ProviderSpec is ConditionalProvider
RegistryBuilder->>Builder: register(spec.provided_type)
else ProviderSpec is Provider
RegistryBuilder->>Builder: register(factory.provides.type_hint)
end
end
RegistryBuilder->>RegistryBuilder: _register_modules(post_order)
loop for each ModuleMetadata
RegistryBuilder->>Module: new(module_type, metadata)
RegistryBuilder->>Module: create_provider(context, Builder, ProviderFilter)
activate Module
Module->>ProviderFilter: filter(self.providers, context, module_type, Builder)
activate ProviderFilter
loop for each ProviderSpec
alt spec is ConditionalProvider
ProviderFilter->>ProviderFilter: build ActivationContext
ProviderFilter->>CondProv: when(ctx)
alt activator returns true
ProviderFilter->>CondProv: get provider
CondProv-->>ProviderFilter: provider
ProviderFilter->>Module: include provider
else activator returns false
ProviderFilter->>ProviderFilter: optionally on_skip
end
else spec is Provider
ProviderFilter-->>Module: include provider
end
end
deactivate ProviderFilter
Module->>Module: _ModuleProvider(active_providers)
Module-->>RegistryBuilder: BaseProvider
deactivate Module
RegistryBuilder->>RegistryBuilder: store provider
end
RegistryBuilder-->>WakuFactory: ModuleRegistry
deactivate RegistryBuilder
WakuFactory->>Dev: WakuApplication
Class diagram for conditional provider activation typesclassDiagram
direction LR
class ActivationBuilder {
<<protocol>>
+has_active(type_: Any) bool
}
class ActivationContext {
+dict~Any,Any~ container_context
+ModuleType module_type
+DynamicModule module_type
+Any provided_type
+ActivationBuilder builder
}
class Activator {
<<typealias>>
}
class Has {
+Any type_
+__call__(ctx: ActivationContext) bool
}
class ConditionalProvider {
+Provider provider
+Activator when
+Any provided_type
}
class IProviderFilter {
<<protocol>>
+filter(providers: list~ProviderSpec~, context: dict~Any,Any~, module_type: ModuleType, builder: ActivationBuilder) list~Provider~
}
class ProviderFilter {
+OnSkipCallback on_skip
+filter(providers: list~ProviderSpec~, context: dict~Any,Any~, module_type: ModuleType, builder: ActivationBuilder) list~Provider~
}
class OnSkipCallback {
<<typealias>>
}
class ProviderSpec {
<<typealias>>
}
class _ActivationBuilder {
-set~Any~ _registered_types
+register(type_: Any) void
+has_active(type_: Any) bool
}
class ModuleMetadata {
+list~ProviderSpec~ providers
+list~ModuleType~ imports
+list~ModuleExtension~ extensions
}
class Module {
-BaseProvider _provider
+Sequence~ProviderSpec~ providers
+create_provider(context: dict~Any,Any~, builder: ActivationBuilder, provider_filter: IProviderFilter) BaseProvider
+provider BaseProvider
}
class _ModuleProvider {
+_ModuleProvider(providers: Iterable~Provider~)
}
class ModuleRegistryBuilder {
-ModuleCompiler _compiler
-ModuleType _root_module_type
-dict~Any,Any~ _context
-IProviderFilter _provider_filter
-_ActivationBuilder _builder
+build() ModuleRegistry
+_build_type_registry(modules: list~tuple~ModuleType,ModuleMetadata~~) void
}
class WakuFactory {
-dict~Any,Any~ _context
-IProviderFilter _provider_filter
+create() WakuApplication
}
%% Relationships
Activator --> ActivationContext : takes
Has --> ActivationContext : uses
Has ..|> Activator
ConditionalProvider --> Provider : wraps
ConditionalProvider --> Activator : when
ProviderFilter ..|> IProviderFilter
ProviderFilter --> ConditionalProvider : filters
ProviderFilter --> ActivationContext : creates
_ActivationBuilder ..|> ActivationBuilder
ModuleMetadata --> ProviderSpec : providers
Module --> ModuleMetadata : constructed_from
Module --> ProviderSpec : providers
Module --> BaseProvider : provider
Module --> IProviderFilter : uses
Module --> ActivationBuilder : uses
_ModuleProvider --> Provider : aggregates
ModuleRegistryBuilder --> Module : builds
ModuleRegistryBuilder --> _ActivationBuilder : owns
ModuleRegistryBuilder --> ModuleMetadata : uses
ModuleRegistryBuilder --> IProviderFilter : uses
WakuFactory --> ModuleRegistryBuilder : creates
WakuFactory --> IProviderFilter : configures
WakuFactory --> ActivationContext : supplies_context
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
object_, you computeactual_typebut still pass the originalprovided_type(which may beNone) intoprovider(...)while usingactual_typeonly for theConditionalProvider.provided_type; consider passingactual_typeintoprovideras well so the underlying factory’sprovides.type_hintstays consistent with theConditionalProvider.provided_typeand with the non-conditional behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `object_`, you compute `actual_type` but still pass the original `provided_type` (which may be `None`) into `provider(...)` while using `actual_type` only for the `ConditionalProvider.provided_type`; consider passing `actual_type` into `provider` as well so the underlying factory’s `provides.type_hint` stays consistent with the `ConditionalProvider.provided_type` and with the non-conditional behavior.
## Individual Comments
### Comment 1
<location> `src/waku/di/_providers.py:224` </location>
<code_context>
+
+
+@overload
+def contextual(provided_type: Any, *, scope: Scope = Scope.REQUEST) -> Provider: ...
+
</code_context>
<issue_to_address>
**issue (review_instructions):** `contextual` uses `Any` for `provided_type`, which breaks the "avoid Any" guideline and could be modeled with a generic type argument instead.
The new overloads and implementation for `contextual` take `provided_type: Any`. Given the guidelines to avoid `Any` and use generics/aliases, this API should be generic, e.g.:
```python
_T = TypeVar('_T')
@overload
def contextual(provided_type: type[_T], *, scope: Scope = Scope.REQUEST) -> Provider: ...
```
and propagate `_T` through the return type. That keeps the DI surface strongly typed while still flexible.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*.py`
**Instructions:**
Avoid `Any` type - create proper types instead
</details>
</issue_to_address>
### Comment 2
<location> `tests/di/activation/test_activation_integration.py:21` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 3
<location> `tests/di/activation/test_activation_integration.py:22` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 4
<location> `tests/di/activation/test_conditional_providers.py:18` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 5
<location> `examples/conditional_providers.py:125-126` </location>
<code_context>
def get_user(self, user_id: str) -> str:
cached = self.cache.get(f'user:{user_id}')
if cached:
return cached
user_data = f'User-{user_id}'
self.cache.set(f'user:{user_id}', user_data)
return user_data
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if cached := self.cache.get(f'user:{user_id}'):
```
</issue_to_address>
### Comment 6
<location> `tests/di/activation/test_activation_integration.py:234` </location>
<code_context>
async def test_custom_filter_receives_providers_and_context() -> None:
received: list[tuple[list[ProviderSpec], dict[Any, Any] | None, ModuleType | DynamicModule, ActivationBuilder]] = []
class RecordingFilter(IProviderFilter):
def filter( # noqa: PLR6301
self,
providers: list[ProviderSpec],
context: dict[Any, Any] | None,
module_type: ModuleType | DynamicModule,
builder: ActivationBuilder,
) -> list[Provider]:
received.append((list(providers), context, module_type, builder))
return [p if isinstance(p, Provider) else p.provider for p in providers]
AppModule = create_basic_module(
providers=[scoped(Service)],
name='AppModule',
)
app = WakuFactory(
AppModule,
context={'env': 'test'},
provider_filter=RecordingFilter(),
).create()
async with app, app.container() as container:
await container.get(Service)
assert len(received) >= 1
_providers, ctx, _module_type, _builder = received[0]
assert ctx is not None
assert ctx.get('env') == 'test'
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify sequence length comparison ([`simplify-len-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-len-comparison/))
```suggestion
assert received
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
fadedDexofan
force-pushed
the
feat/provider-activation
branch
from
75a5079 to
ae6db23
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is attempt to re-implement same behavior from draft-state PR in dishka reagento/dishka#561