diff --git a/spec/ameba/formatter/explain_formatter_spec.cr b/spec/ameba/formatter/explain_formatter_spec.cr index 770d4277..c7aacf9c 100644 --- a/spec/ameba/formatter/explain_formatter_spec.cr +++ b/spec/ameba/formatter/explain_formatter_spec.cr @@ -59,7 +59,7 @@ module Ameba source = Source.new "a = 42", "source.cr" output = explanation(source) output.should contain "DETAILED DESCRIPTION" - output.should contain "TO BE DONE..." + output.should contain "Rule extended description" end it "writes nothing if location not found" do diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 86e992c4..60df2ed0 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -24,6 +24,7 @@ module Ameba end end + # Rule extended description class ErrorRule < Rule::Base properties do description "Always adds an error at 1:1" diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 13ba570c..b27648b0 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -175,8 +175,9 @@ module Ameba::AST node.accept self end - def references?(node : Crystal::Var) - @macro_literals.any?(&.value.includes?(node.name)) + # @[AlwaysInline] + private def includes_reference?(val) + val.to_s.includes?(@reference) end def visit(node : Crystal::ASTNode) @@ -184,23 +185,22 @@ module Ameba::AST end def visit(node : Crystal::MacroLiteral) - !(self.references ||= node.value.includes?(@reference)) + !(@references ||= includes_reference?(node.value)) end def visit(node : Crystal::MacroExpression) - !(self.references ||= node.exp.to_s.includes?(@reference)) + !(@references ||= includes_reference?(node.exp)) end def visit(node : Crystal::MacroFor) - exp, body = node.exp, node.body - !(self.references ||= exp.to_s.includes?(@reference) || - body.to_s.includes?(@reference)) + !(@references ||= includes_reference?(node.exp) || + includes_reference?(node.body)) end def visit(node : Crystal::MacroIf) - !(self.references ||= node.cond.to_s.includes?(@reference) || - node.then.to_s.includes?(@reference) || - node.else.to_s.includes?(@reference)) + !(@references ||= includes_reference?(node.cond) || + includes_reference?(node.then) || + includes_reference?(node.else)) end end diff --git a/src/ameba/ast/visitors/top_level_nodes_visitor.cr b/src/ameba/ast/visitors/top_level_nodes_visitor.cr index 6e606cae..5e5ad913 100644 --- a/src/ameba/ast/visitors/top_level_nodes_visitor.cr +++ b/src/ameba/ast/visitors/top_level_nodes_visitor.cr @@ -12,15 +12,17 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::Require) require_nodes << node + true end - # If a top level node is Crystal::Expressions traverse the children. + # If a top level node is `Crystal::Expressions`, + # then always traverse the children. def visit(node : Crystal::Expressions) true end # A general visit method for rest of the nodes. - # Returns false meaning all child nodes will not be traversed. + # Returns `false`, meaning all child nodes will not be traversed. def visit(node : Crystal::ASTNode) false end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index d376cdc6..c1b16c1a 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -111,7 +111,6 @@ class Ameba::Config # config.formatter = custom_formatter # config.formatter # ``` - # property formatter : Formatter::BaseFormatter do Formatter::DotFormatter.new end @@ -249,17 +248,17 @@ class Ameba::Config {% end %} {% end %} - {% if properties["enabled".id] == nil %} + {% unless properties["enabled".id] %} @[YAML::Field(key: "Enabled")] property? enabled = true {% end %} - {% if properties["severity".id] == nil %} + {% unless properties["severity".id] %} @[YAML::Field(key: "Severity", converter: Ameba::SeverityYamlConverter)] property severity = {{ @type }}.default_severity {% end %} - {% if properties["excluded".id] == nil %} + {% unless properties["excluded".id] %} @[YAML::Field(key: "Excluded")] property excluded : Array(String)? {% end %} diff --git a/src/ameba/formatter/disabled_formatter.cr b/src/ameba/formatter/disabled_formatter.cr index 37865c0f..cf777ee3 100644 --- a/src/ameba/formatter/disabled_formatter.cr +++ b/src/ameba/formatter/disabled_formatter.cr @@ -2,14 +2,14 @@ module Ameba::Formatter # A formatter that shows all disabled lines by inline directives. class DisabledFormatter < BaseFormatter def finished(sources) - output << "Disabled rules using inline directives: \n\n" + output << "Disabled rules using inline directives:\n\n" sources.each do |source| - source.issues.select(&.disabled?).each do |e| - next unless loc = e.location + source.issues.select(&.disabled?).each do |issue| + next unless loc = issue.location output << "#{source.path}:#{loc.line_number}".colorize(:cyan) - output << " #{e.rule.name}\n" + output << " #{issue.rule.name}\n" end end end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index a24c5bd1..eb3b79ee 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -4,8 +4,7 @@ module Ameba::Formatter # A formatter that shows the detailed explanation of the issue at # a specific location. class ExplainFormatter - HEADING = "## " - PREFIX = " " + HEADING_MARKER = "## " include Util @@ -18,13 +17,18 @@ module Ameba::Formatter # Second argument is *location* which indicates the location to explain. # # ``` - # ExplainFormatter.new output, - # file: path, - # line: line_number, - # column: column_number + # ExplainFormatter.new output, { + # file: path, + # line: line_number, + # column: column_number, + # } # ``` def initialize(@output, location) - @location = Crystal::Location.new(location[:file], location[:line], location[:column]) + @location = Crystal::Location.new( + location[:file], + location[:line], + location[:column] + ) end # Reports the explanations at the *@location*. @@ -39,42 +43,59 @@ module Ameba::Formatter end private def explain(source, issue) - rule = issue.rule - return unless location = issue.location - output_title "ISSUE INFO" + output << '\n' + output_title "Issue info" output_paragraph [ - issue.message.colorize(:red).to_s, - location.to_s.colorize(:cyan).to_s, + issue.message.colorize(:red), + location.to_s.colorize(:cyan), ] if affected_code = affected_code(issue, context_lines: 3) - output_title "AFFECTED CODE" + output_title "Affected code" output_paragraph affected_code end + rule = issue.rule + + output_title "Rule info" + output_paragraph "%s of a %s severity" % { + rule.name.colorize(:magenta), + rule.severity.to_s.colorize(rule.severity.color), + } + if rule.responds_to?(:description) - output_title "RULE INFO" - output_paragraph [rule.severity.to_s, rule.name, rule.description] + output_paragraph rule.description end - output_title "DETAILED DESCRIPTION" - output_paragraph(rule.class.parsed_doc || "TO BE DONE...") + rule_doc = colorize_code_fences(rule.class.parsed_doc) + return unless rule_doc + + output_title "Detailed description" + output_paragraph rule_doc + end + + private def colorize_code_fences(string) + return unless string + string + .gsub(/```(.+?)```/m, &.colorize(:dark_gray)) + .gsub(/`(?!`)(.+?)`/, &.colorize(:dark_gray)) end private def output_title(title) - output << HEADING.colorize(:yellow) << title.colorize(:yellow) << '\n' - output << '\n' + output << HEADING_MARKER.colorize(:yellow) + output << title.upcase.colorize(:yellow) + output << "\n\n" end private def output_paragraph(paragraph : String) output_paragraph(paragraph.lines) end - private def output_paragraph(paragraph : Array(String)) + private def output_paragraph(paragraph : Array) paragraph.each do |line| - output << PREFIX << line << '\n' + output << ' ' << line << '\n' end output << '\n' end diff --git a/src/ameba/formatter/flycheck_formatter.cr b/src/ameba/formatter/flycheck_formatter.cr index 936bb001..491567e4 100644 --- a/src/ameba/formatter/flycheck_formatter.cr +++ b/src/ameba/formatter/flycheck_formatter.cr @@ -3,16 +3,16 @@ module Ameba::Formatter @mutex = Mutex.new def source_finished(source : Source) - source.issues.each do |e| - next if e.disabled? - next if e.correctable? && config[:autocorrect]? + source.issues.each do |issue| + next if issue.disabled? + next if issue.correctable? && config[:autocorrect]? - next unless loc = e.location + next unless loc = issue.location @mutex.synchronize do output.printf "%s:%d:%d: %s: [%s] %s\n", - source.path, loc.line_number, loc.column_number, e.rule.severity.symbol, - e.rule.name, e.message.gsub('\n', " ") + source.path, loc.line_number, loc.column_number, issue.rule.severity.symbol, + issue.rule.name, issue.message.gsub('\n', " ") end end end diff --git a/src/ameba/rule/layout/trailing_blank_lines.cr b/src/ameba/rule/layout/trailing_blank_lines.cr index a9767286..dbfa3aa6 100644 --- a/src/ameba/rule/layout/trailing_blank_lines.cr +++ b/src/ameba/rule/layout/trailing_blank_lines.cr @@ -24,7 +24,9 @@ module Ameba::Rule::Layout return if source_lines_size == 1 && last_source_line.empty? last_line_empty = last_source_line.empty? - return if source_lines_size.zero? || (source_lines.last(2).join.presence && last_line_empty) + return if source_lines_size.zero? || + (source_lines.last(2).join.presence && last_line_empty) + if last_line_empty issue_for({source_lines_size, 1}, MSG) else diff --git a/src/ameba/rule/lint/ambiguous_assignment.cr b/src/ameba/rule/lint/ambiguous_assignment.cr index ad7cc9c5..e07dc467 100644 --- a/src/ameba/rule/lint/ambiguous_assignment.cr +++ b/src/ameba/rule/lint/ambiguous_assignment.cr @@ -41,6 +41,7 @@ module Ameba::Rule::Lint op_end_location.column_number - 1 ) op_text = source_between(op_location, op_end_location, source.lines) + return unless op_text return unless MISTAKES.has_key?(op_text) diff --git a/src/ameba/rule/lint/empty_loop.cr b/src/ameba/rule/lint/empty_loop.cr index b58e2366..ee884428 100644 --- a/src/ameba/rule/lint/empty_loop.cr +++ b/src/ameba/rule/lint/empty_loop.cr @@ -52,11 +52,7 @@ module Ameba::Rule::Lint check_node(source, node, node.block) end - def test(source, node : Crystal::While) - check_node(source, node, node.body) if literal?(node.cond) - end - - def test(source, node : Crystal::Until) + def test(source, node : Crystal::While | Crystal::Until) check_node(source, node, node.body) if literal?(node.cond) end diff --git a/src/ameba/rule/lint/hash_duplicated_key.cr b/src/ameba/rule/lint/hash_duplicated_key.cr index c8ffcb4e..5b335edc 100644 --- a/src/ameba/rule/lint/hash_duplicated_key.cr +++ b/src/ameba/rule/lint/hash_duplicated_key.cr @@ -35,7 +35,7 @@ module Ameba::Rule::Lint private def duplicated_keys(entries) entries.map(&.key) .group_by(&.itself) - .tap(&.select! { |_, v| v.size > 1 }) + .select! { |_, v| v.size > 1 } .map { |k, _| k } end end diff --git a/src/ameba/rule/lint/literal_in_condition.cr b/src/ameba/rule/lint/literal_in_condition.cr index d232ca83..294a10f6 100644 --- a/src/ameba/rule/lint/literal_in_condition.cr +++ b/src/ameba/rule/lint/literal_in_condition.cr @@ -33,15 +33,7 @@ module Ameba::Rule::Lint issue_for node, MSG if static_literal?(node.cond) end - def test(source, node : Crystal::If) - check_node source, node - end - - def test(source, node : Crystal::Unless) - check_node source, node - end - - def test(source, node : Crystal::Case) + def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case) check_node source, node end end diff --git a/src/ameba/rule/lint/literal_in_interpolation.cr b/src/ameba/rule/lint/literal_in_interpolation.cr index a8683d12..fce21501 100644 --- a/src/ameba/rule/lint/literal_in_interpolation.cr +++ b/src/ameba/rule/lint/literal_in_interpolation.cr @@ -26,8 +26,8 @@ module Ameba::Rule::Lint def test(source, node : Crystal::StringInterpolation) node.expressions - .select { |e| !e.is_a?(Crystal::StringLiteral) && literal?(e) } - .each { |n| issue_for n, MSG } + .select { |exp| !exp.is_a?(Crystal::StringLiteral) && literal?(exp) } + .each { |exp| issue_for exp, MSG } end end end diff --git a/src/ameba/rule/lint/redundant_string_coercion.cr b/src/ameba/rule/lint/redundant_string_coercion.cr index 0273e6d9..35c5f662 100644 --- a/src/ameba/rule/lint/redundant_string_coercion.cr +++ b/src/ameba/rule/lint/redundant_string_coercion.cr @@ -36,12 +36,12 @@ module Ameba::Rule::Lint end private def string_coercion_nodes(node) - node.expressions.select do |e| - e.is_a?(Crystal::Call) && - e.name == "to_s" && - e.args.size.zero? && - e.named_args.nil? && - e.obj + node.expressions.select do |exp| + exp.is_a?(Crystal::Call) && + exp.name == "to_s" && + exp.args.size.zero? && + exp.named_args.nil? && + exp.obj end end end diff --git a/src/ameba/rule/lint/shadowed_exception.cr b/src/ameba/rule/lint/shadowed_exception.cr index 6c779601..045117bb 100644 --- a/src/ameba/rule/lint/shadowed_exception.cr +++ b/src/ameba/rule/lint/shadowed_exception.cr @@ -45,13 +45,16 @@ module Ameba::Rule::Lint return if rescues.nil? - shadowed(rescues).each { |tp| issue_for tp, MSG % tp.names.join("::") } + shadowed(rescues).each do |path| + issue_for path, MSG % path.names.join("::") + end end private def shadowed(rescues, catch_all = false) traversed_types = Set(String).new - filter_rescues(rescues).each_with_object([] of Crystal::Path) do |types, shadowed| + rescues = filter_rescues(rescues) + rescues.each_with_object([] of Crystal::Path) do |types, shadowed| case when catch_all shadowed.concat(types) @@ -62,7 +65,7 @@ module Ameba::Rule::Lint catch_all = true next else - nodes = types.select { |tp| traverse(tp.to_s, traversed_types) } + nodes = types.select { |path| traverse(path.to_s, traversed_types) } shadowed.concat(nodes) unless nodes.empty? end end diff --git a/src/ameba/rule/lint/shadowing_outer_local_var.cr b/src/ameba/rule/lint/shadowing_outer_local_var.cr index 6a397af8..d61221ec 100644 --- a/src/ameba/rule/lint/shadowing_outer_local_var.cr +++ b/src/ameba/rule/lint/shadowing_outer_local_var.cr @@ -45,11 +45,7 @@ module Ameba::Rule::Lint ] end - def test(source, node : Crystal::ProcLiteral, scope : AST::Scope) - find_shadowing source, scope - end - - def test(source, node : Crystal::Block, scope : AST::Scope) + def test(source, node : Crystal::ProcLiteral | Crystal::Block, scope : AST::Scope) find_shadowing source, scope end diff --git a/src/ameba/rule/lint/syntax.cr b/src/ameba/rule/lint/syntax.cr index 6fbaddda..14e7ba4b 100644 --- a/src/ameba/rule/lint/syntax.cr +++ b/src/ameba/rule/lint/syntax.cr @@ -21,7 +21,7 @@ module Ameba::Rule::Lint class Syntax < Base properties do description "Reports invalid Crystal syntax" - severity Ameba::Severity::Error + severity :error end def test(source) diff --git a/src/ameba/rule/lint/useless_condition_in_when.cr b/src/ameba/rule/lint/useless_condition_in_when.cr index 7cf7784d..2d52f480 100644 --- a/src/ameba/rule/lint/useless_condition_in_when.cr +++ b/src/ameba/rule/lint/useless_condition_in_when.cr @@ -61,12 +61,7 @@ module Ameba::Rule::Lint @parent.accept self end - def visit(node : Crystal::If) - @rule.check_node(@source, @parent, node.cond) - true - end - - def visit(node : Crystal::Unless) + def visit(node : Crystal::If | Crystal::Unless) @rule.check_node(@source, @parent, node.cond) true end diff --git a/src/ameba/rule/metrics/cyclomatic_complexity.cr b/src/ameba/rule/metrics/cyclomatic_complexity.cr index 9b40c2ec..376c188f 100644 --- a/src/ameba/rule/metrics/cyclomatic_complexity.cr +++ b/src/ameba/rule/metrics/cyclomatic_complexity.cr @@ -20,10 +20,12 @@ module Ameba::Rule::Metrics def test(source, node : Crystal::Def) complexity = AST::CountingVisitor.new(node).count + return unless complexity > max_complexity - return unless complexity > max_complexity && (location = node.name_location) - issue_for location, name_end_location(node), - MSG % {complexity, max_complexity} + return unless location = node.name_location + end_location = name_end_location(node) + + issue_for location, end_location, MSG % {complexity, max_complexity} end end end diff --git a/src/ameba/rule/style/is_a_filter.cr b/src/ameba/rule/style/is_a_filter.cr index 6337040d..c2a27df8 100644 --- a/src/ameba/rule/style/is_a_filter.cr +++ b/src/ameba/rule/style/is_a_filter.cr @@ -79,6 +79,7 @@ module Ameba::Rule::Style old = OLD % node.name new = NEW % {node.name, name} msg = MSG % {new, old} + if end_location issue_for(filter_location, end_location, msg) do |corrector| corrector.replace(filter_location, end_location, new) diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index b8bba3cf..83b5ef28 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -39,11 +39,12 @@ module Ameba::Rule::Style Tokenizer.new(source).run do |token| next unless token.type.number? && decimal?(token.raw) - parsed = parse_number token.raw + parsed = parse_number(token.raw) if allowed?(*parsed) && (expected = underscored *parsed) != token.raw location = token.location end_location = location.adjust(column_number: token.raw.size - 1) + issue_for location, end_location, MSG % expected do |corrector| corrector.replace(location, end_location, expected) end @@ -58,21 +59,21 @@ module Ameba::Rule::Style private def allowed?(_sign, value, fraction, _suffix) return true if fraction && fraction.size > 3 - digits = value.chars.select &.to_s.=~ /[0-9]/ + digits = value.chars.select(&.number?) digits.size >= int_min_digits end private def underscored(sign, value, fraction, suffix) - value = slice_digits(value.reverse) { |slice| slice }.reverse - fraction = "." + slice_digits(fraction) { |slice| slice } if fraction + value = slice_digits(value.reverse).reverse + fraction = "." + slice_digits(fraction) if fraction "#{sign}#{value}#{fraction}#{suffix}" end private def slice_digits(value, by = 3) - ([] of String).tap do |slices| + %w[].tap do |slices| value.chars.reject(&.== '_').each_slice(by) do |slice| - slices << (yield slice).join + slices << slice.join end end.join('_') end diff --git a/src/ameba/rule/style/negated_conditions_in_unless.cr b/src/ameba/rule/style/negated_conditions_in_unless.cr index 62e477f7..129ba274 100644 --- a/src/ameba/rule/style/negated_conditions_in_unless.cr +++ b/src/ameba/rule/style/negated_conditions_in_unless.cr @@ -42,7 +42,7 @@ module Ameba::Rule::Style when Crystal::BinaryOp negated_condition?(node.left) || negated_condition?(node.right) when Crystal::Expressions - node.expressions.any? { |e| negated_condition?(e) } + node.expressions.any? { |exp| negated_condition?(exp) } when Crystal::Not true else diff --git a/src/ameba/rule/style/type_names.cr b/src/ameba/rule/style/type_names.cr index c0569987..ebaf23e5 100644 --- a/src/ameba/rule/style/type_names.cr +++ b/src/ameba/rule/style/type_names.cr @@ -66,23 +66,7 @@ module Ameba::Rule::Style issue_for node, MSG % {expected, name} end - def test(source, node : Crystal::ClassDef) - check_node(source, node) - end - - def test(source, node : Crystal::Alias) - check_node(source, node) - end - - def test(source, node : Crystal::LibDef) - check_node(source, node) - end - - def test(source, node : Crystal::EnumDef) - check_node(source, node) - end - - def test(source, node : Crystal::ModuleDef) + def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef) check_node(source, node) end end diff --git a/src/ameba/rule/style/variable_names.cr b/src/ameba/rule/style/variable_names.cr index 44615ab3..c3cded64 100644 --- a/src/ameba/rule/style/variable_names.cr +++ b/src/ameba/rule/style/variable_names.cr @@ -39,15 +39,7 @@ module Ameba::Rule::Style VarVisitor.new self, source end - def test(source, node : Crystal::Var) - check_node source, node - end - - def test(source, node : Crystal::InstanceVar) - check_node source, node - end - - def test(source, node : Crystal::ClassVar) + def test(source, node : Crystal::Var | Crystal::InstanceVar | Crystal::ClassVar) check_node source, node end diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 787874d3..38ce9b82 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -33,6 +33,7 @@ module Ameba @code = corrected_code @lines = nil @ast = nil + true end @@ -46,7 +47,6 @@ module Ameba # source = Ameba::Source.new "a = 1\nb = 2", path # source.lines # => ["a = 1", "b = 2"] # ``` - # getter lines : Array(String) { code.split('\n') } # Returns AST nodes constructed by `Crystal::Parser`. @@ -55,7 +55,6 @@ module Ameba # source = Ameba::Source.new code, path # source.ast # ``` - # getter ast : Crystal::ASTNode do Crystal::Parser.new(code) .tap(&.wants_doc = true) @@ -72,9 +71,9 @@ module Ameba path.ends_with?("_spec.cr") end - # Returns `true` if *filepath* matches the source's path, `false` if it does not. + # Returns `true` if *filepath* matches the source's path, `false` otherwise. def matches_path?(filepath) - path == filepath || path == File.expand_path(filepath) + path.in?(filepath, File.expand_path(filepath)) end end end