Merge pull request #309 from crystal-ameba/Sija/refactor-base-rule

spec/ameba/base_spec.cr

@@ -25,13 +25,17 @@ module Ameba::Rule
       subject = DummyRule.new
 
       it "is enabled by default" do
-        subject.enabled.should be_true
+        subject.enabled?.should be_true
       end
 
       it "has a description property" do
         subject.description.should_not be_nil
       end
 
+      it "has a dummy? property" do
+        subject.dummy?.should be_true
+      end
+
       it "has excluded property" do
         subject.excluded.should be_nil
       end

spec/ameba/config_spec.cr

@@ -132,7 +132,7 @@ module Ameba
         config.sources.size.should eq(1)
       end
 
-      it "returns a lisf of sources excluding 'Excluded'" do
+      it "returns a list of sources excluding 'Excluded'" do
         config.excluded = %w(**/config_spec.cr)
         config.sources.any?(&.fullpath.==(__FILE__)).should be_false
       end
@@ -170,7 +170,7 @@ module Ameba
         name = DummyRule.rule_name
         config.update_rule name, enabled: false
         rule = config.rules.find!(&.name.== name)
-        rule.enabled.should be_false
+        rule.enabled?.should be_false
       end
 
       it "updates excluded property" do
@@ -189,7 +189,7 @@ module Ameba
         name = DummyRule.rule_name
         config.update_rules [name], enabled: false
         rule = config.rules.find!(&.name.== name)
-        rule.enabled.should be_false
+        rule.enabled?.should be_false
       end
 
       it "updates multiple rules by excluded property" do
@@ -204,7 +204,7 @@ module Ameba
         group = DummyRule.group_name
         config.update_rules [group], enabled: false
         rule = config.rules.find!(&.name.== DummyRule.rule_name)
-        rule.enabled.should be_false
+        rule.enabled?.should be_false
       end
 
       it "updates a group by excluded property" do

spec/ameba/formatter/todo_formatter_spec.cr

@@ -2,43 +2,52 @@ require "../../spec_helper"
 require "file_utils"
 
 module Ameba
+  private def with_formatter
+    io = IO::Memory.new
+    formatter = Formatter::TODOFormatter.new(io)
+
+    yield formatter, io
+  end
+
   private def create_todo
-    formatter = Formatter::TODOFormatter.new IO::Memory.new
+    with_formatter do |formatter|
       s = Source.new "a = 1", "source.cr"
       s.add_issue DummyRule.new, {1, 2}, "message"
       file = formatter.finished([s])
       file ? File.read(file.path) : ""
     end
+  end
 
   describe Formatter::TODOFormatter do
     ::Spec.after_each do
-      FileUtils.rm(Ameba::Config::PATH) if File.exists?(Ameba::Config::PATH)
+      FileUtils.rm_rf(Ameba::Config::PATH)
     end
 
     context "problems not found" do
       it "does not create file" do
-        formatter = Formatter::TODOFormatter.new IO::Memory.new
+        with_formatter do |formatter|
           file = formatter.finished [Source.new ""]
           file.should be_nil
         end
+      end
 
       it "reports a message saying file is not created" do
-        io = IO::Memory.new
+        with_formatter do |formatter, io|
-        formatter = Formatter::TODOFormatter.new io
           formatter.finished [Source.new ""]
           io.to_s.should contain "No issues found. File is not generated"
         end
       end
+    end
 
     context "problems found" do
       it "prints a message saying file is created" do
-        io = IO::Memory.new
+        with_formatter do |formatter, io|
-        formatter = Formatter::TODOFormatter.new io
           s = Source.new "a = 1", "source.cr"
           s.add_issue DummyRule.new, {1, 2}, "message"
           formatter.finished([s])
           io.to_s.should contain "Created .ameba.yml"
         end
+      end
 
       it "creates a valid YAML document" do
         YAML.parse(create_todo).should_not be_nil
@@ -77,8 +86,8 @@ module Ameba
       end
 
       context "with multiple issues" do
-        formatter = Formatter::TODOFormatter.new IO::Memory.new
+        it "does generate todo file" do
-
+          with_formatter do |formatter|
             s1 = Source.new "a = 1", "source1.cr"
             s2 = Source.new "a = 1", "source2.cr"
             s1.add_issue DummyRule.new, {1, 2}, "message1"
@@ -93,6 +102,7 @@ module Ameba
               # Run `ameba --only Ameba/DummyRule` for details
               Ameba/DummyRule:
                 Description: Dummy rule that does nothing.
+                Dummy: true
                 Excluded:
                 - source1.cr
                 - source2.cr
@@ -100,20 +110,22 @@ module Ameba
                 Severity: Convention
               CONTENT
           end
+        end
+      end
 
       context "when invalid syntax" do
         it "does generate todo file" do
