Skip to content

Commit

Permalink
Create an analyzer to encapsulate all specs analysis
Browse files Browse the repository at this point in the history
tldr; moved a ton of code out of the linter into another class.

TODOS:
- Create a results mixin
- Separate out the validation into two sections for both consumers and
  specs
- Make sure documentation is good
  • Loading branch information
joshkalpin committed Dec 10, 2013
1 parent b63f2e0 commit 07bb50e
Show file tree
Hide file tree
Showing 5 changed files with 226 additions and 131 deletions.
72 changes: 4 additions & 68 deletions lib/cocoapods-core/specification/linter.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'cocoapods-core/specification/linter/result'
require 'cocoapods-core/specification/linter/analyzer'

module Pod
class Specification
Expand Down Expand Up @@ -149,10 +150,9 @@ def perform_all_specs_analysis
current_spec.available_platforms.each do |platform|
@consumer = Specification::Consumer.new(current_spec, platform)
run_all_specs_validation_hooks
validate_file_patterns
check_tmp_arc_not_nil
check_if_spec_is_empty
check_install_hooks
analyzer = Analyzer.new(self, @consumer)
analyzer.analyze
add_results(analyzer.results)
@consumer = nil
end
end
Expand Down Expand Up @@ -353,12 +353,6 @@ def perform_github_source_checks(s)
end
end

#-----------------------------------------------------------------------#

# @!group All specs validation helpers

private

# Performs validations related to the `compiler_flags` attribute.
#
def _validate_compiler_flags(flags)
Expand All @@ -367,64 +361,6 @@ def _validate_compiler_flags(flags)
end
end

# Checks the attributes that represent file patterns.
#
# @todo Check the attributes hash directly.
#
def validate_file_patterns
attributes = DSL.attributes.values.select(&:file_patterns?)
attributes.each do |attrb|
patterns = consumer.send(attrb.name)
if patterns.is_a?(Hash)
patterns = patterns.values.flatten(1)
end
patterns.each do |pattern|
if pattern.start_with?('/')
error "File patterns must be relative and cannot start with a " \
"slash (#{attrb.name})."
end
end
end
end

# @todo remove in 0.18 and switch the default to true.
#
def check_tmp_arc_not_nil
if consumer.requires_arc.nil?
warning "A value for `requires_arc` should be specified until the " \
"migration to a `true` default."
end
end

# Check empty subspec attributes
#
def check_if_spec_is_empty
methods = %w[ source_files resources preserve_paths dependencies vendored_libraries vendored_frameworks ]
empty_patterns = methods.all? { |m| consumer.send(m).empty? }
empty = empty_patterns && consumer.spec.subspecs.empty?
if empty
error "The #{consumer.spec} spec is empty (no source files, " \
"resources, preserve paths, vendored_libraries, " \
"vendored_frameworks dependencies or subspecs)."
end
end

# Check the hooks
#
def check_install_hooks
unless consumer.spec.pre_install_callback.nil?
warning "The pre install hook of the specification DSL has been " \
"deprecated, use the `resource_bundles` or the " \
"`prepare_command` attributes."
end

unless consumer.spec.post_install_callback.nil?
warning "The post install hook of the specification DSL has been " \
"deprecated, use the `resource_bundles` or the " \
" `prepare_command` attributes."
end
end

# Returns whether the frameworks are valid
#
# @params frameworks [Array<String>]
Expand Down
133 changes: 133 additions & 0 deletions lib/cocoapods-core/specification/linter/analyzer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
require 'cocoapods-core/specification/linter/result'

module Pod
class Specification
class Linter
class Analyzer

attr_reader :consumer

attr_reader :results

attr_reader :linter

def initialize(linter, consumer)
@linter = linter
@consumer = consumer
@results = []
end

def analyze
validate_file_patterns
check_tmp_arc_not_nil
check_if_spec_is_empty
check_install_hooks
end

private

# Checks the attributes that represent file patterns.
#
# @todo Check the attributes hash directly.
#
def validate_file_patterns
attributes = DSL.attributes.values.select(&:file_patterns?)
attributes.each do |attrb|
patterns = consumer.send(attrb.name)
if patterns.is_a?(Hash)
patterns = patterns.values.flatten(1)
end
patterns.each do |pattern|
if pattern.start_with?('/')
error "File patterns must be relative and cannot start with a " \
"slash (#{attrb.name})."
end
end
end
end

