Skip to content
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

Usage of containers.override decorator #301

Closed
JarnoRFB opened this issue Oct 16, 2020 · 9 comments
Closed

Usage of containers.override decorator #301

JarnoRFB opened this issue Oct 16, 2020 · 9 comments

Comments

@JarnoRFB
Copy link
Contributor

Hi,
I think in previous versions is was possible to do something like:

containers.override(BaseContainer):
class OverrideContainer: ....

Afterwards

c = BaseContainer()

would contain all the overrides.
However, the typing now complains that containers.override wants a Container instance and not a class. Has overriding containers on the class level been deprecated, as it also seems to have been removed from the documentation?

@rmk135 rmk135 self-assigned this Oct 16, 2020
@rmk135
Copy link
Member

rmk135 commented Oct 16, 2020

Hey @JarnoRFB ,

Yep, I was going to deprecate overriding of containers on declarative level. Thanks for bringing this up. Let me check what's going on with it now.

@rmk135
Copy link
Member

rmk135 commented Oct 17, 2020

I have fixed typed stubs (see #302 ). The fix is published as 4.0.2.

I thought about declarative container decorators and decided to deprecate them in favour of similar operations on instance level. The main reason for this is design improvement by avoiding a dualism in overriding container providers. I see the future of the framework on instance level for the containers. Please, upgrade to:

c = BaseContainer()
c.override(OverrideContainer())

or:

c = BaseContainer(provider1=..., provider2=...)

I have added deprecation warnings for both @containers.override() and @containers.copy() (see #303). They don't go anywhere and still operational. I'll keep them at least till next major version. The change is published as 4.0.3.

@rmk135 rmk135 closed this as completed Oct 17, 2020
@JarnoRFB
Copy link
Contributor Author

@rmk135 Thanks for the clarification! I still have a few things I can't wrap my head around yet. I mainly use dependency injection, because I want to reuse my application in slightly different contexts, e.g. for different customers that use different data formats.
I often build CLI applications using click. Until now I usually instantiated the base container in the main module, then in the extension module I just imported the main function and overrode the container on the declarative level and reused the original main function.

When I now try to switch to instance level overriding, I make the main function require the container as an argument and return a CLI function. In the extension module, I override the container on the instance level, pass it to the main function and call the resulting CLI function.
So far so good, but one problem I stumbled across is that I did not find a way to access configuration defined in the base container, which I did before by just referencing the base class. Moreover, I want to reused the configuration code as that might even be coupled to the CLI.

Because code is often clearer that explaining I created this little example https://github.com/JarnoRFB/di-overriding. Hope that makes sense.

Glad to hear, if an obvious solution exists or if I am using things not the way the are meant to be used.

@rmk135
Copy link
Member

rmk135 commented Oct 17, 2020

@JarnoRFB , thanks for sharing code sample. That perfectly demonstrates the problem.

Yep, @override does its work great in that case. The main challenge as you said the configuration part. I'll try to play with the sample a bit on a weekend.

@rmk135 rmk135 reopened this Oct 18, 2020
@rmk135
Copy link
Member

rmk135 commented Oct 18, 2020

@JarnoRFB I've made a PR with some refactoring to the sample repo JarnoRFB/di-overriding#1. I consolidated containers and used Selector provider as a switch. How does it look to you?

@JarnoRFB
Copy link
Contributor Author

@rmk135 thanks a lot for looking at this and even presenting a possible solution. However, I believe that using Selector does not solve the fundamental problem, because it requires the base container to have knowledge about possible classes that will be used for extensions and every point at which one will possibly be inserted. I need to be able to package the base module and then write the extension module with the base module as a dependency without having to touch the base module again.

The main challenge for this right now seems not to be the dynamic container overriding, but the sharing of configuration. In Spring, which was my first contact with dependency injection, I believe that is solved using kind of a global configuration, as you can access properties from configurations everywhere using ${config.property} syntax.

Maybe it would then make sense to enable sharing configuration across containers, so that when one writes
config = providers.Configuration("config") in two different containers that are later merged through overriding, one would still access the same configuration. This configuration could still be namespaced by the argument to providers.Configuration.

@rmk135
Copy link
Member

rmk135 commented Oct 19, 2020

Agree.

Well, there is no easy way to extend the configuration that way right now. I like the idea and will think of that as a future improvement.

Having no good alternative for that case I think that the right thing is to "un-deprecate" @containers.override() and get the documentation back at least until there is a good alternative. I'll get it done by the end of the week.

I appreciate your feedback and arguments. Thanks for bringing this up. My sincere respect and gratitude. If you have any other thought - you're welcome anytime.

@JarnoRFB
Copy link
Contributor Author

@rmk135 Yes I believe that "un-deprecating" @containers.override() makes probably sense, until we have a better solution. Thanks a lot for going through the discussion!

@rmk135
Copy link
Member

rmk135 commented Oct 25, 2020

Done in 4.1.0.

Btw, there are two new providers: https://python-dependency-injector.ets-labs.org/providers/resource.html and https://python-dependency-injector.ets-labs.org/providers/dict.html.

Closing the issue. Comment / re-open if needed.

@rmk135 rmk135 closed this as completed Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants