-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Implement "allow" feature for injectable object #122
Conversation
super | ||
end | ||
|
||
private |
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.
Layout/AccessModifierIndentation: Indent access modifiers like private.
def [](*dependency_names) | ||
unless match_by_pattern?(dependency_names) | ||
::Kernel.raise AutoInject::ForbiddenInjectionError, | ||
"injecting #{dependency_names} dependencies forbidden for injector pattern #{@pattern}" |
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.
Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Metrics/LineLength: Line is too long. [101/100]
|
||
# @api private | ||
def allow(pattern) | ||
::Dry::System::AutoInject::Builder.new(@container, pattern: pattern, strategies: @strategies) |
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.
Metrics/LineLength: Line is too long. [103/100]
Hi @davydovanton, thanks for your patience with this PR :) And also for showing me how it might be used when we met last month! After thinking it over, however, I'm not sure if now's the right time to be putting such a specialised feature into dry-system itself, given that at this point we should be looking to round out the existing set of features before getting a 1.0 done. I wonder if this particular feature can be further explored as some kind of add-on extension in the meantime? That could be good in that it'd allow further experimentation with the various ways this kind of feature could work – like I've mentioned to you before, I can certainly see how this feature is useful in certain circumstances, but I feel we could probably do a little more exploration before settling on an ideal approach. Part of me thinks that we could probably do something richer than relying on simple regexp matches on string identifiers... @davydovanton Would you be OK with taking this approach in the meantime (allowing this code to live on outside dry-system itself)? @solnic @flash-gordon did you have any other thoughts? |
I'm closing this one for now since we haven't had active interest in it, and I think it remains quite reasonable to implement outside of dry-system (e.g. you can create an auto_inject injector with this feature, and pass the dry-system container to it directly). After we get past 1.0 and we see (hopefully) wider usage of dry-system, we could consider this again if we see sufficient user interest :) |
Reference to dry-rb/dry-auto_inject#59 and