# @todo remove in 0.18 and switch the default to true.
#
def check_tmp_arc_not_nil
if consumer.requires_arc.nil?
warning "A value for `requires_arc` should be specified until the " \
"migration to a `true` default."
end
end

# Check empty subspec attributes
#
def check_if_spec_is_empty
methods = %w[ source_files resources preserve_paths dependencies vendored_libraries vendored_frameworks ]

This comment has been minimized.

Copy link
@alloy

alloy Dec 10, 2013

Member

Instead of hardcoded attributes, can this be moved into the DSL properties of said attributes?

This comment has been minimized.

Copy link
@alloy

alloy Dec 10, 2013

Member

i.e. add a :allows_blank => false option.

This comment has been minimized.

Copy link
@joshkalpin

joshkalpin Dec 10, 2013

Author Member

I'll look into it.

This comment has been minimized.

Copy link
@fabiopelosin

fabiopelosin Dec 10, 2013

Member

They are the file patterns attributes + the dependencies and - the preserve_paths (which I'm wondering why it is there).

This comment has been minimized.

Copy link
@joshkalpin

joshkalpin Dec 11, 2013

Author Member

@irrationalfab I'm not fully understanding your comment. I organized this file to be (almost) all of the methods being called under the perform_all_specs_analysis portion of the linter. I can move things around if it makes sense but I'm not sure where else this method would live.

empty_patterns = methods.all? { |m| consumer.send(m).empty? }
empty = empty_patterns && consumer.spec.subspecs.empty?
if empty
error "The #{consumer.spec} spec is empty (no source files, " \
"resources, preserve paths, vendored_libraries, " \
"vendored_frameworks dependencies or subspecs)."
end
end

# Check the hooks
#
def check_install_hooks
unless consumer.spec.pre_install_callback.nil?
warning "The pre install hook of the specification DSL has been " \
"deprecated, use the `resource_bundles` or the " \
"`prepare_command` attributes."
end

unless consumer.spec.post_install_callback.nil?
warning "The post install hook of the specification DSL has been " \
"deprecated, use the `resource_bundles` or the " \
" `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
end
19 changes: 17 additions & 2 deletions lib/cocoapods-core/specification/linter/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ def to_s
end
end

private

# Adds an error result with the given message.
#
# @param [String] message
Expand All @@ -63,6 +61,23 @@ 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)
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
Expand Down
70 changes: 70 additions & 0 deletions spec/specification/linter/analyzer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
require File.expand_path('../../../spec_helper', __FILE__)

module Pod
describe Specification::Linter::Analyzer do

describe 'File patterns & Build settings' do

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))
end

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

matched = results.select do |result|
values.all? do |value|
result.message.downcase.include?(value.downcase)
end
end

matched.size.should == 1
end

it "checks if any file patterns is absolute" do
@spec.source_files = '/Classes'
@analyzer.analyze
message_should_include('patterns', 'relative', 'source_files')
end

it "checks if a specification is empty" do
consumer = Specification::Consumer
consumer.any_instance.stubs(:source_files).returns([])
consumer.any_instance.stubs(:resources).returns({})
consumer.any_instance.stubs(:preserve_paths).returns([])
consumer.any_instance.stubs(:subspecs).returns([])
consumer.any_instance.stubs(:dependencies).returns([])
consumer.any_instance.stubs(:vendored_libraries).returns([])
consumer.any_instance.stubs(:vendored_frameworks).returns([])
@analyzer.analyze
message_should_include('spec', 'empty')
end

xit "requires that the require_arc value is specified until the switch to a true default" do
# TODO the default value is invalidating this test
@consumer.requires_arc = nil
@analyzer.analyze
message = @analyzer.results.first.message
message.should.include('`requires_arc` should be specified')
end

it "checks if the pre install hook has been defined" do
@spec.pre_install {}
@analyzer.analyze
message_should_include('pre install hook', 'deprecated')
end

it "checks if the post install hook has been defined" do
@spec.post_install {}
@analyzer.analyze
message_should_include('post install hook', 'deprecated')
end
end
end
end
Loading

0 comments on commit 07bb50e

Please sign in to comment.