-
Notifications
You must be signed in to change notification settings - Fork 82
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
base: master
Are you sure you want to change the base?
Conversation
Nice! |
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 . |
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 I don't think it would be any more confusing than |
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! |
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. |
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.