-          formatter = Formatter::TODOFormatter.new IO::Memory.new
+          with_formatter do |formatter|
             s = Source.new "def invalid_syntax"
             s.add_issue Rule::Lint::Syntax.new, {1, 2}, "message"
 
             file = formatter.finished [s]
             file.should be_nil
           end
+        end
 
         it "prints an error message" do
-          io = IO::Memory.new
+          with_formatter do |formatter, io|
-          formatter = Formatter::TODOFormatter.new io
             s = Source.new "def invalid_syntax"
             s.add_issue Rule::Lint::Syntax.new, {1, 2}, "message"
 
@@ -124,4 +136,5 @@ module Ameba
         end
       end
     end
+  end
 end

spec/ameba/rule/lint/syntax_spec.cr

@@ -8,7 +8,7 @@ module Ameba::Rule::Lint
       expect_no_issues subject, <<-CRYSTAL
         def hello
           puts "totally valid"
-        rescue e: Exception
+        rescue e : Exception
         end
         CRYSTAL
     end

spec/spec_helper.cr

@@ -7,6 +7,7 @@ module Ameba
   class DummyRule < Rule::Base
     properties do
       description : String = "Dummy rule that does nothing."
+      dummy true
     end
 
     def test(source)
@@ -74,14 +75,14 @@ module Ameba
 
   # A rule that always raises an error
   class RaiseRule < Rule::Base
-    property should_raise = false
+    property? should_raise = false
 
     properties do
       description "Internal rule that always raises"
     end
 
     def test(source)
-      should_raise && raise "something went wrong"
+      should_raise? && raise "something went wrong"
     end
   end
 
@@ -240,7 +241,7 @@ module Ameba
   end
 
   class TestNodeVisitor < Crystal::Visitor
-    NODES = [
+    NODES = {
       Crystal::NilLiteral,
       Crystal::Var,
       Crystal::Assign,
@@ -254,7 +255,7 @@ module Ameba
       Crystal::MacroLiteral,
       Crystal::Expressions,
       Crystal::ControlExpression,
-    ]
+    }
 
     def initialize(node)
       node.accept self

src/ameba/ast/branch.cr

@@ -11,7 +11,6 @@ module Ameba::AST
   #   do_something a # --> Branch D
   # end
   # ```
-  #
   class Branch
     # The actual branch node.
     getter node : Crystal::ASTNode

src/ameba/ast/branchable.cr

@@ -6,7 +6,7 @@ module Ameba::AST
   # are branchables.
   #
   # ```
-  # white a > 100 # Branchable A
+  # while a > 100 # Branchable A
   #   if b > 2    # Branchable B
   #     a += 1
   #   end
@@ -37,7 +37,7 @@ module Ameba::AST
 
     # Returns true if this node or one of the parent branchables is a loop, false otherwise.
     def loop?
-      loop?(node) || parent.try(&.loop?) || false
+      loop?(node) || !!parent.try(&.loop?)
     end
   end
 end

src/ameba/ast/variabling/assignment.cr

@@ -30,6 +30,7 @@ module Ameba::AST
     # ```
     def initialize(@node, @variable, @scope)
       return unless scope = @variable.scope
+
       @branch = Branch.of(@node, scope)
       @referenced = true if @variable.special? ||
                             @variable.scope.type_definition? ||
@@ -37,7 +38,7 @@ module Ameba::AST
     end
 
     def referenced_in_loop?
-      @variable.referenced? && @branch.try &.in_loop?
+      @variable.referenced? && !!@branch.try(&.in_loop?)
     end
 
     # Returns true if this assignment is an op assign, false if not.

src/ameba/ast/variabling/variable.cr

@@ -161,7 +161,7 @@ module Ameba::AST
     def declared_before?(node)
       var_location, node_location = location, node.location
 
