Merge pull request #419 from crystal-ameba/add-block-parameter-name-rule

Add `Naming/BlockParameterName` rule

bench/check_sources.cr

@@ -15,7 +15,7 @@ Benchmark.ips do |x|
     20,
     30,
     40,
-  ].each do |n|
+  ].each do |n| # ameba:disable Naming/BlockParameterName
     config = Ameba::Config.load
     config.formatter = Ameba::Formatter::BaseFormatter.new
     config.globs = get_files(n)

spec/ameba/cli/cmd_spec.cr

@@ -11,23 +11,23 @@ module Ameba::Cli
     end
 
     describe ".parse_args" do
-      %w(-s --silent).each do |f|
-        it "accepts #{f} flag" do
-          c = Cli.parse_args [f]
+      %w(-s --silent).each do |flag|
+        it "accepts #{flag} flag" do
+          c = Cli.parse_args [flag]
           c.formatter.should eq :silent
         end
       end
 
-      %w(-c --config).each do |f|
-        it "accepts #{f} flag" do
-          c = Cli.parse_args [f, "config.yml"]
+      %w(-c --config).each do |flag|
+        it "accepts #{flag} flag" do
+          c = Cli.parse_args [flag, "config.yml"]
           c.config.should eq Path["config.yml"]
         end
       end
 
-      %w(-f --format).each do |f|
-        it "accepts #{f} flag" do
-          c = Cli.parse_args [f, "my-formatter"]
+      %w(-f --format).each do |flag|
+        it "accepts #{flag} flag" do
+          c = Cli.parse_args [flag, "my-formatter"]
           c.formatter.should eq "my-formatter"
         end
       end

spec/ameba/rule/naming/block_parameter_name_spec.cr

@@ -0,0 +1,100 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Naming
+  subject = BlockParameterName.new
+    .tap(&.min_name_length = 3)
+    .tap(&.allowed_names = %w[_ e i j k v])
+
+  describe BlockParameterName do
+    it "passes if block parameter name matches #allowed_names" do
+      subject.allowed_names.each do |name|
+        expect_no_issues subject, <<-CRYSTAL
+          %w[].each { |#{name}| }
+          CRYSTAL
+      end
+    end
+
+    it "fails if block parameter name doesn't match #allowed_names" do
+      expect_issue subject, <<-CRYSTAL
+        %w[].each { |x| }
+                   # ^ error: Disallowed block parameter name found
+        CRYSTAL
+    end
+
+    context "properties" do
+      context "#min_name_length" do
+        it "allows setting custom values" do
+          rule = BlockParameterName.new
+          rule.allowed_names = %w[a b c]
+
+          rule.min_name_length = 3
+          expect_issue rule, <<-CRYSTAL
+            %w[].each { |x| }
+                       # ^ error: Disallowed block parameter name found
+            CRYSTAL
+
+          rule.min_name_length = 1
+          expect_no_issues rule, <<-CRYSTAL
+            %w[].each { |x| }
+            CRYSTAL
+        end
+      end
+
+      context "#allow_names_ending_in_numbers" do
+        it "allows setting custom values" do
+          rule = BlockParameterName.new
+          rule.min_name_length = 1
+          rule.allowed_names = %w[]
+
+          rule.allow_names_ending_in_numbers = false
+          expect_issue rule, <<-CRYSTAL
+            %w[].each { |x1| }
+                       # ^ error: Disallowed block parameter name found
+            CRYSTAL
+
+          rule.allow_names_ending_in_numbers = true
+          expect_no_issues rule, <<-CRYSTAL
+            %w[].each { |x1| }
+            CRYSTAL
+        end
+      end
+
+      context "#allowed_names" do
+        it "allows setting custom names" do
+          rule = BlockParameterName.new
+          rule.min_name_length = 3
+
+          rule.allowed_names = %w[a b c]
+          expect_issue rule, <<-CRYSTAL
+            %w[].each { |x| }
+                       # ^ error: Disallowed block parameter name found
+            CRYSTAL
+
+          rule.allowed_names = %w[x y z]
+          expect_no_issues rule, <<-CRYSTAL
+            %w[].each { |x| }
+            CRYSTAL
+        end
+      end
+
+      context "#forbidden_names" do
+        it "allows setting custom names" do
+          rule = BlockParameterName.new
+          rule.min_name_length = 1
+          rule.allowed_names = %w[]
+
+          rule.forbidden_names = %w[x y z]
+          expect_issue rule, <<-CRYSTAL
+            %w[].each { |x| }
+                       # ^ error: Disallowed block parameter name found
+            CRYSTAL
+
+          rule.forbidden_names = %w[a b c]
+          expect_no_issues rule, <<-CRYSTAL
+            %w[].each { |x| }
+            CRYSTAL
+        end
+      end
+    end
+  end
+end

src/ameba/ast/util.cr

@@ -21,8 +21,8 @@ module Ameba::AST::Util
         static_literal?(node.to)}
     when Crystal::ArrayLiteral,
          Crystal::TupleLiteral
