Skip to content
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

Clears some rubocop offenses #281

Merged
merged 4 commits into from
Oct 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,23 @@ Metrics/AbcSize:
- 'lib/rubycritic/analysers/smells/flay.rb'
- 'lib/rubycritic/cli/options.rb'

# Offense count: 6
# Offense count: 2
# Configuration parameters: CountComments.
Metrics/MethodLength:
Exclude:
- 'lib/rubycritic/analysers/helpers/ast_node.rb'
- 'lib/rubycritic/analysers/smells/flay.rb'
- 'lib/rubycritic/command_factory.rb'
- 'lib/rubycritic/core/analysed_module.rb'
- 'lib/rubycritic/reporter.rb'
- 'lib/rubycritic/revision_comparator.rb'
- 'lib/rubycritic/platforms/base.rb'

# Offense count: 4
# Offense count: 1
Naming/AccessorMethodName:
Exclude:
- 'lib/rubycritic/analysers/helpers/ast_node.rb'
- 'lib/rubycritic/generators/html/base.rb'
- 'lib/rubycritic/revision_comparator.rb'

# Offense count: 1
Style/ClassVars:
Exclude:
- 'lib/rubycritic/source_control_systems/base.rb'

# Offense count: 3
# Cop supports --auto-correct.
# Configuration parameters: EnforcedStyle, SupportedStyles.
# SupportedStyles: use_perl_names, use_english_names
Style/SpecialGlobalVars:
Enabled: false

# Offense count: 1
Style/MethodMissing:
Exclude:
Expand Down
2 changes: 1 addition & 1 deletion .todo.reek
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ TooManyStatements:
exclude:
- RubyCritic::RakeTask#initialize
- RubyCritic::Analyser::Complexity#run
- Parser::AST::Node#get_module_names
- Parser::AST::Node#module_names
- RubyCritic::Analyser::FlaySmells#run
- RubyCritic::Cli::Application#execute
- RubyCritic::Cli::Options#parse
Expand Down
24 changes: 14 additions & 10 deletions lib/rubycritic/analysers/helpers/ast_node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,32 @@ def recursive_children
end
end

def get_module_names
def module_names
ast_node_children = children.select do |child|
child.is_a?(Parser::AST::Node)
end

children_modules = ast_node_children.flat_map(&:get_module_names)
children_modules = ast_node_children.flat_map(&:module_names)

if MODULE_TYPES.include?(type)
if children_modules.empty?
[module_name]
else
children_modules.map do |children_module|
"#{module_name}::#{children_module}"
end
end
module_names_with_children children_modules
else
children_modules
end
end

private

def module_names_with_children(children_modules)
if children_modules.empty?
[module_name]
else
children_modules.map do |children_module|
"#{module_name}::#{children_module}"
end
end
end

def module_name
name_segments = []
current_node = children[0]
Expand All @@ -63,7 +67,7 @@ def count_nodes_of_type(*)
0
end

def get_module_names
def module_names
[]
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubycritic/analysers/helpers/modules_locator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def first_name
end

def names
names = node.get_module_names
names = node.module_names
if names.empty?
name_from_path
else
Expand Down
19 changes: 6 additions & 13 deletions lib/rubycritic/command_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,18 @@

module RubyCritic
class CommandFactory
COMMAND_CLASS_MODES = %i[version help ci compare default].freeze

def self.create(options = {})
Config.set(options)
command_class(Config.mode).new(options)
end

def self.command_class(mode)
case mode
when :version
require 'rubycritic/commands/version'
Command::Version
when :help
require 'rubycritic/commands/help'
Command::Help
when :ci
require 'rubycritic/commands/ci'
Command::Ci
when :compare_branches
require 'rubycritic/commands/compare'
Command::Compare
mode = mode.to_s.split('_').first.to_sym
if COMMAND_CLASS_MODES.include? mode
require "rubycritic/commands/#{mode}"
Command.const_get(mode.capitalize)
else
require 'rubycritic/commands/default'
Command::Default
Expand Down
2 changes: 1 addition & 1 deletion lib/rubycritic/commands/compare.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def build_details
def critique(branch)
module_collection = AnalysersRunner.new(paths).run
Config.send(:"#{branch}_collection=", module_collection)
RevisionComparator.new(paths).set_statuses(module_collection)
RevisionComparator.new(paths).statuses = module_collection
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubycritic/commands/default.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def execute

def critique
analysed_modules = AnalysersRunner.new(paths).run
RevisionComparator.new(paths).set_statuses(analysed_modules)
RevisionComparator.new(paths).statuses = analysed_modules
end

def report(analysed_modules)
Expand Down
16 changes: 6 additions & 10 deletions lib/rubycritic/reporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,17 @@

