-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Added dependency injection #1662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really clean code, love it!
Just a few small changes, not sure if all are correct 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, i didn't know ObjectIdentifier
but i agree with @activcoding is we can use a identifier, we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks decent enough, I might add something after the current comments have been processed/fixed.
However, what I would absolutely want with this PR is extra documentation in the Documentation.docc
. As those cannot contain diagrams, code snippets would be nice. Next to that, if this is merged, we need to add a wiki page for, where we can have those diagrams.
Lastly, I'm not sure if a DependencyInjection folder in features is the right place to put this? Like, it's not really a feature feature, more of a support library. What do others think?
Refactors and made ServiceContainer thread safe
I'm thinking we can make a section titled
I agree, I moved it under the Utils folder. I made a Service folder for the FileService PR where all the services are intended to go. Since this is directly related to the services maybe we should move it there? Thoughts? |
Thanks for the great suggestions, made an update and requesting a re-review.
Update: Looks like it's working now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really excited to get this started(Thanks for the good docs, saved me a lot of thinking...)!
By the way, have you had a chance to test the code with the new ObjectIdentifier(.:)
?
This PR doesn't have any services to test it but I am using this most up to date implementation in the experimental branch and its working pretty well! |
@matthijseikelenboom What changes were you thinking of adding? |
Description
Added the dependency injection system using property wrappers. Right now, this is the simplest way of adding dependency injection, over the macro based solutions. The decision to use a macro based DI can be discussed in the future, and changing DI systems will be an easy refactor for this code.
Related Issues
Checklist
Screenshots