-      {true, node.elements.all? do |el|
-        static_literal?(el)
+      {true, node.elements.all? do |element|
+        static_literal?(element)
       end}
     when Crystal::HashLiteral
       {true, node.entries.all? do |entry|

src/ameba/ast/variabling/variable.cr

@@ -136,7 +136,7 @@ module Ameba::AST
       case assign
       when Crystal::Assign           then eql?(assign.target)
       when Crystal::OpAssign         then eql?(assign.target)
-      when Crystal::MultiAssign      then assign.targets.any? { |t| eql?(t) }
+      when Crystal::MultiAssign      then assign.targets.any? { |target| eql?(target) }
       when Crystal::UninitializedVar then eql?(assign.var)
       else
         false

src/ameba/ast/visitors/redundant_control_expression_visitor.cr

@@ -43,7 +43,7 @@ module Ameba::AST
     end
 
     private def traverse_case(node)
-      node.whens.each { |n| traverse_node n.body }
+      node.whens.each { |when_node| traverse_node when_node.body }
       traverse_node(node.else)
     end
 
@@ -54,7 +54,7 @@ module Ameba::AST
     private def traverse_exception_handler(node)
       traverse_node node.body
       traverse_node node.else
-      node.rescues.try &.each { |n| traverse_node n.body }
+      node.rescues.try &.each { |rescue_node| traverse_node rescue_node.body }
     end
   end
 end

src/ameba/cli/cmd.cr

@@ -75,11 +75,11 @@ module Ameba::Cli
       parser.on("-h", "--help", "Show this help") { print_help(parser) }
       parser.on("-r", "--rules", "Show all available rules") { opts.rules = true }
       parser.on("-s", "--silent", "Disable output") { opts.formatter = :silent }
-      parser.unknown_args do |f|
-        if f.size == 1 && f.first =~ /.+:\d+:\d+/
-          configure_explain_opts(f.first, opts)
+      parser.unknown_args do |arr|
+        if arr.size == 1 && arr.first.matches?(/.+:\d+:\d+/)
+          configure_explain_opts(arr.first, opts)
         else
-          opts.globs = f unless f.empty?
+          opts.globs = arr unless arr.empty?
         end
       end
 

src/ameba/formatter/todo_formatter.cr

@@ -39,12 +39,12 @@ module Ameba::Formatter
     end
 
     private def rule_issues_map(issues)
-      Hash(Rule::Base, Array(Issue)).new.tap do |h|
+      Hash(Rule::Base, Array(Issue)).new.tap do |hash|
         issues.each do |issue|
           next if issue.disabled? || issue.rule.is_a?(Rule::Lint::Syntax)
           next if issue.correctable? && config[:autocorrect]?
 
-          (h[issue.rule] ||= Array(Issue).new) << issue
+          (hash[issue.rule] ||= Array(Issue).new) << issue
         end
       end
     end

src/ameba/rule/lint/redundant_string_coercion.cr

@@ -30,8 +30,8 @@ module Ameba::Rule::Lint
     MSG = "Redundant use of `Object#to_s` in interpolation"
 
     def test(source, node : Crystal::StringInterpolation)
-      string_coercion_nodes(node).each do |n|
-        issue_for n.name_location, n.end_location, MSG
+      string_coercion_nodes(node).each do |expr|
+        issue_for expr.name_location, expr.end_location, MSG
       end
     end
 

src/ameba/rule/naming/block_parameter_name.cr

@@ -0,0 +1,54 @@
+module Ameba::Rule::Naming
+  # A rule that reports non-descriptive block parameter names.
+  #
+  # Favour this:
+  #
+  # ```
+  # tokens.each { |token| token.last_accessed_at = Time.utc }
+  # ```
+  #
+  # Over this:
+  #
+  # ```
+  # tokens.each { |t| t.last_accessed_at = Time.utc }
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Naming/BlockParameterName:
+  #   Enabled: true
+  #   MinNameLength: 3
+  #   AllowNamesEndingInNumbers: true
+  #   AllowedNames: [_, e, i, j, k, v, x, y, ex, io, ws, tx, id, k1, k2, v1, v2]
+  #   ForbiddenNames: []
+  # ```
+  class BlockParameterName < Base
+    properties do
+      description "Disallows non-descriptive block parameter names"
+      min_name_length 3
+      allow_names_ending_in_numbers true
+      allowed_names %w[_ e i j k v x y ex io ws tx id k1 k2 v1 v2]
+      forbidden_names %w[]
+    end
+
+    MSG = "Disallowed block parameter name found"
+
+    def test(source, node : Crystal::Call)
+      node.try(&.block).try(&.args).try &.each do |arg|
+        issue_for arg, MSG unless valid_name?(arg.name)
+      end
+    end
+
+    private def valid_name?(name)
+      return true if name.blank? # happens with compound names like `(arg1, arg2)`
+      return true if name.in?(allowed_names)
+
+      return false if name.in?(forbidden_names)
+      return false if name.size < min_name_length
+      return false if name[-1].ascii_number? && !allow_names_ending_in_numbers?
+
+      true
+    end
+  end
+end

src/ameba/rule/naming/rescued_exceptions_variable_name.cr

@@ -34,13 +34,13 @@ module Ameba::Rule::Naming
     MSG_SINGULAR = "Disallowed variable name, use '%s' instead"
 
     def test(source, node : Crystal::ExceptionHandler)
-      node.rescues.try &.each do |r|
-        next if valid_name?(r.name)
+      node.rescues.try &.each do |rescue_node|
+        next if valid_name?(rescue_node.name)
 
         message =
           allowed_names.size == 1 ? MSG_SINGULAR : MSG
 
-        issue_for r, message % allowed_names.join("', '")
+        issue_for rescue_node, message % allowed_names.join("', '")
       end
     end