Skip to content

Commit

Permalink
Convert the result helpers to a mixin
Browse files Browse the repository at this point in the history
  • Loading branch information
@joshkalpin
joshkalpin committed Dec 10, 2013
1 parent 07bb50e commit d22bca6
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 114 deletions.
8 changes: 3 additions & 5 deletions lib/cocoapods-core/specification/linter.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class Specification
#
class Linter

include ResultHelpers

# @return [Specification] the specification to lint.
#
attr_reader :spec
Expand Down Expand Up @@ -63,10 +65,6 @@ def lint

public

# @return [Array<Result>] all the results generated by the Linter.
#
attr_reader :results

# @return [Array<Result>] all the errors generated by the Linter.
#
def errors
Expand Down Expand Up @@ -150,7 +148,7 @@ def perform_all_specs_analysis
current_spec.available_platforms.each do |platform|
@consumer = Specification::Consumer.new(current_spec, platform)
run_all_specs_validation_hooks
analyzer = Analyzer.new(self, @consumer)
analyzer = Analyzer.new(@consumer)
analyzer.analyze
add_results(analyzer.results)
@consumer = nil
Expand Down
55 changes: 4 additions & 51 deletions lib/cocoapods-core/specification/linter/analyzer.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,9 @@ class Specification
class Linter
class Analyzer

attr_reader :consumer

attr_reader :results

attr_reader :linter
include Linter::ResultHelpers

def initialize(linter, consumer)
@linter = linter
def initialize(consumer)
@consumer = consumer
@results = []
end
Expand All @@ -26,6 +21,8 @@ def analyze

private

attr_reader :consumer

# Checks the attributes that represent file patterns.
#
# @todo Check the attributes hash directly.
Expand Down Expand Up @@ -83,50 +80,6 @@ def check_install_hooks
" `prepare_command` attributes."
end
end

# Adds an error result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def error(message)
add_result(:error, message)
end

# Adds an warning result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def warning(message)
add_result(:warning, message)
end

# Adds a result of the given type with the given message. If there is a
# current platform it is added to the result. If a result with the same
# type and the same message is already available the current platform is
# added to the existing result.
#
# @param [Symbol] type
# The type of the result (`:error`, `:warning`).
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def add_result(type, message)
result = results.find { |r| r.type == type && r.message == message }
unless result
result = Result.new(type, message)
results << result
end
result.platforms << consumer.platform_name if consumer
end
end
end
end
Expand Down
112 changes: 59 additions & 53 deletions lib/cocoapods-core/specification/linter/result.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,65 +39,71 @@ def to_s
end
end

# Adds an error result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def error(message)
add_result(:error, message)
end
module ResultHelpers
# @return [Array<Result>] all of the generated results.
#
attr_reader :results

# Adds an warning result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def warning(message)
add_result(:warning, message)
end
# Adds an error result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def error(message)
add_result(:error, message)
end

# Adds an warning result with the given message.
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def warning(message)
add_result(:warning, message)
end

# Merges results passed in with the current results
#
# @param [Array<Resul>] results
# The results to be merged.
#
# @return [void]
#
def add_results(results)
results.each do |result|
if result.type == :warning
warning(result.message)
else
error(result.message)
# Merges results passed in with the current results
#
# @param [Array<Result>] results
# The results to be merged.
#
# @return [void]
#
def add_results(results)
results.each do |result|
if result.type == :warning
warning(result.message)
else
error(result.message)
end
end
end
end

# Adds a result of the given type with the given message. If there is a
# current platform it is added to the result. If a result with the same
# type and the same message is already available the current platform is
# added to the existing result.
#
# @param [Symbol] type
# The type of the result (`:error`, `:warning`).
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def add_result(type, message)
result = results.find { |r| r.type == type && r.message == message }
unless result
result = Result.new(type, message)
results << result
# Adds a result of the given type with the given message. If there is a
# current platform it is added to the result. If a result with the same
# type and the same message is already available the current platform is
# added to the existing result.
#
# @param [Symbol] type
# The type of the result (`:error`, `:warning`).
#
# @param [String] message
# The message of the result.
#
# @return [void]
#
def add_result(type, message)
result = results.find { |r| r.type == type && r.message == message }
unless result
result = Result.new(type, message)
results << result
end
result.platforms << consumer.platform_name if consumer
end
result.platforms << consumer.platform_name if consumer
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions spec/specification/linter/analyzer_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ module Pod
before do
fixture_path = 'spec-repos/test_repo/Specs/BananaLib/1.0/BananaLib.podspec'
podspec_path = fixture(fixture_path)
@linter = Specification::Linter.new(podspec_path)
@spec = @linter.spec
@analyzer = Specification::Linter::Analyzer.new(@linter, @spec.consumer(:ios))
linter = Specification::Linter.new(podspec_path)
@spec = linter.spec
@analyzer = Specification::Linter::Analyzer.new(@spec.consumer(:ios))
end

def message_should_include(*values)
@linter.lint
results = @linter.results
@analyzer.analyze
results = @analyzer.results
results.should.not.be.nil

matched = results.select do |result|
Expand Down

4 comments on commit d22bca6

@fabiopelosin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer a separated class to increase encapsulation, however I know that it is a bit of a stretch. @alloy what is your take?

@alloy
Copy link
Member

@alloy alloy commented on d22bca6 Dec 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not immediately clear to me why a mixin helps here. Can that be clarified?

@joshkalpin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the analyzer and the linter need to produce results and they need to be of the same form. I was trying to avoid duplicating code and adding in more complexity.

@alloy
Copy link
Member

@alloy alloy commented on d22bca6 Dec 12, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I wonder, aren’t there more places where we essentially have lists of values that are namespaced? (i.e. like :error and :warning in this case?) If so, then maybe we can abstract that in a class that has a merge method and use that instead, which also cleans up the merging code, which seems to be the bulk of the duplication.

Please sign in to comment.