Merge pull request #434 from crystal-ameba/misc-refactors

v1.6.1

shard.yml

@@ -1,5 +1,5 @@
 name: ameba
-version: 1.6.0
+version: 1.6.1
 
 authors:
   - Vitalii Elenhaupt <velenhaupt@gmail.com>

spec/ameba/ast/scope_spec.cr

@@ -57,13 +57,15 @@ module Ameba::AST
           end
         end
         CRYSTAL
-      scope = Scope.new nodes.def_nodes.first
+
       var_node = nodes.var_nodes.first
-      scope.add_variable var_node
+
+      scope = Scope.new nodes.def_nodes.first
+      scope.add_variable(var_node)
       scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
 
       variable = Variable.new(var_node, scope)
-      variable.reference nodes.var_nodes.first, scope.inner_scopes.first
+      variable.reference(nodes.var_nodes.first, scope.inner_scopes.first)
 
       scope.references?(variable).should be_true
     end
@@ -77,13 +79,15 @@ module Ameba::AST
           end
         end
         CRYSTAL
-      scope = Scope.new nodes.def_nodes.first
+
       var_node = nodes.var_nodes.first
-      scope.add_variable var_node
+
+      scope = Scope.new nodes.def_nodes.first
+      scope.add_variable(var_node)
       scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
 
       variable = Variable.new(var_node, scope)
-      variable.reference nodes.var_nodes.first, scope.inner_scopes.first
+      variable.reference(nodes.var_nodes.first, scope.inner_scopes.first)
 
       scope.references?(variable, check_inner_scopes: false).should be_false
     end
@@ -98,9 +102,11 @@ module Ameba::AST
           end
         end
         CRYSTAL
-      scope = Scope.new nodes.def_nodes.first
+
       var_node = nodes.var_nodes.first
-      scope.add_variable var_node
+
+      scope = Scope.new nodes.def_nodes.first
+      scope.add_variable(var_node)
       scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
 
       variable = Variable.new(var_node, scope)
@@ -120,7 +126,7 @@ module Ameba::AST
   describe "#find_variable" do
     it "returns the variable in the scope by name" do
       scope = Scope.new as_node("foo = 1")
-      scope.add_variable Crystal::Var.new "foo"
+      scope.add_variable(Crystal::Var.new "foo")
       scope.find_variable("foo").should_not be_nil
     end
 
@@ -133,7 +139,7 @@ module Ameba::AST
   describe "#assign_variable" do
     it "creates a new assignment" do
       scope = Scope.new as_node("foo = 1")
-      scope.add_variable Crystal::Var.new "foo"
+      scope.add_variable(Crystal::Var.new "foo")
       scope.assign_variable("foo", Crystal::Var.new "foo")
       var = scope.find_variable("foo").should_not be_nil
       var.assignments.size.should eq 1
@@ -141,7 +147,7 @@ module Ameba::AST
 
     it "does not create the assignment if variable is wrong" do
       scope = Scope.new as_node("foo = 1")
-      scope.add_variable Crystal::Var.new "foo"
+      scope.add_variable(Crystal::Var.new "foo")
       scope.assign_variable("bar", Crystal::Var.new "bar")
       var = scope.find_variable("foo").should_not be_nil
       var.assignments.size.should eq 0

spec/ameba/ast/variabling/variable_spec.cr

@@ -85,13 +85,16 @@ module Ameba::AST
             3.times { |i| a = a + i }
           end
           CRYSTAL
-        scope = Scope.new nodes.def_nodes.first
+
         var_node = nodes.var_nodes.first
-        scope.add_variable var_node
+
+        scope = Scope.new(nodes.def_nodes.first)
+        scope.add_variable(var_node)
         scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
 
         variable = Variable.new(var_node, scope)
-        variable.reference nodes.var_nodes.last, scope.inner_scopes.last
+        variable.reference(nodes.var_nodes.last, scope.inner_scopes.last)
+
         variable.captured_by_block?.should be_truthy
       end
 