-      return if var_location.nil? || node_location.nil?
+      return unless var_location && node_location
 
       (var_location.line_number < node_location.line_number) ||
         (var_location.line_number == node_location.line_number &&

src/ameba/ast/visitors/counting_visitor.cr

@@ -3,7 +3,7 @@ module Ameba::AST
   class CountingVisitor < Crystal::Visitor
     DEFAULT_COMPLEXITY = 1
 
-    getter macro_condition = false
+    getter? macro_condition = false
 
     # Creates a new counting visitor
     def initialize(@scope : Crystal::ASTNode)
@@ -27,13 +27,13 @@ module Ameba::AST
     {% for node in %i(if while until rescue or and) %}
       # :nodoc:
       def visit(node : Crystal::{{ node.id.capitalize }})
-        @complexity += 1 unless macro_condition
+        @complexity += 1 unless macro_condition?
       end
     {% end %}
 
     # :nodoc:
     def visit(node : Crystal::Case)
-      return true if macro_condition
+      return true if macro_condition?
 
       # Count the complexity of an exhaustive `Case` as 1
       # Otherwise count the number of `When`s

src/ameba/ast/visitors/node_visitor.cr

@@ -2,7 +2,7 @@ require "./base_visitor"
 
 module Ameba::AST
   # List of nodes to be visited by Ameba's rules.
-  NODES = [
+  NODES = {
     Alias,
     IsA,
     Assign,
@@ -27,7 +27,7 @@ module Ameba::AST
     When,
     While,
     Until,
-  ]
+  }
 
   # An AST Visitor that traverses the source and allows all nodes
   # to be inspected by rules.
@@ -35,7 +35,6 @@ module Ameba::AST
   # ```
   # visitor = Ameba::AST::NodeVisitor.new(rule, source)
   # ```
-  #
   class NodeVisitor < BaseVisitor
     @skip : Array(Crystal::ASTNode.class)?
 

src/ameba/ast/visitors/scope_visitor.cr

@@ -4,7 +4,7 @@ module Ameba::AST
   # AST Visitor that traverses the source and constructs scopes.
   class ScopeVisitor < BaseVisitor
     # Non-exhaustive list of nodes to be visited by Ameba's rules.
-    NODES = [
+    NODES = {
       ClassDef,
       ModuleDef,
       EnumDef,
@@ -17,7 +17,7 @@ module Ameba::AST
       Block,
       Macro,
       MacroFor,
-    ]
+    }
 
     SUPER_NODE_NAME  = "super"
     RECORD_NODE_NAME = "record"

src/ameba/cli/cmd.cr

@@ -184,10 +184,11 @@ module Ameba::Cli
 
   private def print_rules(config)
     config.rules.each do |rule|
-      puts \
+      puts "%s [%s] - %s" % {
-        "#{rule.name.colorize(:white)} " \
+        rule.name.colorize(:white),
-        "[#{rule.severity.symbol.to_s.colorize(:green)}] - " \
+        rule.severity.symbol.to_s.colorize(:green),
-        "#{rule.description.colorize(:dark_gray)}"
+        rule.description.colorize(:dark_gray),
+      }
     end
     exit 0
   end

src/ameba/config.cr

@@ -242,12 +242,16 @@ class Ameba::Config
         {% properties[name] = {key: key, default: value, type: type, converter: converter} %}
 
         @[YAML::Field(key: {{ key }}, converter: {{ converter }}, type: {{ type }})]
+        {% if type == Bool %}
+          property? {{ name }} : {{ type }} = {{ value }}
+        {% else %}
           property {{ name }} : {{ type }} = {{ value }}
         {% end %}
+      {% end %}
 
       {% if properties["enabled".id] == nil %}
         @[YAML::Field(key: "Enabled")]
-        property enabled = true
+        property? enabled = true
       {% end %}
 
       {% if properties["severity".id] == nil %}

src/ameba/formatter/dot_formatter.cr

@@ -45,10 +45,11 @@ module Ameba::Formatter
             end
           end
           output.puts
-          output.puts \
+          output.puts ("[%s] %s: %s" % {
-            "[#{issue.rule.severity.symbol}] " \
+            issue.rule.severity.symbol,
-            "#{issue.rule.name}: " \
+            issue.rule.name,
-            "#{issue.message}".colorize(:red)
+            issue.message,
+          }).colorize(:red)
 
           if show_affected_code && (code = affected_code(issue))
             output << code.colorize(:default)

src/ameba/rule/base.cr

@@ -1,10 +1,10 @@
 module Ameba::Rule
   # List of names of the special rules, which
   # behave differently than usual rules.
-  SPECIAL = [
+  SPECIAL = {
     Lint::Syntax.rule_name,
     Lint::UnneededDisableDirective.rule_name,
-  ]
+  }
 
   # Represents a base of all rules. In other words, all rules
   # inherits from this struct:
@@ -88,7 +88,7 @@ module Ameba::Rule
     # my_rule.excluded?(source) # => true or false
     # ```
     def excluded?(source)
-      excluded.try &.any? do |path|
+      !!excluded.try &.any? do |path|
         source.matches_path?(path) ||
           Dir.glob(path).any? { |glob| source.matches_path?(glob) }
       end

src/ameba/rule/lint/unused_argument.cr

@@ -41,15 +41,15 @@ module Ameba::Rule::Lint
     end
 
     def test(source, node : Crystal::ProcLiteral, scope : AST::Scope)
-      ignore_procs || find_unused_arguments source, scope
+      ignore_procs? || find_unused_arguments source, scope
     end
 
     def test(source, node : Crystal::Block, scope : AST::Scope)
-      ignore_blocks || find_unused_arguments source, scope
+      ignore_blocks? || find_unused_arguments source, scope
     end
 
     def test(source, node : Crystal::Def, scope : AST::Scope)
-      ignore_defs || find_unused_arguments source, scope
+      ignore_defs? || find_unused_arguments source, scope
     end
 
     private def find_unused_arguments(source, scope)

src/ameba/rule/style/parentheses_around_condition.cr

@@ -46,7 +46,7 @@ module Ameba::Rule::Style
       when Crystal::Yield
         !in_ternary || node.has_parentheses? || node.exps.empty?
       when Crystal::Assign, Crystal::OpAssign, Crystal::MultiAssign
-        !in_ternary && !allow_safe_assignment
+        !in_ternary && !allow_safe_assignment?
       else
         true
       end
@@ -55,7 +55,7 @@ module Ameba::Rule::Style
     def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case | Crystal::While | Crystal::Until)
       cond = node.cond
 
