diff --git a/bench/check_sources.cr b/bench/check_sources.cr index d66f9603d..f5fbc2e3a 100644 --- a/bench/check_sources.cr +++ b/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) diff --git a/spec/ameba/cli/cmd_spec.cr b/spec/ameba/cli/cmd_spec.cr index 867b6d455..c98d8f711 100644 --- a/spec/ameba/cli/cmd_spec.cr +++ b/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 diff --git a/spec/ameba/rule/naming/block_parameter_name_spec.cr b/spec/ameba/rule/naming/block_parameter_name_spec.cr new file mode 100644 index 000000000..992f7e248 --- /dev/null +++ b/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 diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 11dfa6d8f..ce71fc9ca 100644 --- a/src/ameba/ast/util.cr +++ b/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| diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index a0b66e045..c2eee962b 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/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 diff --git a/src/ameba/ast/visitors/redundant_control_expression_visitor.cr b/src/ameba/ast/visitors/redundant_control_expression_visitor.cr index 100f7f21d..43f11fa3c 100644 --- a/src/ameba/ast/visitors/redundant_control_expression_visitor.cr +++ b/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 diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index e9a0ffbdd..5cb9bc0b8 100644 --- a/src/ameba/cli/cmd.cr +++ b/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 diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index 3e6efee37..5dc797e86 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/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 diff --git a/src/ameba/rule/lint/redundant_string_coercion.cr b/src/ameba/rule/lint/redundant_string_coercion.cr index 0279bba94..befb3a9b8 100644 --- a/src/ameba/rule/lint/redundant_string_coercion.cr +++ b/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 diff --git a/src/ameba/rule/naming/block_parameter_name.cr b/src/ameba/rule/naming/block_parameter_name.cr new file mode 100644 index 000000000..c4bf358ab --- /dev/null +++ b/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 diff --git a/src/ameba/rule/naming/rescued_exceptions_variable_name.cr b/src/ameba/rule/naming/rescued_exceptions_variable_name.cr index 893fe49e2..8dcfd1aaa 100644 --- a/src/ameba/rule/naming/rescued_exceptions_variable_name.cr +++ b/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