@@ -101,8 +104,10 @@ module Ameba::AST
             a = 1
           end
           CRYSTAL
-        scope.add_variable Crystal::Var.new "a"
+
+        scope.add_variable(Crystal::Var.new "a")
         variable = scope.variables.first
+
         variable.captured_by_block?.should be_falsey
       end
     end

spec/ameba/config_spec.cr

@@ -85,7 +85,7 @@ module Ameba
       end
 
       it "raises when custom config file doesn't exist" do
-        expect_raises(Exception, "Unable to load config file: Config file does not exist foo.yml") do
+        expect_raises(Exception, "Unable to load config file: Config file does not exist") do
           Config.load "foo.yml"
         end
       end

spec/ameba/rule/lint/literals_comparison_spec.cr

@@ -10,6 +10,8 @@ module Ameba::Rule::Lint
         ["foo"] === [foo]
         "foo" == foo
         "foo" != foo
+        "foo" == FOO
+        FOO == "foo"
         foo == "foo"
         foo != "foo"
         CRYSTAL

src/ameba/ast/scope.cr

@@ -34,9 +34,8 @@ module Ameba::AST
     # The actual AST node that represents a current scope.
     getter node : Crystal::ASTNode
 
-    delegate to_s, to: node
+    delegate location, end_location, to_s,
-    delegate location, to: node
+      to: @node
-    delegate end_location, to: node
 
     def_equals_and_hash node, location
 

src/ameba/ast/variabling/argument.cr

@@ -19,9 +19,8 @@ module Ameba::AST
     # Variable of this argument (may be the same node)
     getter variable : Variable
 
-    delegate location, to: @node
+    delegate location, end_location, to_s,
-    delegate end_location, to: @node
+      to: @node
-    delegate to_s, to: @node
 
     # Creates a new argument.
     #

src/ameba/ast/variabling/assignment.cr

@@ -19,9 +19,8 @@ module Ameba::AST
     # A scope assignment belongs to
     getter scope : Scope
 
-    delegate to_s, to: @node
+    delegate location, end_location, to_s,
-    delegate location, to: @node
+      to: @node
-    delegate end_location, to: @node
 
     # Creates a new assignment.
     #

src/ameba/ast/variabling/ivariable.cr

@@ -2,10 +2,8 @@ module Ameba::AST
   class InstanceVariable
     getter node : Crystal::InstanceVar
 
-    delegate location, to: @node
+    delegate location, end_location, name, to_s,
-    delegate end_location, to: @node
+      to: @node
-    delegate name, to: @node
-    delegate to_s, to: @node
 
     def initialize(@node)
     end

src/ameba/ast/variabling/type_dec_variable.cr

@@ -2,9 +2,8 @@ module Ameba::AST
   class TypeDecVariable
     getter node : Crystal::TypeDeclaration
 
-    delegate location, to: @node
+    delegate location, end_location, to_s,
-    delegate end_location, to: @node
+      to: @node
-    delegate to_s, to: @node
 
     def initialize(@node)
     end

src/ameba/ast/variabling/variable.cr

@@ -17,10 +17,8 @@ module Ameba::AST
     # Node of the first assignment which can be available before any reference.
     getter assign_before_reference : Crystal::ASTNode?
 
-    delegate location, to: @node
+    delegate location, end_location, name, to_s,
-    delegate end_location, to: @node
+      to: @node
-    delegate name, to: @node
-    delegate to_s, to: @node
 
     # Creates a new variable(in the scope).
     #
@@ -54,7 +52,7 @@ module Ameba::AST
     #
     # ```
     # variable = Variable.new(node, scope)
-    # variable.reference(var_node)
+    # variable.reference(var_node, some_scope)
     # variable.referenced? # => true
     # ```
     def referenced?
@@ -74,6 +72,11 @@ module Ameba::AST
       end
     end
 
+    # :ditto:
+    def reference(scope : Scope)
+      reference(node, scope)
+    end
+
     # Reference variable's assignments.
     #
     # ```