-      if cond.is_a?(Crystal::Assign) && allow_safe_assignment
+      if cond.is_a?(Crystal::Assign) && allow_safe_assignment?
         issue_for cond, MSG_MISSING do |corrector|
           corrector.insert_before(cond, '(')
           corrector.insert_after(cond, ')')
@@ -65,7 +65,7 @@ module Ameba::Rule::Style
 
       is_ternary = node.is_a?(Crystal::If) && node.ternary?
 
-      return if is_ternary && exclude_ternary
+      return if is_ternary && exclude_ternary?
 
       return unless cond.is_a?(Crystal::Expressions)
       return unless cond.keyword.paren?

src/ameba/rule/style/redundant_next.cr

@@ -113,8 +113,8 @@ module Ameba::Rule::Style
     end
 
     def test(source, node : Crystal::Next, visitor : AST::RedundantControlExpressionVisitor)
-      return if allow_multi_next && node.exp.is_a?(Crystal::TupleLiteral)
+      return if allow_multi_next? && node.exp.is_a?(Crystal::TupleLiteral)
-      return if allow_empty_next && (node.exp.nil? || node.exp.try(&.nop?))
+      return if allow_empty_next? && (node.exp.nil? || node.exp.try(&.nop?))
 
       if exp_code = control_exp_code(node, source.lines)
         issue_for node, MSG do |corrector|

src/ameba/rule/style/redundant_return.cr

@@ -110,8 +110,8 @@ module Ameba::Rule::Style
     end
 
     def test(source, node : Crystal::Return, visitor : AST::RedundantControlExpressionVisitor)
-      return if allow_multi_return && node.exp.is_a?(Crystal::TupleLiteral)
+      return if allow_multi_return? && node.exp.is_a?(Crystal::TupleLiteral)
-      return if allow_empty_return && (node.exp.nil? || node.exp.try(&.nop?))
+      return if allow_empty_return? && (node.exp.nil? || node.exp.try(&.nop?))
 
       if exp_code = control_exp_code(node, source.lines)
         issue_for node, MSG do |corrector|

src/ameba/rule/style/verbose_block.cr

@@ -190,11 +190,12 @@ module Ameba::Rule::Style
     protected def issue_for_valid(source, call : Crystal::Call, block : Crystal::Block, body : Crystal::Call)
       return unless location = call.name_location
       return unless end_location = block.end_location
-      return if exclude_calls_with_block && body.block
+
-      return if exclude_multiple_line_blocks && !same_location_lines?(call, body)
+      return if exclude_calls_with_block? && body.block
-      return if exclude_prefix_operators && prefix_operator?(body)
+      return if exclude_multiple_line_blocks? && !same_location_lines?(call, body)
-      return if exclude_operators && operator?(body.name)
+      return if exclude_prefix_operators? && prefix_operator?(body)
-      return if exclude_setters && setter?(body.name)
+      return if exclude_operators? && operator?(body.name)
+      return if exclude_setters? && setter?(body.name)
 
       call_code =
         call_code(source, call, body)

src/ameba/runner.cr

@@ -62,7 +62,7 @@ module Ameba
       @sources = config.sources
       @formatter = config.formatter
       @severity = config.severity
-      @rules = config.rules.select(&.enabled).reject!(&.special?)
+      @rules = config.rules.select(&.enabled?).reject!(&.special?)
       @autocorrect = config.autocorrect?
 
       @unneeded_disable_directive_rule =
@@ -220,7 +220,7 @@ module Ameba
 
     private def check_unneeded_directives(source)
       return unless rule = @unneeded_disable_directive_rule
-      return unless rule.enabled
+      return unless rule.enabled?
 
       rule.test(source)
     end

src/ameba/tokenizer.cr

@@ -10,7 +10,6 @@ module Ameba
   #   puts token
   # end
   # ```
-  #
   class Tokenizer
     # Instantiates Tokenizer using a `source`.
     #