Merge pull request #312 from crystal-ameba/Sija/refactors-round-3

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

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"

src/ameba/ast/variabling/variable.cr

@@ -175,8 +175,9 @@ module Ameba::AST
         node.accept self
       end
 
-      def references?(node : Crystal::Var)
+      # @[AlwaysInline]
-        @macro_literals.any?(&.value.includes?(node.name))
+      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
+        !(@references ||= includes_reference?(node.exp) ||
-        !(self.references ||= exp.to_s.includes?(@reference) ||
+                          includes_reference?(node.body))
-                              body.to_s.includes?(@reference))
       end
 
       def visit(node : Crystal::MacroIf)
-        !(self.references ||= node.cond.to_s.includes?(@reference) ||
+        !(@references ||= includes_reference?(node.cond) ||
-                              node.then.to_s.includes?(@reference) ||
+                          includes_reference?(node.then) ||
-                              node.else.to_s.includes?(@reference))
+                          includes_reference?(node.else))
       end
     end
 

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

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 %}

src/ameba/formatter/disabled_formatter.cr

@@ -5,11 +5,11 @@ module Ameba::Formatter
       output << "Disabled rules using inline directives:\n\n"
 
       sources.each do |source|
-        source.issues.select(&.disabled?).each do |e|
+        source.issues.select(&.disabled?).each do |issue|
-          next unless loc = e.location
+          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

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 = "## "
+    HEADING_MARKER = "## "
-    PREFIX  = " "
 
     include Util
 
@@ -18,13 +17,18 @@ module Ameba::Formatter
     # Second argument is *location* which indicates the location to explain.
     #
     # ```
-    # ExplainFormatter.new output,
+    # ExplainFormatter.new output, {
     #   file:   path,
     #   line:   line_number,
-    #   column: column_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,
+        issue.message.colorize(:red),
-        location.to_s.colorize(:cyan).to_s,
+        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.description
-        output_paragraph [rule.severity.to_s, rule.name, rule.description]
       end
 
-      output_title "DETAILED DESCRIPTION"
+      rule_doc = colorize_code_fences(rule.class.parsed_doc)
-      output_paragraph(rule.class.parsed_doc || "TO BE DONE...")
+      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 << HEADING_MARKER.colorize(:yellow)
-      output << '\n'
+      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

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|
+      source.issues.each do |issue|
-        next if e.disabled?
+        next if issue.disabled?
-        next if e.correctable? && config[:autocorrect]?
+        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,
+            source.path, loc.line_number, loc.column_number, issue.rule.severity.symbol,
-            e.rule.name, e.message.gsub('\n', " ")
+            issue.rule.name, issue.message.gsub('\n', " ")
         end
       end
     end

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

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)
 

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)
+    def test(source, node : Crystal::While | Crystal::Until)
-      check_node(source, node, node.body) if literal?(node.cond)
-    end
-
-    def test(source, node : Crystal::Until)
       check_node(source, node, node.body) if literal?(node.cond)
     end
 

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

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)
+    def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case)
-      check_node source, node
-    end
-
-    def test(source, node : Crystal::Unless)
-      check_node source, node
-    end
-
-    def test(source, node : Crystal::Case)
       check_node source, node
     end
   end

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) }
+        .select { |exp| !exp.is_a?(Crystal::StringLiteral) && literal?(exp) }
-        .each { |n| issue_for n, MSG }
+        .each { |exp| issue_for exp, MSG }
     end
   end
 end

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|
+      node.expressions.select do |exp|
-        e.is_a?(Crystal::Call) &&
+        exp.is_a?(Crystal::Call) &&
-          e.name == "to_s" &&
+          exp.name == "to_s" &&
-          e.args.size.zero? &&
+          exp.args.size.zero? &&
-          e.named_args.nil? &&
+          exp.named_args.nil? &&
-          e.obj
+          exp.obj
       end
     end
   end

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

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)
+    def test(source, node : Crystal::ProcLiteral | Crystal::Block, scope : AST::Scope)
-      find_shadowing source, scope
-    end
-
-    def test(source, node : Crystal::Block, scope : AST::Scope)
       find_shadowing source, scope
     end
 

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)

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)
+      def visit(node : Crystal::If | Crystal::Unless)
-        @rule.check_node(@source, @parent, node.cond)
-        true
-      end
-
-      def visit(node : Crystal::Unless)
         @rule.check_node(@source, @parent, node.cond)
         true
       end

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)
+      return unless location = node.name_location
-      issue_for location, name_end_location(node),
+      end_location = name_end_location(node)
-        MSG % {complexity, max_complexity}
+
+      issue_for location, end_location, MSG % {complexity, max_complexity}
     end
   end
 end

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)

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
+      value = slice_digits(value.reverse).reverse
-      fraction = "." + slice_digits(fraction) { |slice| slice } if fraction
+      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

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

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)
+    def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef)
-      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)
       check_node(source, node)
     end
   end

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)
+    def test(source, node : Crystal::Var | Crystal::InstanceVar | Crystal::ClassVar)
-      check_node source, node
-    end
-
-    def test(source, node : Crystal::InstanceVar)
-      check_node source, node
-    end
-
-    def test(source, node : Crystal::ClassVar)
       check_node source, node
     end
 

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