@@ -208,9 +211,9 @@ module Ameba::AST
       return if references.size > assignments.size
       return if assignments.any?(&.op_assign?)
 
-      @assign_before_reference = assignments.find { |ass|
+      @assign_before_reference = assignments
-        !ass.in_branch?
+        .find(&.in_branch?.!)
-      }.try &.node
+        .try(&.node)
     end
   end
 end

src/ameba/ast/visitors/scope_visitor.cr

@@ -153,7 +153,7 @@ module Ameba::AST
 
     # :nodoc:
     def visit(node : Crystal::Var)
-      variable = @current_scope.find_variable node.name
+      variable = @current_scope.find_variable(node.name)
 
       case
       when @current_scope.arg?(node) # node is an argument
@@ -161,7 +161,7 @@ module Ameba::AST
       when variable.nil? && @current_assign # node is a variable
         @current_scope.add_variable(node)
       when variable # node is a reference
-        reference = variable.reference node, @current_scope
+        reference = variable.reference(node, @current_scope)
         if @current_assign.is_a?(Crystal::OpAssign) || !reference.target_of?(@current_assign)
           variable.reference_assignments!
         end
@@ -178,9 +178,7 @@ module Ameba::AST
       when scope.type_definition? && accessor_macro?(node) then return false
       when scope.def? && special_node?(node)
         scope.arguments.each do |arg|
-          variable = arg.variable
+          ref = arg.variable.reference(scope)
-
-          ref = variable.reference(variable.node, scope)
           ref.explicit = false
         end
       end

src/ameba/config.cr

@@ -97,8 +97,9 @@ class Ameba::Config
     @excluded = load_array_section(config, "Excluded")
     @globs = load_array_section(config, "Globs", DEFAULT_GLOBS)
 
-    return unless formatter_name = load_formatter_name(config)
+    if formatter_name = load_formatter_name(config)
-    self.formatter = formatter_name
+      self.formatter = formatter_name
+    end
   end
 
   # Loads YAML configuration file by `path`.
@@ -120,8 +121,8 @@ class Ameba::Config
 
   protected def self.read_config(path = nil)
     if path
-      raise ArgumentError.new("Config file does not exist #{path}") unless File.exists?(path)
+      return File.read(path) if File.exists?(path)
-      return File.read(path)
+      raise "Config file does not exist"
     end
     each_config_path do |config_path|
       return File.read(config_path) if File.exists?(config_path)

src/ameba/formatter/base_formatter.cr

@@ -17,13 +17,13 @@ module Ameba::Formatter
     # A list of sources to inspect is passed as an argument.
     def started(sources); end
 
-    # Callback that indicates when source inspection is finished.
+    # Callback that indicates when source inspection is started.
     # A corresponding source is passed as an argument.
-    def source_finished(source : Source); end
+    def source_started(source : Source); end
 
     # Callback that indicates when source inspection is finished.
     # A corresponding source is passed as an argument.
-    def source_started(source : Source); end
+    def source_finished(source : Source); end
 
     # Callback that indicates when inspection is finished.
     # A list of inspected sources is passed as an argument.

src/ameba/formatter/todo_formatter.cr

@@ -26,16 +26,21 @@ module Ameba::Formatter
     end
 
     private def generate_todo_config(issues)
-      file = File.new(@config_path, mode: "w")
+      File.open(@config_path, mode: "w") do |file|
-      file << header
+        file << header
-      rule_issues_map(issues).each do |rule, rule_issues|
+
-        file << "\n# Problems found: #{rule_issues.size}"
+        rule_issues_map(issues).each do |rule, rule_issues|
-        file << "\n# Run `ameba --only #{rule.name}` for details"
+          rule_todo = rule_todo(rule, rule_issues)
-        file << rule_todo(rule, rule_issues).gsub("---", "")
+          rule_todo =
+            {rule_todo.name => rule_todo}
+              .to_yaml.gsub("---", "")
+
+          file << "\n# Problems found: #{rule_issues.size}"
+          file << "\n# Run `ameba --only #{rule.name}` for details"
+          file << rule_todo
+        end
+        file
       end