module RubyCritic
module Reporter
REPORT_GENERATOR_CLASS_FORMATS = %i[json console lint].freeze

def self.generate_report(analysed_modules)
report_generator_class.new(analysed_modules).generate_report
end

def self.report_generator_class
case Config.format
when :json
require 'rubycritic/generators/json_report'
Generator::JsonReport
when :console
require 'rubycritic/generators/console_report'
Generator::ConsoleReport
when :lint
require 'rubycritic/generators/lint_report'
Generator::LintReport
config_format = Config.format
if REPORT_GENERATOR_CLASS_FORMATS.include? config_format
require "rubycritic/generators/#{config_format}_report"
Generator.const_get("#{config_format.capitalize}Report")
else
require 'rubycritic/generators/html_report'
Generator::HtmlReport
Expand Down
21 changes: 10 additions & 11 deletions lib/rubycritic/revision_comparator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@ def initialize(paths)
@paths = paths
end

def set_statuses(analysed_modules_now)
if Config.source_control_system.revision?
analysed_modules_before = load_cached_analysed_modules
if analysed_modules_before
SmellsStatusSetter.set(
analysed_modules_before.flat_map(&:smells),
analysed_modules_now.flat_map(&:smells)
)
end
end
analysed_modules_now
def statuses=(analysed_modules_now)
return unless Config.source_control_system.revision?

analysed_modules_before = load_cached_analysed_modules
return unless analysed_modules_before

SmellsStatusSetter.set(
analysed_modules_before.flat_map(&:smells),
analysed_modules_now.flat_map(&:smells)
)
end

private
Expand Down
1 change: 1 addition & 0 deletions lib/rubycritic/source_control_systems/base.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require 'English'
require 'shellwords'

module RubyCritic
Expand Down
4 changes: 2 additions & 2 deletions lib/rubycritic/source_control_systems/git.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def git(arg)
end

def self.supported?
git('branch 2>&1') && $?.success?
git('branch 2>&1') && $CHILD_STATUS.success?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is breaking the tests. We must require "English" so that this works (see rubocop/rubocop#1747).

You can add the require to the top level rubycritic module I think.

Copy link
Contributor Author

@harman28 harman28 Oct 8, 2018

Choose a reason for hiding this comment

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

Fixing this.

You can add the require to the top level rubycritic module

I was going to require only in the files that used that variable. Would requiring in the top module be better for some reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just because it might be used elsewhere. But I guess if this is the only file that uses $CHILD_STATUS then we can leave the require here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean: if $CHILD_STATUS is only used on RubyCritic::SourceControlSystem then it can be imported required only in this module.

end

def self.to_s
Expand All @@ -37,7 +37,7 @@ def date_of_last_commit(path)
end

def revision?
head_reference && $?.success?
head_reference && $CHILD_STATUS.success?
end

def head_reference
Expand Down
2 changes: 1 addition & 1 deletion lib/rubycritic/source_control_systems/mercurial.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class Mercurial < Base
register_system

def self.supported?
`hg verify 2>&1` && $?.success?
`hg verify 2>&1` && $CHILD_STATUS.success?
end

def self.to_s
Expand Down
13 changes: 6 additions & 7 deletions test/lib/rubycritic/revision_comparator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
describe RubyCritic::RevisionComparator do
subject { RubyCritic::RevisionComparator.new([]) }

describe '#set_statuses' do
describe '#statuses=' do
context 'in a SCS with :revision? == false' do
before do
RubyCritic::Config.expects(:source_control_system)
Expand All @@ -16,8 +16,7 @@

it 'does not attempt to compare with previous results' do
subject.expects(:load_cached_analysed_modules).never
result = subject.set_statuses([])
result.must_equal([])
subject.statuses = []
end
end

Expand All @@ -36,12 +35,12 @@

it 'does not load them' do
RubyCritic::Serializer.expects(:new).never
subject.set_statuses([])
subject.statuses = []
end

it 'does not invoke RubyCritic::SmellsStatusSetter' do
RubyCritic::SmellsStatusSetter.expects(:set).never
subject.set_statuses([])
subject.statuses = []
end
end

Expand All @@ -53,12 +52,12 @@
end

it 'loads them' do
subject.set_statuses([])
subject.statuses = []
end

it 'invokes RubyCritic::SmellsStatusSetter' do
RubyCritic::SmellsStatusSetter.expects(:set).once
subject.set_statuses([])
subject.statuses = []
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'minitest/autorun'
require 'minitest/around/spec'
require 'minitest/pride'
require 'mocha/mini_test'
require 'mocha/minitest'
require 'ostruct'
require 'diff/lcs'

Expand Down