injector icon indicating copy to clipboard operation
injector copied to clipboard

Add default scope support

Open garar opened this issue 7 years ago • 6 comments

Hello!

I added support for default scope to injector.

This removes a lot of boiler plate, in projects that use other scopes than NoScope by default. For example, a lot of programs, use DI to store services, which are singletons. If this is merged, it will allow, to specify default scope, when creating Injector instance.

Thanks for all your work!

P.S. About version number: I assumed injector uses semantic versioning, this is why I set the new version to 0.14.0. I can correct it if this is necessary.

garar avatar Jul 15 '17 13:07 garar

Nice! What should happen when you make a child injector from a parent with a default scope? Currently it looks like it's ignored but this might not be so intuitive. If the scoped isn't passed in when making the child, then maybe the parent's should be used. Although with scope=None you can't tell if they're not setting the param or set it to None and really want the default equivalent to NoScope so you'll have to have a special flag on create_child_injector like dont_use_parent_default_scope.

juharris avatar Jul 16 '17 01:07 juharris

Although with scope=None you can't tell if they're not setting the param or set it to None and really want the default equivalent to NoScope so you'll have to have a special flag on create_child_injector like dont_use_parent_default_scope.

I don't think this is necessary. The semantics can simply be: "if scope=None, use the parents". If you want NoScope semantics, pass NoScope.

Edit: Forgot to mention I agree with everything else in your comment @juharris, and I like the general idea @garar .

alecthomas avatar Jul 17 '17 05:07 alecthomas

Without getting into the details of this patch and ignoring the parent/child injector consequences for now; I'd just like to say I'm not a massive fan of this. I'm not sure if the added mental overhead (having to remember what default scope Injector is configured with) is worth it. I can easily imagine a position that makes it worth it, mind you, mine just doesn't necessarily align with it.

Additionally it makes it difficult to reuse Injector configuration between Injector instances using different default scopes (say you have a binding between Interface and Implementation with implicit singleton scope thanks to one Injector default scope being singleton - you can't just use it with another Injector that uses different default scope).

jstasiak avatar Jul 17 '17 08:07 jstasiak

@jstasiak I don't think it would be any more confusing than auto_bind, but it's a good point about child injectors... having them with different binding scopes would be confusing.

alecthomas avatar Jul 17 '17 16:07 alecthomas

Hello!

I've updated this pull request with some changes:

I added parent injector support. Now, if you do not explicitly provide scope when creating child injector, parent's scope will be used.

About your comments:

I understand that this is not so clear and if you want to reject this change it is ok. Personally I don't use parent/child functionality, and this is probably why I missed this in the first place.

I also understand @jstasiak comment about reusing bindings/configurations in injectors with different default scopes. This might cause problems/bugs.

Nevertheless, I decided to update my pull request because it's better to talk about something real, you can read it and see how it looks. For me it looks quite good, maybe because, as I mentioned, I don't use parent injectors and my injector configuration and modules are quite simple.

The most important part for me, is that it won't affect existing code.

@alecthomas This is now three commits, I think you have an option to squash them to one when you are going to merge it, or if you prefer I can do it on new branch. I'm not sure, I can update this PR, I probably will have to create new PR.

Thanks!

garar avatar Jul 17 '17 20:07 garar

I do think this is super useful. Sorry I didn't mean to start a whole covfefe over the parent/child thing. It might be interesting to think about how Guice would solve it, although I don't think Guice would like this change since Guice seems to encourage more thought into design as I saw mentioned in forums. I think this change makes sense in Python where you typically have smaller ad-hoc projects.

juharris avatar Jul 18 '17 03:07 juharris