-      file
-    ensure
-      file.close if file
     end
 
     private def rule_issues_map(issues)
@@ -60,11 +65,11 @@ module Ameba::Formatter
     end
 
     private def rule_todo(rule, issues)
-      rule.excluded = issues
+      rule.dup.tap do |rule_todo|
-        .compact_map(&.location.try &.filename.try &.to_s)
+        rule_todo.excluded = issues
-        .uniq!
+          .compact_map(&.location.try &.filename.try &.to_s)
-
+          .uniq!
-      {rule.name => rule}.to_yaml
+      end
     end
   end
 end

src/ameba/rule/base.cr

@@ -32,14 +32,15 @@ module Ameba::Rule
     # This method is designed to test the source passed in. If source has issues
     # that are tested by this rule, it should add an issue.
     #
-    # Be default it uses a node visitor to traverse all the nodes in the source.
+    # By default it uses a node visitor to traverse all the nodes in the source.
+    #
     # NOTE: Must be overridden for other type of rules.
     def test(source : Source)
       AST::NodeVisitor.new self, source
     end
 
+    # NOTE: Can't be abstract
     def test(source : Source, node : Crystal::ASTNode, *opts)
-      # can't be abstract
     end
 
     # A convenient addition to `#test` method that does the same

src/ameba/rule/naming/block_parameter_name.cr

@@ -41,7 +41,7 @@ module Ameba::Rule::Naming
     end
 
     private def valid_name?(name)
-      return true if name.blank? # happens with compound names like `(arg1, arg2)`
+      return true if name.blank? # TODO: handle unpacked variables
       return true if name.in?(allowed_names)
 
       return false if name.in?(forbidden_names)

src/ameba/rule/style/verbose_block.cr

@@ -110,23 +110,22 @@ module Ameba::Rule::Style
       i
     end
 
-    protected def args_to_s(io : IO, node : Crystal::Call, short_block = nil, skip_last_arg = false)
+    protected def args_to_s(io : IO, node : Crystal::Call, short_block = nil, skip_last_arg = false) : Nil
-      node.args.dup.tap do |args|
+      args = node.args.dup
-        args.pop? if skip_last_arg
+      args.pop? if skip_last_arg
-        args.join io, ", "
+      args.join io, ", "
 
-        named_args = node.named_args
+      named_args = node.named_args
-        if named_args
+      if named_args
-          io << ", " unless args.empty? || named_args.empty?
+        io << ", " unless args.empty? || named_args.empty?
-          named_args.join io, ", " do |arg, inner_io|
+        named_args.join io, ", " do |arg, inner_io|
-            inner_io << arg.name << ": " << arg.value
+          inner_io << arg.name << ": " << arg.value
-          end
         end
+      end
 
-        if short_block
+      if short_block
-          io << ", " unless args.empty? && (named_args.nil? || named_args.empty?)
+        io << ", " unless args.empty? && (named_args.nil? || named_args.empty?)
-          io << short_block
+        io << short_block
-        end
       end
     end
 
@@ -164,9 +163,7 @@ module Ameba::Rule::Style
       return unless block_end_location = block.body.end_location
 
       block_code = source_between(block_location, block_end_location, source.lines)
-      return unless block_code.try(&.starts_with?("&."))
+      block_code if block_code.try(&.starts_with?("&."))
-
-      block_code
     end
 
     protected def call_code(source, call, body)
@@ -230,7 +227,7 @@ module Ameba::Rule::Style
       # we filter out the blocks that are of call type - `i.to_i64.odd?`
       return unless (body = block.body).is_a?(Crystal::Call)
 
-      # we need to "unwind" the chain calls, so the final receiver object
+      # we need to "unwind" the call chain, so the final receiver object
       # ends up being a variable - `i`
       obj = body.obj
       while obj.is_a?(Crystal::Call)