ivankorobkov / python-inject

Python dependency injection
Apache License 2.0
671 stars 77 forks source link

Injector configuration #65

Closed chkoar closed 8 months ago

chkoar commented 4 years ago

Great work!

IMHO configure, configure_once and clear_and_configure could be merged to just configure.

configure should not raise an exception. If the intention is to inform the user that the injector is already configured, the warnings module could be used. clear_and_configure could be achieved by using a clear argument in configure, having as default whatever you want. configure_once could be the default (or not) behaviour of configure. The same can be applied by having an extra argument.

I may missing something cause I am new to the library but that's my input.

ivankorobkov commented 4 years ago

Hi!

I'm sorry for the delay, github was down yesterday.

IMHO configure, configure_once and clear_and_configure could be merged to just configure.

I agree, these can be params in configure. However, we still need configure_once and clear_and_configure to maintain backwards compatibility. Internally, they can call configure with specific params.

If the intention is to inform the user that the injector is already configured, the warnings module could be used.

Trying to configure (i.e. replace) an already configured injector is a programming error. Skipping it and giving just a warning will lead to different hard-to-debug bugs.

chkoar commented 4 years ago

I agree, these can be params in configure. However, we still need configure_once and clear_and_configure to maintain backwards compatibility. Internally, they can call configure with specific params.

I agree. This usually involves a deprecation cycle.

Trying to configure (i.e. replace) an already configured injector is a programming error. Skipping it and giving just a warning will lead to different hard-to-debug bugs.

Having explicitly configure(config, clear=True) would avoid that, right?

ivankorobkov commented 4 years ago

Having explicitly configure(config, clear=True) would avoid that, right?

Yes. It should replace the injector instead.

gabis-precog commented 3 years ago

Trying to configure (i.e. replace) an already configured injector is a programming error. Skipping it and giving just a warning will lead to different hard-to-debug bugs.

Would maybe be nice to allow overriding an binding while inside the configure method. example:

(sorry about the formatting, couldn't figure out how to properly insert multi-line code)

def default_config(binder): binder.bind('a', 1)` binder.bind('b', 1)

def config(binder): binder.install(default_config) binder.bind('a', 2) # override to default

ivankorobkov commented 3 years ago

@gabis-precog Hi! Thank you for your comment.

Long time ago I used Guice in Java, a dependency injector by Google. It had overrides. On the one hand, they solved some problems and allowed more flexible injector configuration. On the other hand, with overrides in multiple modules it becomes hard to reason about application and its dependency graph. You just lose track of what and where is initialized, and you lose isolation.

python-inject is specifically made to be as simple as possible. It’s a feature 🙂

An injector configuration is just a callable. You can always implement your own logic on top of it if you really need it.

NikiTsv commented 8 months ago

@ivankorobkov I really love the library and its simplicity. It solved a lot of problems in my project and I thank you for that. However when testing you need to swap some dependencies and making a whole new configuration from scratch just to swap one service with a mock gets out of hand. I think a simply utilizing an 'override=True' param to the install method will solve a lot of problems for a lot of people.

jell-o-fishi commented 8 months ago

My 2 cents: This is the workaround I place for this in every project I work on. Seems to work so far:

def allow_binder_override_during_config():
    def _check_class(self, cls: inject.Binding) -> None:
        if cls is None:
            raise inject.InjectorException('Binding key cannot be None')

        if self._is_forward_str(cls):
            ref = ForwardRef(cls)
            if ref in self._bindings:
                raise inject.InjectorException('Duplicate forward binding, i.e. "int" and int, key=%s', cls)

    inject.Binder._check_class = _check_class

allow_binder_override_during_config()  # NOTE: Intentionally called on module level to fix injector override logic
ivankorobkov commented 8 months ago

Hi,

@NikiTsv thank you for your proposal. I you make a pull request with this change, I can merge it and release the next version.

NikiTsv commented 8 months ago

Hi,

@NikiTsv thank you for your proposal. I you make a pull request with this change, I can merge it and release the next version.

okay, will do, thanks!

NikiTsv commented 8 months ago

@ivankorobkov made a PR feel free to take a look and comment or adjust if needed.

ivankorobkov commented 8 months ago

Released https://pypi.org/project/inject/5.1.0/

@chkoar added support for configure(clear=True) and configure(once=True) as you proposed.

chkoar commented 8 months ago

That’s great!