-
Notifications
You must be signed in to change notification settings - Fork 12
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
[WIP] add more YARD docs #36
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,19 @@ | ||
# Data class used by {Pippi::Checks::Check} | ||
class CheckDescriptor | ||
|
||
attr_accessor :check, :clazz_to_decorate, :method_sequence, :should_check_subsequent_calls | ||
|
||
# @param [Pippi::Checks::Check] check_object the check associated with this | ||
# CheckDescriptor | ||
# @param [Class] clazz_to_decorate the class who owns the method sequence being | ||
# watched | ||
# @param [Pippi::Checks::MethodSequence] method_sequence the method sequence to | ||
# watch for | ||
def initialize(check_object, clazz_to_decorate, method_sequence) | ||
@clazz_to_decorate = clazz_to_decorate | ||
@method_sequence = method_sequence | ||
@check = check_object | ||
@should_check_subsequent_calls = true | ||
end | ||
|
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ def initialize(check_descriptor) | |
end | ||
|
||
def decorate | ||
# Decorate watched Class with an accessor to the check instance | ||
check_descriptor.clazz_to_decorate.class_exec(check_descriptor.check, self) do |my_check, method_sequence_check_instance| | ||
name = "@_pippi_check_#{my_check.class.name.split('::').last.downcase}" | ||
self.instance_variable_set(name, my_check) | ||
|
@@ -18,14 +19,21 @@ def decorate | |
second_method_decorator = if method_sequence_check_instance.check_descriptor.method_sequence.decorator | ||
method_sequence_check_instance.check_descriptor.method_sequence.decorator | ||
else | ||
# Create a new module which will be used to intercept method calls via Object#extend | ||
Module.new do | ||
descriptor = method_sequence_check_instance.check_descriptor.method_sequence.method2 | ||
# Define a new method which will be used to intercept & decorate the original method call | ||
# Delegate to super after calling decorator | ||
define_method(descriptor) do |*args, &blk| | ||
# Using "self.class" implies that the first method invocation returns the same type as the receiver | ||
# e.g., Array#select returns an Array. Would need to further parameterize this to get | ||
# different behavior. | ||
# | ||
# TODO jbodah - We could use a closure here instead of an instance | ||
# variable which might solve the issue noted above | ||
the_check = self.class.instance_variable_get(name) | ||
the_check.add_problem | ||
# TODO - @tcopeland could you explain this block? | ||
if method_sequence_check_instance.check_descriptor.should_check_subsequent_calls && method_sequence_check_instance.check_descriptor.clazz_to_decorate == self.class | ||
problem_location = caller_locations.find { |c| c.to_s !~ /byebug|lib\/pippi\/checks/ } | ||
the_check.method_names_that_indicate_this_is_being_used_as_a_collection.each do |this_means_its_ok_sym| | ||
|
@@ -38,12 +46,17 @@ def decorate | |
end | ||
|
||
# e.g., "select" in "select followed by size" | ||
# | ||
# Create a new module which will be used to intercept method calls via Module#prepend | ||
first_method_decorator = Module.new do | ||
descriptor = method_sequence_check_instance.check_descriptor.method_sequence.method1 | ||
# Intercept and decorate the original method call | ||
# Dynamically add the second decorator to the return value via Object#extend | ||
define_method(descriptor) do |*args, &blk| | ||
result = super(*args, &blk) | ||
if self.class.instance_variable_get(name) | ||
result.extend second_method_decorator | ||
# TODO - @tcopeland could you explain this block? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tcopeland would you mind explaining what's going on here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jbodah this one is to handle this case:
That is, in this case there's a mutator method that's called between |
||
self.class.instance_variable_get(name).mutator_methods(result.class).each do |this_means_its_ok_sym| | ||
result.define_singleton_method(this_means_its_ok_sym, self.class.instance_variable_get(name).its_ok_watcher_proc(second_method_decorator, method_sequence_check_instance.check_descriptor.method_sequence.method2)) | ||
end | ||
|
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.
@tcopeland would you mind explaining what's going on here?
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.
Hey sorry for the delay in replying @jbodah.
The problem this is trying to solve is illustrated in this sequence of method calls:
Normally pippi would flag that code since
select
is called and thenfirst
is called on the result. But in this case, the result is also used as a collection later - that is, we calleach
on it. So the code can't be simplified down into a single call todetect
.Pippi's logic is more or less to add a
Problem
, and then remove it if needed. There might be a nicer way to do that... like, adding it as a provisional problem and not actually adding it to the report until we confirm it.