-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use dynamic modules to allow correct call
inheritance
#20
Conversation
@@ -0,0 +1,26 @@ | |||
module Injectable | |||
module CallFacade | |||
def self.[](klass) |
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.
Complex method Injectable::CallFacade::[] (23.6)
define_method :call do |args = {}| | ||
if self.instance_of?(klass) | ||
check_call_definition! | ||
check_missing_arguments!(self.class.required_call_arguments, args) |
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.
Injectable::CallFacade#self.[] calls 'self.class' 2 times
@@ -0,0 +1,26 @@ | |||
module Injectable | |||
module CallFacade |
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.
Injectable::CallFacade has no descriptive comment
@@ -0,0 +1,26 @@ | |||
module Injectable | |||
module CallFacade | |||
def self.[](klass) |
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.
Injectable::CallFacade#self.[] has approx 8 statements
module Injectable | ||
module CallFacade | ||
def self.[](klass) | ||
m = Module.new do |
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.
Injectable::CallFacade#self.[] has the variable name 'm'
def self.[](klass) | ||
m = Module.new do | ||
# Entry point of the service. | ||
# Arguments for this method should be declared explicitly with '.argument' |
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.
Line is too long. [82/80]
@@ -0,0 +1,26 @@ | |||
module Injectable | |||
module CallFacade | |||
def self.[](klass) |
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.
Method has too many lines. [14/10]
@@ -0,0 +1,26 @@ | |||
module Injectable | |||
module CallFacade |
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.
Missing top-level module documentation comment.
# Arguments for this method should be declared explicitly with '.argument' | ||
# and declare this method without arguments | ||
define_method :call do |args = {}| | ||
if self.instance_of?(klass) |
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.
Redundant self
detected.
private | ||
|
||
def check_call_definition! | ||
return if (self.class.ancestors - [Injectable::InstanceMethods]).any? do |ancestor| | ||
ancestor.instance_methods(false).include?(:call) | ||
ancestor.instance_methods(false).include?(:call) && !ancestor.respond_to?(:facade?) |
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.
Line is too long. [91/80]
Code Climate has analyzed commit a7ae61e and detected 12 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
super() | ||
end | ||
|
||
def self.facade? |
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.
Here we can use different ways to recognize a module as a facade. It's anyways important, otherwise check_call_definition!
will give false positives.
This should close #19
With this change, the ancestors of a subclass of a service are:
Each concrete class over Injectable gets a dynamically generated module, with a
:call
implementation thatThis way we have an alternating chain of methods (one in the facade and one in the concrete class) that call each other.
The dynamically generated module does not need to be assigned to a constant, but it helps when reading traces.