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

Add default scope support #68

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

garar
Copy link

@garar garar commented Jul 15, 2017

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.

@juharris
Copy link

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.

@alecthomas
Copy link
Collaborator

alecthomas commented Jul 17, 2017

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 .

@jstasiak
Copy link
Collaborator

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).

@alecthomas
Copy link
Collaborator

@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.

@garar
Copy link
Author

garar commented Jul 17, 2017

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!

@juharris
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants