From 2eff8326698cfbfc40878a8f7292c0e7847527bc Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sat, 9 Jan 2021 20:48:18 +0100 Subject: [PATCH 01/37] Add --rules switch to the CLI --- spec/ameba/cli/cmd_spec.cr | 10 ++++++++++ src/ameba/cli/cmd.cr | 20 ++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/spec/ameba/cli/cmd_spec.cr b/spec/ameba/cli/cmd_spec.cr index f44ff6c6b..def5b6bff 100644 --- a/spec/ameba/cli/cmd_spec.cr +++ b/spec/ameba/cli/cmd_spec.cr @@ -42,6 +42,16 @@ module Ameba::Cli c.except.should eq %w(RULE1 RULE2) end + it "defaults rules? flag to false" do + c = Cli.parse_args %w(file.cr) + c.rules?.should eq false + end + + it "accepts --rules flag" do + c = Cli.parse_args %w(--rules) + c.rules?.should eq true + end + it "defaults all? flag to false" do c = Cli.parse_args %w(file.cr) c.all?.should eq false diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 42778da28..e34ba2415 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -14,6 +14,10 @@ module Ameba::Cli configure_formatter(config, opts) configure_rules(config, opts) + if opts.rules? + print_rules(config) + end + runner = Ameba.run(config) if location = opts.location_to_explain @@ -34,6 +38,7 @@ module Ameba::Cli property except : Array(String)? property location_to_explain : NamedTuple(file: String, line: Int32, column: Int32)? property fail_level : Severity? + property? rules = false property? all = false property? colors = true property? without_affected_code = false @@ -44,7 +49,8 @@ module Ameba::Cli parser.banner = "Usage: ameba [options] [file1 file2 ...]" parser.on("-v", "--version", "Print version") { print_version } - parser.on("-h", "--help", "Show this help") { show_help parser } + 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+/ @@ -145,8 +151,18 @@ module Ameba::Cli exit 0 end - private def show_help(parser) + private def print_help(parser) puts parser exit 0 end + + private def print_rules(config) + config.rules.each do |rule| + puts \ + "#{rule.name.colorize(:white)} " \ + "[#{rule.severity.symbol.to_s.colorize(:green)}] - " \ + "#{rule.description.colorize(:dark_gray)}" + end + exit 0 + end end From 19c370aee06a6c7bbaec7c42b1d3331f77256fc5 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 11 Jan 2021 19:13:58 +0100 Subject: [PATCH 02/37] Misc refactors (#180) * Optimize Severity#symbol * Remove empty else branches * Optimize map+compact/flatten calls * Misc stylistic refactors --- src/ameba/ast/branchable.cr | 3 +-- src/ameba/ast/flow_expression.cr | 2 -- src/ameba/ast/scope.cr | 10 +++---- src/ameba/cli/cmd.cr | 29 +++++++++++++-------- src/ameba/config.cr | 15 ++++++----- src/ameba/formatter/dot_formatter.cr | 9 ++++--- src/ameba/formatter/explain_formatter.cr | 15 +++++------ src/ameba/formatter/flycheck_formatter.cr | 2 +- src/ameba/formatter/todo_formatter.cr | 18 ++++++------- src/ameba/formatter/util.cr | 6 ++--- src/ameba/glob_utils.cr | 4 +-- src/ameba/inline_comments.cr | 6 +++-- src/ameba/rule/base.cr | 8 +++--- src/ameba/rule/lint/percent_array.cr | 4 --- src/ameba/rule/lint/redundant_with_index.cr | 2 -- src/ameba/rule/style/redundant_begin.cr | 4 +-- src/ameba/runner.cr | 6 ++--- src/ameba/severity.cr | 8 ++++-- src/ameba/source.cr | 25 +++++++----------- src/ameba/tokenizer.cr | 9 +------ 20 files changed, 87 insertions(+), 98 deletions(-) diff --git a/src/ameba/ast/branchable.cr b/src/ameba/ast/branchable.cr index 2495dbb2a..4bf9819ef 100644 --- a/src/ameba/ast/branchable.cr +++ b/src/ameba/ast/branchable.cr @@ -37,8 +37,7 @@ module Ameba::AST # Returns true if this node or one of the parent branchables is a loop, false otherwise. def loop? - return true if loop?(node) - parent.try(&.loop?) || false + loop?(node) || parent.try(&.loop?) || false end end end diff --git a/src/ameba/ast/flow_expression.cr b/src/ameba/ast/flow_expression.cr index 6b2149622..8c80eb531 100644 --- a/src/ameba/ast/flow_expression.cr +++ b/src/ameba/ast/flow_expression.cr @@ -59,8 +59,6 @@ module Ameba::AST end when Crystal::BinaryOp unreachable_nodes << current_node.right if flow_expression?(current_node.left, in_loop?) - else - # nop end unreachable_nodes diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 3627ef73e..a6a8ee081 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -117,8 +117,8 @@ module Ameba::AST # Returns true instance variable assinged in this scope. def assigns_ivar?(name) - arguments.find { |arg| arg.name == name } && - ivariables.find { |var| var.name == "@#{name}" } + arguments.find(&.name.== name) && + ivariables.find(&.name.== "@#{name}") end # Returns true if and only if current scope represents some @@ -137,13 +137,13 @@ module Ameba::AST def references?(variable : Variable) variable.references.any? do |reference| reference.scope == self || - inner_scopes.any?(&.references? variable) + inner_scopes.any?(&.references?(variable)) end || variable.used_in_macro? end # Returns true if current scope is a def, false if not. def def? - node.is_a? Crystal::Def + node.is_a?(Crystal::Def) end # Returns true if this scope is a top level scope, false if not. @@ -173,7 +173,7 @@ module Ameba::AST # the same Crystal node as `@node`. def eql?(node) node == @node && - !node.location.nil? && + node.location && node.location == @node.location end end diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index e34ba2415..7b9e6bae3 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -72,12 +72,12 @@ module Ameba::Cli parser.on("--only RULE1,RULE2,...", "Run only given rules (or groups)") do |rules| - opts.only = rules.split "," + opts.only = rules.split(',') end parser.on("--except RULE1,RULE2,...", "Disable the given rules (or groups)") do |rules| - opts.except = rules.split "," + opts.except = rules.split(',') end parser.on("--all", "Enables all available rules") do @@ -90,7 +90,8 @@ module Ameba::Cli opts.config = "" end - parser.on("--fail-level SEVERITY", "Change the level of failure to exit. Defaults to Convention") do |level| + parser.on("--fail-level SEVERITY", + "Change the level of failure to exit. Defaults to Convention") do |level| opts.fail_level = Severity.parse(level) end @@ -113,13 +114,13 @@ module Ameba::Cli end private def configure_rules(config, opts) - if only = opts.only - config.rules.map! { |r| r.enabled = false; r } + case + when only = opts.only + config.rules.map! &.tap(&.enabled = false) config.update_rules(only, enabled: true) - elsif opts.all? - config.rules.map! { |r| r.enabled = true; r } + when opts.all? + config.rules.map! &.tap(&.enabled = true) end - config.update_rules(opts.except, enabled: false) end @@ -127,7 +128,8 @@ module Ameba::Cli if name = opts.formatter config.formatter = name end - config.formatter.config[:without_affected_code] = opts.without_affected_code? + config.formatter.config[:without_affected_code] = + opts.without_affected_code? end private def configure_explain_opts(loc, opts) @@ -138,10 +140,15 @@ module Ameba::Cli end private def parse_explain_location(arg) - location = arg.split(":", remove_empty: true).map &.strip + location = arg.split(':', remove_empty: true).map! &.strip raise ArgumentError.new unless location.size === 3 + file, line, column = location - {file: file, line: line.to_i, column: column.to_i} + { + file: file, + line: line.to_i, + column: column.to_i, + } rescue raise "location should have PATH:line:column format" end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 1ecbd3a8c..5bf0261ee 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -31,7 +31,6 @@ class Ameba::Config !lib ) - setter formatter : Formatter::BaseFormatter? getter rules : Array(Rule::Base) property severity = Severity::Convention @@ -66,7 +65,9 @@ class Ameba::Config @excluded = load_array_section(config, "Excluded") @globs = load_array_section(config, "Globs", DEFAULT_GLOBS) - self.formatter = load_formatter_name(config) + if formatter_name = load_formatter_name(config) + self.formatter = formatter_name + end end # Loads YAML configuration file by `path`. @@ -84,7 +85,7 @@ class Ameba::Config end def self.formatter_names - AVAILABLE_FORMATTERS.keys.join("|") + AVAILABLE_FORMATTERS.keys.join('|') end # Returns a list of sources matching globs and excluded sections. @@ -111,8 +112,8 @@ class Ameba::Config # config.formatter # ``` # - def formatter - @formatter ||= Formatter::DotFormatter.new + property formatter : Formatter::BaseFormatter do + Formatter::DotFormatter.new end # Sets formatter by name. @@ -138,7 +139,7 @@ class Ameba::Config # ``` # def update_rule(name, enabled = true, excluded = nil) - index = @rules.index { |r| r.name == name } + index = @rules.index { |rule| rule.name == name } raise ArgumentError.new("Rule `#{name}` does not exist") unless index rule = @rules[index] @@ -172,7 +173,7 @@ class Ameba::Config private def load_formatter_name(config) name = config["Formatter"]?.try &.["Name"]? - name ? name.to_s : nil + name.try(&.to_s) end private def load_array_section(config, section_name, default = [] of String) diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr index 396069e53..54fe53ac9 100644 --- a/src/ameba/formatter/dot_formatter.cr +++ b/src/ameba/formatter/dot_formatter.cr @@ -35,14 +35,17 @@ module Ameba::Formatter next if issue.disabled? next if (location = issue.location).nil? - output << "#{location}\n".colorize(:cyan) - output << "[#{issue.rule.severity.symbol}] #{issue.rule.name}: #{issue.message}\n".colorize(:red) + output.puts location.colorize(:cyan) + output.puts \ + "[#{issue.rule.severity.symbol}] " \ + "#{issue.rule.name}: " \ + "#{issue.message}".colorize(:red) if show_affected_code && (code = affected_code(source, location)) output << code.colorize(:default) end - output << "\n" + output << '\n' end end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index 6670245ba..9220ae3fa 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -4,9 +4,8 @@ module Ameba::Formatter # A formatter that shows the detailed explanation of the issue at # a specific location. class ExplainFormatter - LINE_BREAK = "\n" - HEADING = "## " - PREFIX = " " + HEADING = "## " + PREFIX = " " include Util @@ -65,19 +64,19 @@ module Ameba::Formatter end private def output_title(title) - output << HEADING.colorize(:yellow) << title.colorize(:yellow) << LINE_BREAK - output << LINE_BREAK + output << HEADING.colorize(:yellow) << title.colorize(:yellow) << '\n' + output << '\n' end private def output_paragraph(paragraph : String) - output_paragraph(paragraph.split(LINE_BREAK)) + output_paragraph(paragraph.split('\n')) end private def output_paragraph(paragraph : Array(String)) paragraph.each do |line| - output << PREFIX << line << LINE_BREAK + output << PREFIX << line << '\n' end - output << LINE_BREAK + output << '\n' end end end diff --git a/src/ameba/formatter/flycheck_formatter.cr b/src/ameba/formatter/flycheck_formatter.cr index 89423ff39..50d5de6ca 100644 --- a/src/ameba/formatter/flycheck_formatter.cr +++ b/src/ameba/formatter/flycheck_formatter.cr @@ -9,7 +9,7 @@ module Ameba::Formatter @mutex.synchronize do output.printf "%s:%d:%d: %s: [%s] %s\n", source.path, loc.line_number, loc.column_number, e.rule.severity.symbol, - e.rule.name, e.message.gsub("\n", " ") + e.rule.name, e.message.gsub('\n', " ") end end end diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index fd0b81000..88584d1d6 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/src/ameba/formatter/todo_formatter.cr @@ -8,19 +8,20 @@ module Ameba::Formatter def finished(sources) super - issues = sources.map(&.issues).flatten + + issues = sources.flat_map(&.issues) unless issues.any? { |issue| !issue.disabled? } - @output << "No issues found. File is not generated.\n" + @output.puts "No issues found. File is not generated." return end - if issues.any? { |issue| issue.syntax? } - @output << "Unable to generate TODO file. Please fix syntax issues.\n" + if issues.any?(&.syntax?) + @output.puts "Unable to generate TODO file. Please fix syntax issues." return end file = generate_todo_config issues - @output << "Created #{file.path}\n" + @output.puts "Created #{file.path}" file end @@ -57,10 +58,9 @@ module Ameba::Formatter end private def rule_todo(rule, issues) - rule.excluded = - issues.map(&.location.try &.filename.try &.to_s) - .compact - .uniq! + rule.excluded = issues + .compact_map(&.location.try &.filename.try &.to_s) + .uniq! {rule.name => rule}.to_yaml end diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr index 7219a67bb..26410313d 100644 --- a/src/ameba/formatter/util.cr +++ b/src/ameba/formatter/util.cr @@ -2,9 +2,9 @@ module Ameba::Formatter module Util def affected_code(source, location, max_length = 100, placeholder = " ...", prompt = "> ") line, column = location.line_number, location.column_number - affected_line = source.lines[line - 1]? + affected_line = source.lines[line - 1]?.presence - return if affected_line.nil? || affected_line.strip.empty? + return unless affected_line if affected_line.size > max_length && column < max_length affected_line = affected_line[0, max_length - placeholder.size - 1] + placeholder @@ -14,7 +14,7 @@ module Ameba::Formatter position = column - (affected_line.size - stripped.size) + prompt.size String.build do |str| - str << prompt << stripped << "\n" + str << prompt << stripped << '\n' str << " " * (position - 1) str << "^".colorize(:yellow) end diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr index a714f8851..b014bf389 100644 --- a/src/ameba/glob_utils.cr +++ b/src/ameba/glob_utils.cr @@ -22,10 +22,10 @@ module Ameba # ``` # def expand(globs) - globs.map do |glob| + globs.flat_map do |glob| glob += "/**/*.cr" if File.directory?(glob) Dir[glob] - end.flatten.uniq! + end.uniq! end private def rejected_globs(globs) diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr index cece57fec..7d1ec14d9 100644 --- a/src/ameba/inline_comments.cr +++ b/src/ameba/inline_comments.cr @@ -89,8 +89,10 @@ module Ameba private def line_disabled?(line, rule) return false unless directive = parse_inline_directive(line) - Action.parse?(directive[:action]).try(&.disable?) && - (directive[:rules].includes?(rule.name) || directive[:rules].includes?(rule.group)) + return false unless Action.parse?(directive[:action]).try(&.disable?) + + directive[:rules].includes?(rule.name) || + directive[:rules].includes?(rule.group) end private def commented_out?(line) diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index c895499a2..de4c363ff 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -95,7 +95,7 @@ module Ameba::Rule def excluded?(source) excluded.try &.any? do |path| source.matches_path?(path) || - Dir.glob(path).any? { |glob| source.matches_path? glob } + Dir.glob(path).any? { |glob| source.matches_path?(glob) } end end @@ -123,11 +123,11 @@ module Ameba::Rule end protected def self.rule_name - name.gsub("Ameba::Rule::", "").gsub("::", "/") + name.gsub("Ameba::Rule::", "").gsub("::", '/') end protected def self.group_name - rule_name.split("/")[0...-1].join("/") + rule_name.split('/')[0...-1].join('/') end protected def self.subclasses @@ -157,7 +157,7 @@ module Ameba::Rule def self.parsed_doc source = File.read(path_to_source_file) nodes = Crystal::Parser.new(source).tap(&.wants_doc = true).parse - type_name = rule_name.split("/").last? + type_name = rule_name.split('/').last? DocFinder.new(nodes, type_name).doc end diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr index 0f1d3de82..afdf02230 100644 --- a/src/ameba/rule/lint/percent_array.cr +++ b/src/ameba/rule/lint/percent_array.cr @@ -49,8 +49,6 @@ module Ameba::Rule::Lint issue_for start_token.not_nil!, issue.not_nil! end issue = start_token = nil - else - # nop end end end @@ -61,8 +59,6 @@ module Ameba::Rule::Lint check_array_entry entry, string_array_unwanted_symbols, "%w" when .starts_with? "%i" check_array_entry entry, symbol_array_unwanted_symbols, "%i" - else - # nop end end diff --git a/src/ameba/rule/lint/redundant_with_index.cr b/src/ameba/rule/lint/redundant_with_index.cr index f67766d6d..d9c646f39 100644 --- a/src/ameba/rule/lint/redundant_with_index.cr +++ b/src/ameba/rule/lint/redundant_with_index.cr @@ -42,8 +42,6 @@ module Ameba::Rule::Lint report source, node, "Remove redundant with_index" when "each_with_index" report source, node, "Use each instead of each_with_index" - else - # nop end end diff --git a/src/ameba/rule/style/redundant_begin.cr b/src/ameba/rule/style/redundant_begin.cr index bfb028a63..1318bb4b2 100644 --- a/src/ameba/rule/style/redundant_begin.cr +++ b/src/ameba/rule/style/redundant_begin.cr @@ -76,8 +76,6 @@ module Ameba::Rule::Style redundant_begin_in_handler?(source, body, node) when Crystal::Expressions redundant_begin_in_expressions?(body) - else - # nop end end @@ -88,7 +86,7 @@ module Ameba::Rule::Style private def redundant_begin_in_handler?(source, handler, node) return false if begin_exprs_in_handler?(handler) || inner_handler?(handler) - code = node_source(node, source.lines).try &.join("\n") + code = node_source(node, source.lines).try &.join('\n') def_redundant_begin? code if code rescue false diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index ece391877..e742dab2c 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -50,7 +50,6 @@ module Ameba .find &.name.==(Rule::Lint::UnneededDisableDirective.rule_name) end - # :nodoc: protected def initialize(@rules, @sources, @formatter, @severity) end @@ -80,9 +79,8 @@ module Ameba end end - channels.each do |c| - e = c.receive - raise e unless e.nil? + channels.each do |chan| + chan.receive.try { |e| raise e } end self diff --git a/src/ameba/severity.cr b/src/ameba/severity.cr index 917cf4e98..3b0e4327c 100644 --- a/src/ameba/severity.cr +++ b/src/ameba/severity.cr @@ -9,8 +9,12 @@ module Ameba # ``` # Severity::Warning.symbol # => 'W' # ``` - def symbol - to_s[0] + def symbol : Char + case self + in Error then 'E' + in Warning then 'W' + in Convention then 'C' + end end # Creates Severity by the name. diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 3fd8d751d..70d38627a 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -11,10 +11,6 @@ module Ameba # Crystal code (content of a source file). getter code : String - @lines : Array(String)? - @ast : Crystal::ASTNode? - @fullpath : String? - # Creates a new source by `code` and `path`. # # For example: @@ -24,7 +20,7 @@ module Ameba # Ameba::Source.new File.read(path), path # ``` # - def initialize(@code : String, @path = "") + def initialize(@code, @path = "") end # Returns lines of code splitted by new line character. @@ -38,9 +34,7 @@ module Ameba # source.lines # => ["a = 1", "b = 2"] # ``` # - def lines - @lines ||= @code.split("\n") - end + getter lines : Array(String) { code.split('\n') } # Returns AST nodes constructed by `Crystal::Parser`. # @@ -49,16 +43,15 @@ module Ameba # source.ast # ``` # - def ast - @ast ||= - Crystal::Parser.new(code) - .tap { |parser| parser.wants_doc = true } - .tap { |parser| parser.filename = @path } - .parse + getter ast : Crystal::ASTNode do + Crystal::Parser.new(code) + .tap(&.wants_doc = true) + .tap(&.filename = path) + .parse end - def fullpath - @fullpath ||= File.expand_path @path + getter fullpath : String do + File.expand_path(path) end # Returns true if *filepath* matches the source's path, false if it does not. diff --git a/src/ameba/tokenizer.cr b/src/ameba/tokenizer.cr index 070e5b552..e79d12cb4 100644 --- a/src/ameba/tokenizer.cr +++ b/src/ameba/tokenizer.cr @@ -53,8 +53,7 @@ module Ameba false end - private def run_normal_state(lexer, break_on_rcurly = false, - &block : Crystal::Token -> _) + private def run_normal_state(lexer, break_on_rcurly = false, &block : Crystal::Token -> _) loop do token = @lexer.next_token block.call token @@ -68,8 +67,6 @@ module Ameba break when :"}" break if break_on_rcurly - else - # go on end end end @@ -86,8 +83,6 @@ module Ameba run_normal_state lexer, break_on_rcurly: true, &block when :EOF break - else - # go on end end end @@ -102,8 +97,6 @@ module Ameba break when :EOF break - else - # go on end end end From c4d34d74d86bdd978b5ae34136d6c980ab7e1cda Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 12 Jan 2021 16:20:43 +0100 Subject: [PATCH 03/37] Add support for showing code context lines (#181) * Add support for showing code context lines * Show context lines only in ExplainFormatter * Add spec coverage for context_lines option --- spec/ameba/formatter/util_spec.cr | 33 ++++++++++- src/ameba/formatter/explain_formatter.cr | 2 +- src/ameba/formatter/util.cr | 74 ++++++++++++++++++++---- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/spec/ameba/formatter/util_spec.cr b/spec/ameba/formatter/util_spec.cr index e2f95423d..9a002baa7 100644 --- a/spec/ameba/formatter/util_spec.cr +++ b/spec/ameba/formatter/util_spec.cr @@ -22,7 +22,38 @@ module Ameba::Formatter a = 1 ) location = Crystal::Location.new("filename", 1, 1) - subject.affected_code(source, location).should eq "> a = 1\n \e[33m^\e[0m" + subject.deansify(subject.affected_code(source, location)) + .should eq "> a = 1\n ^" + end + + it "returns correct line if it is found" do + source = Source.new <<-EOF + # pre:1 + # pre:2 + # pre:3 + # pre:4 + # pre:5 + a = 1 + # post:1 + # post:2 + # post:3 + # post:4 + # post:5 + EOF + + location = Crystal::Location.new("filename", 6, 1) + subject.deansify(subject.affected_code(source, location, context_lines: 3)) + .should eq <<-STR + > # pre:3 + > # pre:4 + > # pre:5 + > a = 1 + ^ + > # post:1 + > # post:2 + > # post:3 + + STR end end end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index 9220ae3fa..87fb15b6d 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -49,7 +49,7 @@ module Ameba::Formatter @location.to_s.colorize(:cyan).to_s, ] - if affected_code = affected_code(source, @location) + if affected_code = affected_code(source, @location, context_lines: 3) output_title "AFFECTED CODE" output_paragraph affected_code end diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr index 26410313d..822489f06 100644 --- a/src/ameba/formatter/util.cr +++ b/src/ameba/formatter/util.cr @@ -1,22 +1,74 @@ module Ameba::Formatter module Util - def affected_code(source, location, max_length = 100, placeholder = " ...", prompt = "> ") - line, column = location.line_number, location.column_number - affected_line = source.lines[line - 1]?.presence + def deansify(message : String?) : String? + message.try &.gsub(/\x1b[^m]*m/, "").presence + end + + def affected_code(source, location, context_lines = 0, max_length = 100, placeholder = " ...", prompt = "> ") + lines = source.lines + lineno, column = + location.line_number, location.column_number + + return unless affected_line = lines[lineno - 1]?.presence - return unless affected_line + trim_line = Proc(String, String).new do |line| + if line.size > max_length + line = line[0, max_length - placeholder.size - 1] + placeholder + end + line + end - if affected_line.size > max_length && column < max_length - affected_line = affected_line[0, max_length - placeholder.size - 1] + placeholder + if column < max_length + affected_line = trim_line.call(affected_line) end - stripped = affected_line.lstrip - position = column - (affected_line.size - stripped.size) + prompt.size + show_context = context_lines > 0 + if show_context + pre_context, post_context = %w[], %w[] + + lines.each_with_index do |line, i| + case i + 1 + when lineno - context_lines...lineno + pre_context << trim_line.call(line) + when lineno + # + when lineno + 1..lineno + context_lines + post_context << trim_line.call(line) + end + end + + # remove empty lines at the beginning/end + pre_context.shift? unless pre_context.first?.presence + post_context.pop? unless post_context.last?.presence + end String.build do |str| - str << prompt << stripped << '\n' - str << " " * (position - 1) - str << "^".colorize(:yellow) + if show_context + pre_context.try &.each do |line| + str << prompt + str.puts(line.colorize(:dark_gray)) + end + + str << prompt + str.puts(affected_line.colorize(:white)) + + str << " " * (prompt.size + column - 1) + str.puts("^".colorize(:yellow)) + + post_context.try &.each do |line| + str << prompt + str.puts(line.colorize(:dark_gray)) + end + else + stripped = affected_line.lstrip + position = column - (affected_line.size - stripped.size) + prompt.size + + str << prompt + str.puts(stripped) + + str << " " * (position - 1) + str << "^".colorize(:yellow) + end end end end From 6f0f8352f9c0189bf4c6416e77d47ca1e1c80e04 Mon Sep 17 00:00:00 2001 From: Anton Maminov Date: Sat, 16 Jan 2021 20:42:27 +0200 Subject: [PATCH 04/37] fix Crystal nightly --- src/ameba/reportable.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr index 354539506..e31ef3a74 100644 --- a/src/ameba/reportable.cr +++ b/src/ameba/reportable.cr @@ -5,7 +5,7 @@ module Ameba getter issues = [] of Issue # Adds a new issue to the list of issues. - def add_issue(rule, location, end_location, message, status = nil) + def add_issue(rule, location : Crystal::Location?, end_location : Crystal::Location?, message, status = nil) status ||= :disabled if location_disabled?(location, rule) issues << Issue.new rule, location, end_location, message, status end From 1a091c1f1a5b6c52ee10c047975847c5f81fd198 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 13:16:04 +0100 Subject: [PATCH 05/37] Optimize rules (#185) * Replace pointless interpolations with actual values * Rules optimizations * Stylistic refactors * Remove extraneous blank lines * Remove some instances of .not_nil! usage --- spec/ameba/rule/lint/redundant_with_object_spec.cr | 2 +- src/ameba/cli/cmd.cr | 9 +++++++-- src/ameba/rule/layout/line_length.cr | 5 +---- src/ameba/rule/layout/trailing_blank_lines.cr | 1 - src/ameba/rule/layout/trailing_whitespace.cr | 4 +--- src/ameba/rule/lint/bad_directive.cr | 5 +++-- src/ameba/rule/lint/comparison_to_boolean.cr | 5 +---- src/ameba/rule/lint/debugger_statement.cr | 1 - src/ameba/rule/lint/empty_ensure.cr | 1 - src/ameba/rule/lint/empty_expression.cr | 4 +--- src/ameba/rule/lint/hash_duplicated_key.cr | 5 ++--- src/ameba/rule/lint/literal_in_condition.cr | 1 - src/ameba/rule/lint/literal_in_interpolation.cr | 1 - src/ameba/rule/lint/percent_array.cr | 1 - src/ameba/rule/lint/rand_zero.cr | 3 +-- src/ameba/rule/lint/redundant_string_coercion.cr | 1 - src/ameba/rule/lint/redundant_with_index.cr | 4 ++-- src/ameba/rule/lint/redundant_with_object.cr | 7 +++---- src/ameba/rule/lint/shadowed_argument.cr | 1 - src/ameba/rule/lint/shadowed_exception.cr | 1 - src/ameba/rule/lint/shadowing_local_outer_var.cr | 1 - src/ameba/rule/lint/shared_var_in_fiber.cr | 3 +-- src/ameba/rule/lint/syntax.cr | 1 - src/ameba/rule/lint/unneeded_disable_directive.cr | 4 ++-- src/ameba/rule/lint/unreachable_code.cr | 1 - src/ameba/rule/lint/unused_argument.cr | 1 - src/ameba/rule/lint/useless_assign.cr | 1 - src/ameba/rule/lint/useless_condition_in_when.cr | 6 +----- src/ameba/rule/metrics/cyclomatic_complexity.cr | 14 +++++++------- src/ameba/rule/performance/any_after_filter.cr | 8 ++++---- .../rule/performance/first_last_after_filter.cr | 7 +++---- src/ameba/rule/performance/size_after_filter.cr | 8 ++++---- src/ameba/rule/style/constant_names.cr | 3 +-- src/ameba/rule/style/is_a_nil.cr | 5 +++-- src/ameba/rule/style/large_numbers.cr | 5 ++--- src/ameba/rule/style/method_names.cr | 3 +-- .../rule/style/negated_conditions_in_unless.cr | 4 +--- src/ameba/rule/style/predicate_name.cr | 1 - src/ameba/rule/style/redundant_begin.cr | 5 +---- src/ameba/rule/style/type_names.cr | 3 +-- src/ameba/rule/style/unless_else.cr | 4 +--- src/ameba/rule/style/variable_names.cr | 1 - src/ameba/rule/style/while_true.cr | 4 +--- 43 files changed, 57 insertions(+), 98 deletions(-) diff --git a/spec/ameba/rule/lint/redundant_with_object_spec.cr b/spec/ameba/rule/lint/redundant_with_object_spec.cr index f6adc130e..bc3a9baf6 100644 --- a/spec/ameba/rule/lint/redundant_with_object_spec.cr +++ b/spec/ameba/rule/lint/redundant_with_object_spec.cr @@ -77,7 +77,7 @@ module Ameba::Rule::Lint issue.rule.should_not be_nil issue.location.to_s.should eq "source.cr:2:14" issue.end_location.to_s.should eq "source.cr:2:30" - issue.message.should eq "Use each instead of each_with_object" + issue.message.should eq "Use `each` instead of `each_with_object`" end end end diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 7b9e6bae3..d32543c45 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -8,8 +8,13 @@ module Ameba::Cli def run(args = ARGV) opts = parse_args args config = Config.load opts.config, opts.colors? - config.globs = opts.globs.not_nil! if opts.globs - config.severity = opts.fail_level.not_nil! if opts.fail_level + + if globs = opts.globs + config.globs = globs + end + if fail_level = opts.fail_level + config.severity = fail_level + end configure_formatter(config, opts) configure_rules(config, opts) diff --git a/src/ameba/rule/layout/line_length.cr b/src/ameba/rule/layout/line_length.cr index a4c24d37d..df8430701 100644 --- a/src/ameba/rule/layout/line_length.cr +++ b/src/ameba/rule/layout/line_length.cr @@ -8,7 +8,6 @@ module Ameba::Rule::Layout # Enabled: true # MaxLength: 100 # ``` - # struct LineLength < Base properties do enabled false @@ -20,9 +19,7 @@ module Ameba::Rule::Layout def test(source) source.lines.each_with_index do |line, index| - next unless line.size > max_length - - issue_for({index + 1, max_length + 1}, MSG) + issue_for({index + 1, max_length + 1}, MSG) if line.size > max_length end end end diff --git a/src/ameba/rule/layout/trailing_blank_lines.cr b/src/ameba/rule/layout/trailing_blank_lines.cr index 24499e9c0..bf2db9d67 100644 --- a/src/ameba/rule/layout/trailing_blank_lines.cr +++ b/src/ameba/rule/layout/trailing_blank_lines.cr @@ -7,7 +7,6 @@ module Ameba::Rule::Layout # Layout/TrailingBlankLines: # Enabled: true # ``` - # struct TrailingBlankLines < Base properties do description "Disallows trailing blank lines" diff --git a/src/ameba/rule/layout/trailing_whitespace.cr b/src/ameba/rule/layout/trailing_whitespace.cr index a4df797ee..77456c1a7 100644 --- a/src/ameba/rule/layout/trailing_whitespace.cr +++ b/src/ameba/rule/layout/trailing_whitespace.cr @@ -7,7 +7,6 @@ module Ameba::Rule::Layout # Layout/TrailingWhitespace: # Enabled: true # ``` - # struct TrailingWhitespace < Base properties do description "Disallows trailing whitespaces" @@ -17,8 +16,7 @@ module Ameba::Rule::Layout def test(source) source.lines.each_with_index do |line, index| - next unless line =~ /\s$/ - issue_for({index + 1, line.size}, MSG) + issue_for({index + 1, line.size}, MSG) if line =~ /\s$/ end end end diff --git a/src/ameba/rule/lint/bad_directive.cr b/src/ameba/rule/lint/bad_directive.cr index 76b6d3589..1fcfd896a 100644 --- a/src/ameba/rule/lint/bad_directive.cr +++ b/src/ameba/rule/lint/bad_directive.cr @@ -17,7 +17,6 @@ module Ameba::Rule::Lint # Lint/BadDirective: # Enabled: true # ``` - # struct BadDirective < Base properties do description "Reports bad comment directives" @@ -48,7 +47,9 @@ module Ameba::Rule::Lint private def check_rules(source, token, rules) bad_names = rules - ALL_RULE_NAMES - ALL_GROUP_NAMES - issue_for token, "Such rules do not exist: %s" % bad_names.join(", ") unless bad_names.empty? + return if bad_names.empty? + + issue_for token, "Such rules do not exist: %s" % bad_names.join(", ") end end end diff --git a/src/ameba/rule/lint/comparison_to_boolean.cr b/src/ameba/rule/lint/comparison_to_boolean.cr index 313db5a22..649d61a56 100644 --- a/src/ameba/rule/lint/comparison_to_boolean.cr +++ b/src/ameba/rule/lint/comparison_to_boolean.cr @@ -19,7 +19,6 @@ module Ameba::Rule::Lint # Lint/ComparisonToBoolean: # Enabled: true # ``` - # struct ComparisonToBoolean < Base properties do enabled false @@ -33,9 +32,7 @@ module Ameba::Rule::Lint to_boolean = node.args.first?.try &.is_a?(Crystal::BoolLiteral) || node.obj.is_a?(Crystal::BoolLiteral) - return unless comparison && to_boolean - - issue_for node, MSG + issue_for node, MSG if comparison && to_boolean end end end diff --git a/src/ameba/rule/lint/debugger_statement.cr b/src/ameba/rule/lint/debugger_statement.cr index 976744901..b5131384c 100644 --- a/src/ameba/rule/lint/debugger_statement.cr +++ b/src/ameba/rule/lint/debugger_statement.cr @@ -10,7 +10,6 @@ module Ameba::Rule::Lint # Lint/DebuggerStatement: # Enabled: true # ``` - # struct DebuggerStatement < Base properties do description "Disallows calls to debugger" diff --git a/src/ameba/rule/lint/empty_ensure.cr b/src/ameba/rule/lint/empty_ensure.cr index f8fdeb48e..94deeb369 100644 --- a/src/ameba/rule/lint/empty_ensure.cr +++ b/src/ameba/rule/lint/empty_ensure.cr @@ -38,7 +38,6 @@ module Ameba::Rule::Lint # Lint/EmptyEnsure # Enabled: true # ``` - # struct EmptyEnsure < Base properties do description "Disallows empty ensure statement" diff --git a/src/ameba/rule/lint/empty_expression.cr b/src/ameba/rule/lint/empty_expression.cr index 682ed8a35..e3fae35ac 100644 --- a/src/ameba/rule/lint/empty_expression.cr +++ b/src/ameba/rule/lint/empty_expression.cr @@ -27,7 +27,6 @@ module Ameba::Rule::Lint # Lint/EmptyExpression: # Enabled: true # ``` - # struct EmptyExpression < Base include AST::Util @@ -41,8 +40,7 @@ module Ameba::Rule::Lint def test(source, node : Crystal::NilLiteral) exp = node_source(node, source.lines).try &.join - - return if exp.nil? || exp == "nil" + return if exp.in?(nil, "nil") issue_for node, MSG % exp end diff --git a/src/ameba/rule/lint/hash_duplicated_key.cr b/src/ameba/rule/lint/hash_duplicated_key.cr index c9de6a788..6a581b4f5 100644 --- a/src/ameba/rule/lint/hash_duplicated_key.cr +++ b/src/ameba/rule/lint/hash_duplicated_key.cr @@ -19,7 +19,6 @@ module Ameba::Rule::Lint # Lint/HashDuplicatedKey: # Enabled: true # ``` - # struct HashDuplicatedKey < Base properties do description "Disallows duplicated keys in hash literals" @@ -28,7 +27,7 @@ module Ameba::Rule::Lint MSG = "Duplicated keys in hash literal: %s" def test(source, node : Crystal::HashLiteral) - return unless (keys = duplicated_keys(node.entries)).any? + return if (keys = duplicated_keys(node.entries)).empty? issue_for node, MSG % keys.join(", ") end @@ -36,7 +35,7 @@ module Ameba::Rule::Lint private def duplicated_keys(entries) entries.map(&.key) .group_by(&.itself) - .select { |_, v| v.size > 1 } + .tap(&.select! { |_, v| v.size > 1 }) .map { |k, _| k } end end diff --git a/src/ameba/rule/lint/literal_in_condition.cr b/src/ameba/rule/lint/literal_in_condition.cr index 2dc2e6004..154785169 100644 --- a/src/ameba/rule/lint/literal_in_condition.cr +++ b/src/ameba/rule/lint/literal_in_condition.cr @@ -19,7 +19,6 @@ module Ameba::Rule::Lint # Lint/LiteralInCondition: # Enabled: true # ``` - # struct LiteralInCondition < Base include AST::Util diff --git a/src/ameba/rule/lint/literal_in_interpolation.cr b/src/ameba/rule/lint/literal_in_interpolation.cr index df674f74f..6c96c1c87 100644 --- a/src/ameba/rule/lint/literal_in_interpolation.cr +++ b/src/ameba/rule/lint/literal_in_interpolation.cr @@ -15,7 +15,6 @@ module Ameba::Rule::Lint # Lint/LiteralInInterpolation # Enabled: true # ``` - # struct LiteralInInterpolation < Base include AST::Util diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr index afdf02230..b8bb1b547 100644 --- a/src/ameba/rule/lint/percent_array.cr +++ b/src/ameba/rule/lint/percent_array.cr @@ -23,7 +23,6 @@ module Ameba::Rule::Lint # StringArrayUnwantedSymbols: ',"' # SymbolArrayUnwantedSymbols: ',:' # ``` - # struct PercentArrays < Base properties do description "Disallows some unwanted symbols in percent array literals" diff --git a/src/ameba/rule/lint/rand_zero.cr b/src/ameba/rule/lint/rand_zero.cr index 6c2b70f28..efd8ab7b6 100644 --- a/src/ameba/rule/lint/rand_zero.cr +++ b/src/ameba/rule/lint/rand_zero.cr @@ -22,7 +22,6 @@ module Ameba::Rule::Lint # Lint/RandZero: # Enabled: true # ``` - # struct RandZero < Base properties do description "Disallows rand zero calls" @@ -36,7 +35,7 @@ module Ameba::Rule::Lint (arg = node.args.first) && (arg.is_a? Crystal::NumberLiteral) && (value = arg.value) && - (value == "0" || value == "1") + value.in?("0", "1") issue_for node, MSG % node end diff --git a/src/ameba/rule/lint/redundant_string_coercion.cr b/src/ameba/rule/lint/redundant_string_coercion.cr index f21484a22..4e85de492 100644 --- a/src/ameba/rule/lint/redundant_string_coercion.cr +++ b/src/ameba/rule/lint/redundant_string_coercion.cr @@ -20,7 +20,6 @@ module Ameba::Rule::Lint # Lint/RedundantStringCoersion # Enabled: true # ``` - # struct RedundantStringCoercion < Base include AST::Util diff --git a/src/ameba/rule/lint/redundant_with_index.cr b/src/ameba/rule/lint/redundant_with_index.cr index d9c646f39..35d3ffc91 100644 --- a/src/ameba/rule/lint/redundant_with_index.cr +++ b/src/ameba/rule/lint/redundant_with_index.cr @@ -26,7 +26,6 @@ module Ameba::Rule::Lint # Lint/RedundantWithIndex: # Enabled: true # ``` - # struct RedundantWithIndex < Base properties do description "Disallows redundant `with_index` calls" @@ -35,7 +34,8 @@ module Ameba::Rule::Lint def test(source, node : Crystal::Call) args, block = node.args, node.block - return if args.size > 1 || block.nil? || with_index_arg?(block.not_nil!) + return if block.nil? || args.size > 1 + return if with_index_arg?(block) case node.name when "with_index" diff --git a/src/ameba/rule/lint/redundant_with_object.cr b/src/ameba/rule/lint/redundant_with_object.cr index 357e45d24..585e63849 100644 --- a/src/ameba/rule/lint/redundant_with_object.cr +++ b/src/ameba/rule/lint/redundant_with_object.cr @@ -27,21 +27,20 @@ module Ameba::Rule::Lint # Lint/RedundantWithObject: # Enabled: true # ``` - # struct RedundantWithObject < Base properties do description "Disallows redundant `with_object` calls" end + MSG = "Use `each` instead of `each_with_object`" + def test(source, node : Crystal::Call) return if node.name != "each_with_object" || node.args.size != 1 || node.block.nil? || with_index_arg?(node.block.not_nil!) - issue_for node.name_location, - node.name_end_location, - "Use each instead of each_with_object" + issue_for node.name_location, node.name_end_location, MSG end private def with_index_arg?(block : Crystal::Block) diff --git a/src/ameba/rule/lint/shadowed_argument.cr b/src/ameba/rule/lint/shadowed_argument.cr index 0c27c25df..444c24e45 100644 --- a/src/ameba/rule/lint/shadowed_argument.cr +++ b/src/ameba/rule/lint/shadowed_argument.cr @@ -35,7 +35,6 @@ module Ameba::Rule::Lint # Lint/ShadowedArgument: # Enabled: true # ``` - # struct ShadowedArgument < Base properties do description "Disallows shadowed arguments" diff --git a/src/ameba/rule/lint/shadowed_exception.cr b/src/ameba/rule/lint/shadowed_exception.cr index 535703407..a937f8bb5 100644 --- a/src/ameba/rule/lint/shadowed_exception.cr +++ b/src/ameba/rule/lint/shadowed_exception.cr @@ -33,7 +33,6 @@ module Ameba::Rule::Lint # Lint/ShadowedException: # Enabled: true # ``` - # struct ShadowedException < Base properties do description "Disallows rescued exception that get shadowed" diff --git a/src/ameba/rule/lint/shadowing_local_outer_var.cr b/src/ameba/rule/lint/shadowing_local_outer_var.cr index 846cc4840..dbfc4da05 100644 --- a/src/ameba/rule/lint/shadowing_local_outer_var.cr +++ b/src/ameba/rule/lint/shadowing_local_outer_var.cr @@ -30,7 +30,6 @@ module Ameba::Rule::Lint # Lint/ShadowingOuterLocalVar: # Enabled: true # ``` - # struct ShadowingOuterLocalVar < Base properties do description "Disallows the usage of the same name as outer local variables" \ diff --git a/src/ameba/rule/lint/shared_var_in_fiber.cr b/src/ameba/rule/lint/shared_var_in_fiber.cr index 8011fa036..56cc465ba 100644 --- a/src/ameba/rule/lint/shared_var_in_fiber.cr +++ b/src/ameba/rule/lint/shared_var_in_fiber.cr @@ -49,7 +49,6 @@ module Ameba::Rule::Lint # Lint/SharedVarInFiber: # Enabled: true # ``` - # struct SharedVarInFiber < Base properties do description "Disallows shared variables in fibers." @@ -77,7 +76,7 @@ module Ameba::Rule::Lint declared_in = variable.assignments.first?.try &.branch variable.assignments - .reject { |assign| assign.scope.spawn_block? } + .reject(&.scope.spawn_block?) .any? do |assign| assign.branch.try(&.in_loop?) && assign.branch != declared_in end diff --git a/src/ameba/rule/lint/syntax.cr b/src/ameba/rule/lint/syntax.cr index 8d135401e..6c4efa48f 100644 --- a/src/ameba/rule/lint/syntax.cr +++ b/src/ameba/rule/lint/syntax.cr @@ -18,7 +18,6 @@ module Ameba::Rule::Lint # rescue e : Exception # end # ``` - # struct Syntax < Base properties do description "Reports invalid Crystal syntax" diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr index 6128fbeba..7016f7699 100644 --- a/src/ameba/rule/lint/unneeded_disable_directive.cr +++ b/src/ameba/rule/lint/unneeded_disable_directive.cr @@ -24,7 +24,6 @@ module Ameba::Rule::Lint # Lint/UnneededDisableDirective # Enabled: true # ``` - # struct UnneededDisableDirective < Base properties do description "Reports unneeded disable directives in comments" @@ -47,11 +46,12 @@ module Ameba::Rule::Lint return unless directive[:action] == "disable" directive[:rules].reject do |rule_name| + next if rule_name == self.name source.issues.any? do |issue| issue.rule.name == rule_name && issue.disabled? && issue_at_location?(source, issue, location) - end && rule_name != self.name + end end end diff --git a/src/ameba/rule/lint/unreachable_code.cr b/src/ameba/rule/lint/unreachable_code.cr index ded09bf57..69f3731f2 100644 --- a/src/ameba/rule/lint/unreachable_code.cr +++ b/src/ameba/rule/lint/unreachable_code.cr @@ -41,7 +41,6 @@ module Ameba::Rule::Lint # Lint/UnreachableCode: # Enabled: true # ``` - # struct UnreachableCode < Base include AST::Util diff --git a/src/ameba/rule/lint/unused_argument.cr b/src/ameba/rule/lint/unused_argument.cr index 6abadca80..9615ec4cc 100644 --- a/src/ameba/rule/lint/unused_argument.cr +++ b/src/ameba/rule/lint/unused_argument.cr @@ -24,7 +24,6 @@ module Ameba::Rule::Lint # IgnoreBlocks: false # IgnoreProcs: false # ``` - # struct UnusedArgument < Base properties do description "Disallows unused arguments" diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index bff181889..23f69d4f2 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/src/ameba/rule/lint/useless_assign.cr @@ -25,7 +25,6 @@ module Ameba::Rule::Lint # Lint/UselessAssign: # Enabled: true # ``` - # struct UselessAssign < Base properties do description "Disallows useless variable assignments" diff --git a/src/ameba/rule/lint/useless_condition_in_when.cr b/src/ameba/rule/lint/useless_condition_in_when.cr index 4b2fb36cf..f9f40ca65 100644 --- a/src/ameba/rule/lint/useless_condition_in_when.cr +++ b/src/ameba/rule/lint/useless_condition_in_when.cr @@ -30,7 +30,6 @@ module Ameba::Rule::Lint # Lint/UselessConditionInWhen: # Enabled: true # ``` - # struct UselessConditionInWhen < Base properties do description "Disallows useless conditions in when" @@ -43,10 +42,7 @@ module Ameba::Rule::Lint # simple implementation in future. protected def check_node(source, when_node, cond) cond_s = cond.to_s - return if when_node - .conds - .map(&.to_s) - .none? { |c| c == cond_s } + return if when_node.conds.none?(&.to_s.==(cond_s)) issue_for cond, MSG end diff --git a/src/ameba/rule/metrics/cyclomatic_complexity.cr b/src/ameba/rule/metrics/cyclomatic_complexity.cr index fb2e2fe22..167ebfe77 100644 --- a/src/ameba/rule/metrics/cyclomatic_complexity.cr +++ b/src/ameba/rule/metrics/cyclomatic_complexity.cr @@ -8,7 +8,6 @@ module Ameba::Rule::Metrics # Enabled: true # MaxComplexity: 10 # ``` - # struct CyclomaticComplexity < Base properties do description "Disallows methods with a cyclomatic complexity higher than `MaxComplexity`" @@ -21,17 +20,18 @@ module Ameba::Rule::Metrics complexity = AST::CountingVisitor.new(node).count if complexity > max_complexity && (location = node.name_location) - issue_for( - location, - def_name_end_location(node), - MSG % {complexity, max_complexity} - ) + issue_for location, def_name_end_location(node), MSG % { + complexity, max_complexity, + } end end private def def_name_end_location(node) return unless location = node.name_location - line_number, column_number = location.line_number, location.column_number + + line_number, column_number = + location.line_number, location.column_number + Crystal::Location.new(location.filename, line_number, column_number + node.name.size) end end diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index 5e4ae0772..b8aa3f342 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -24,10 +24,9 @@ module Ameba::Rule::Performance # - select # - reject # ``` - # struct AnyAfterFilter < Base ANY_NAME = "any?" - MSG = "Use `#{ANY_NAME} {...}` instead of `%s {...}.#{ANY_NAME}`" + MSG = "Use `any? {...}` instead of `%s {...}.any?`" properties do filter_names : Array(String) = %w(select reject) @@ -36,9 +35,10 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == ANY_NAME && (obj = node.obj) + return unless obj.is_a?(Crystal::Call) + return if obj.block.nil? || !node.block.nil? - if node.block.nil? && obj.is_a?(Crystal::Call) && - filter_names.includes?(obj.name) && !obj.block.nil? + if filter_names.includes?(obj.name) issue_for obj.name_location, node.name_end_location, MSG % obj.name end end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index a35994230..78f3302a5 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -23,7 +23,6 @@ module Ameba::Rule::Performance # FilterNames: # - select # ``` - # struct FirstLastAfterFilter < Base CALL_NAMES = %w(first last first? last?) MSG = "Use `find {...}` instead of `%s {...}.%s`" @@ -45,10 +44,10 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless CALL_NAMES.includes?(node.name) && (obj = node.obj) - return if node.args.any? + return unless obj.is_a?(Crystal::Call) + return if obj.block.nil? || !node.block.nil? || node.args.any? - if node.block.nil? && obj.is_a?(Crystal::Call) && - filter_names.includes?(obj.name) && !obj.block.nil? + if filter_names.includes?(obj.name) message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name} end diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index 1fe26359b..6a2814326 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -30,10 +30,9 @@ module Ameba::Rule::Performance # - select # - reject # ``` - # struct SizeAfterFilter < Base SIZE_NAME = "size" - MSG = "Use `count {...}` instead of `%s {...}.#{SIZE_NAME}`." + MSG = "Use `count {...}` instead of `%s {...}.size`." properties do filter_names : Array(String) = %w(select reject) @@ -51,9 +50,10 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == SIZE_NAME && (obj = node.obj) + return unless obj.is_a?(Crystal::Call) + return if obj.block.nil? - if obj.is_a?(Crystal::Call) && - filter_names.includes?(obj.name) && !obj.block.nil? + if filter_names.includes?(obj.name) issue_for obj.name_location, node.name_end_location, MSG % obj.name end end diff --git a/src/ameba/rule/style/constant_names.cr b/src/ameba/rule/style/constant_names.cr index 3628006e7..fdd7caada 100644 --- a/src/ameba/rule/style/constant_names.cr +++ b/src/ameba/rule/style/constant_names.cr @@ -21,7 +21,6 @@ module Ameba::Rule::Style # Style/ConstantNames: # Enabled: true # ``` - # struct ConstantNames < Base properties do description "Enforces constant names to be in screaming case" @@ -34,7 +33,7 @@ module Ameba::Rule::Style name = target.names.first expected = name.upcase - return if expected == name || name.camelcase == name + return if name.in?(expected, name.camelcase) issue_for target, MSG % {expected, name} end diff --git a/src/ameba/rule/style/is_a_nil.cr b/src/ameba/rule/style/is_a_nil.cr index 18653bbc2..cbf4aced3 100644 --- a/src/ameba/rule/style/is_a_nil.cr +++ b/src/ameba/rule/style/is_a_nil.cr @@ -19,7 +19,6 @@ module Ameba::Rule::Style # Style/IsANil: # Enabled: true # ``` - # struct IsANil < Base properties do description "Disallows calls to `is_a?(Nil)` in favor of `nil?`" @@ -32,7 +31,9 @@ module Ameba::Rule::Style return if node.nil_check? const = node.const - issue_for const, MSG if const.is_a?(Crystal::Path) && const.names == PATH_NIL_NAMES + return unless const.is_a?(Crystal::Path) && const.names == PATH_NIL_NAMES + + issue_for const, MSG end end end diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index a9a2b33db..caba735f9 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -26,7 +26,6 @@ module Ameba::Rule::Style # Enabled: true # IntMinDigits: 5 # i.e. integers higher than 9999 # ``` - # struct LargeNumbers < Base properties do description "Disallows usage of large numbers without underscore" @@ -53,7 +52,7 @@ module Ameba::Rule::Style end private def allowed?(_sign, value, fraction, _suffix) - return true if !fraction.nil? && fraction.size > 3 + return true if fraction && fraction.size > 3 digits = value.chars.select &.to_s.=~ /[0-9]/ digits.size >= int_min_digits @@ -71,7 +70,7 @@ module Ameba::Rule::Style value.chars.reject(&.== '_').each_slice(by) do |slice| slices << (yield slice).join end - end.join("_") + end.join('_') end private def parse_number(value) diff --git a/src/ameba/rule/style/method_names.cr b/src/ameba/rule/style/method_names.cr index 905de3735..c53a0f9af 100644 --- a/src/ameba/rule/style/method_names.cr +++ b/src/ameba/rule/style/method_names.cr @@ -37,7 +37,6 @@ module Ameba::Rule::Style # Style/MethodNames: # Enabled: true # ``` - # struct MethodNames < Base properties do description "Enforces method names to be in underscored case" @@ -51,7 +50,7 @@ module Ameba::Rule::Style line_number = node.location.try &.line_number column_number = node.name_location.try &.column_number - return if line_number.nil? || column_number.nil? + return unless line_number && column_number issue_for( {line_number, column_number}, diff --git a/src/ameba/rule/style/negated_conditions_in_unless.cr b/src/ameba/rule/style/negated_conditions_in_unless.cr index 8781b36a9..7225c5df6 100644 --- a/src/ameba/rule/style/negated_conditions_in_unless.cr +++ b/src/ameba/rule/style/negated_conditions_in_unless.cr @@ -26,7 +26,6 @@ module Ameba::Rule::Style # Style/NegatedConditionsInUnless: # Enabled: true # ``` - # struct NegatedConditionsInUnless < Base properties do description "Disallows negated conditions in unless" @@ -35,8 +34,7 @@ module Ameba::Rule::Style MSG = "Avoid negated conditions in unless blocks" def test(source, node : Crystal::Unless) - return unless negated_condition? node.cond - issue_for node, MSG + issue_for node, MSG if negated_condition?(node.cond) end private def negated_condition?(node) diff --git a/src/ameba/rule/style/predicate_name.cr b/src/ameba/rule/style/predicate_name.cr index b23d938c3..a7db3ef65 100644 --- a/src/ameba/rule/style/predicate_name.cr +++ b/src/ameba/rule/style/predicate_name.cr @@ -28,7 +28,6 @@ module Ameba::Rule::Style # Style/PredicateName: # Enabled: true # ``` - # struct PredicateName < Base properties do description "Disallows tautological predicate names" diff --git a/src/ameba/rule/style/redundant_begin.cr b/src/ameba/rule/style/redundant_begin.cr index 1318bb4b2..db6902d8e 100644 --- a/src/ameba/rule/style/redundant_begin.cr +++ b/src/ameba/rule/style/redundant_begin.cr @@ -55,7 +55,6 @@ module Ameba::Rule::Style # Style/RedundantBegin: # Enabled: true # ``` - # struct RedundantBegin < Base include AST::Util properties do @@ -65,9 +64,7 @@ module Ameba::Rule::Style MSG = "Redundant `begin` block detected" def test(source, node : Crystal::Def) - return unless redundant_begin?(source, node) - - issue_for node, MSG + issue_for node, MSG if redundant_begin?(source, node) end private def redundant_begin?(source, node) diff --git a/src/ameba/rule/style/type_names.cr b/src/ameba/rule/style/type_names.cr index 2daa28f84..8cc8bb0d3 100644 --- a/src/ameba/rule/style/type_names.cr +++ b/src/ameba/rule/style/type_names.cr @@ -51,7 +51,6 @@ module Ameba::Rule::Style # Style/TypeNames: # Enabled: true # ``` - # struct TypeNames < Base properties do description "Enforces type names in camelcase manner" @@ -62,7 +61,7 @@ module Ameba::Rule::Style private def check_node(source, node) name = node.name.to_s expected = name.camelcase - return if expected == name + return if name == expected issue_for node, MSG % {expected, name} end diff --git a/src/ameba/rule/style/unless_else.cr b/src/ameba/rule/style/unless_else.cr index 55909cdc7..3583c79c3 100644 --- a/src/ameba/rule/style/unless_else.cr +++ b/src/ameba/rule/style/unless_else.cr @@ -42,7 +42,6 @@ module Ameba::Rule::Style # Style/UnlessElse: # Enabled: true # ``` - # struct UnlessElse < Base properties do description "Disallows the use of an `else` block with the `unless`" @@ -51,8 +50,7 @@ module Ameba::Rule::Style MSG = "Favour if over unless with else" def test(source, node : Crystal::Unless) - return if node.else.nop? - issue_for node, MSG + issue_for node, MSG unless node.else.nop? end end end diff --git a/src/ameba/rule/style/variable_names.cr b/src/ameba/rule/style/variable_names.cr index 108c54d0c..3d0efa34f 100644 --- a/src/ameba/rule/style/variable_names.cr +++ b/src/ameba/rule/style/variable_names.cr @@ -22,7 +22,6 @@ module Ameba::Rule::Style # Style/VariableNames: # Enabled: true # ``` - # struct VariableNames < Base properties do description "Enforces variable names to be in underscored case" diff --git a/src/ameba/rule/style/while_true.cr b/src/ameba/rule/style/while_true.cr index f57289e0a..06e9f1481 100644 --- a/src/ameba/rule/style/while_true.cr +++ b/src/ameba/rule/style/while_true.cr @@ -25,7 +25,6 @@ module Ameba::Rule::Style # Style/WhileTrue: # Enabled: true # ``` - # struct WhileTrue < Base properties do description "Disallows while statements with a true literal as condition" @@ -34,8 +33,7 @@ module Ameba::Rule::Style MSG = "While statement using true literal as condition" def test(source, node : Crystal::While) - return unless node.cond.true_literal? - issue_for node, MSG + issue_for node, MSG if node.cond.true_literal? end end end From 06852c490b7707abafbf28b56e7efe38543e8bb0 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 13:25:50 +0100 Subject: [PATCH 06/37] Followup to #185 --- src/ameba/ast/variabling/assignment.cr | 2 +- src/ameba/ast/variabling/variable.cr | 4 ++-- src/ameba/config.cr | 2 +- src/ameba/formatter/todo_formatter.cr | 2 +- src/ameba/rule/lint/literal_in_condition.cr | 3 +-- src/ameba/rule/lint/percent_array.cr | 7 +++---- src/ameba/rule/lint/rand_zero.cr | 2 +- src/ameba/rule/lint/redundant_string_coercion.cr | 4 +++- src/ameba/rule/lint/shadowed_exception.cr | 7 +++---- src/ameba/rule/metrics/cyclomatic_complexity.cr | 5 ++--- src/ameba/rule/performance/any_after_filter.cr | 8 +++----- src/ameba/rule/performance/first_last_after_filter.cr | 11 +++++------ src/ameba/rule/performance/size_after_filter.cr | 8 +++----- src/ameba/rule/style/constant_names.cr | 2 +- src/ameba/rule/style/is_a_nil.cr | 5 +++-- src/ameba/rule/style/negated_conditions_in_unless.cr | 2 +- 16 files changed, 34 insertions(+), 40 deletions(-) diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index 6908572a2..626e847f3 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/src/ameba/ast/variabling/assignment.cr @@ -49,7 +49,7 @@ module Ameba::AST # a ||= 1 # ``` def op_assign? - node.is_a? Crystal::OpAssign + node.is_a?(Crystal::OpAssign) end # Returns true if this assignment is in a branch, false if not. diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 1a7eab42a..526b417f1 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -91,8 +91,8 @@ module Ameba::AST next if consumed_branches.includes?(assignment.branch) assignment.referenced = true - break unless assignment.branch - consumed_branches << assignment.branch.not_nil! + break unless branch = assignment.branch + consumed_branches << branch end end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 5bf0261ee..c3f7b3390 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -270,7 +270,7 @@ class Ameba::Config include YAML::Serializable::Strict def self.new(config = nil) - if (raw = config.try &.raw).is_a? Hash + if (raw = config.try &.raw).is_a?(Hash) yaml = raw[rule_name]?.try &.to_yaml end from_yaml yaml || "{}" diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index 88584d1d6..473144074 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/src/ameba/formatter/todo_formatter.cr @@ -41,7 +41,7 @@ module Ameba::Formatter private def rule_issues_map(issues) Hash(Rule::Base, Array(Issue)).new.tap do |h| issues.each do |issue| - next if issue.disabled? || issue.rule.is_a? Rule::Lint::Syntax + next if issue.disabled? || issue.rule.is_a?(Rule::Lint::Syntax) (h[issue.rule] ||= Array(Issue).new) << issue end end diff --git a/src/ameba/rule/lint/literal_in_condition.cr b/src/ameba/rule/lint/literal_in_condition.cr index 154785169..642350d6f 100644 --- a/src/ameba/rule/lint/literal_in_condition.cr +++ b/src/ameba/rule/lint/literal_in_condition.cr @@ -30,8 +30,7 @@ module Ameba::Rule::Lint MSG = "Literal value found in conditional" def check_node(source, node) - return unless literal?(node.cond) - issue_for node, MSG + issue_for node, MSG if literal?(node.cond) end def test(source, node : Crystal::If) diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr index b8bb1b547..c8ea02a30 100644 --- a/src/ameba/rule/lint/percent_array.cr +++ b/src/ameba/rule/lint/percent_array.cr @@ -26,8 +26,8 @@ module Ameba::Rule::Lint struct PercentArrays < Base properties do description "Disallows some unwanted symbols in percent array literals" - string_array_unwanted_symbols ",\"" - symbol_array_unwanted_symbols ",:" + string_array_unwanted_symbols %(,") + symbol_array_unwanted_symbols %(,:) end MSG = "Symbols `%s` may be unwanted in %s array literals" @@ -62,8 +62,7 @@ module Ameba::Rule::Lint end private def check_array_entry(entry, symbols, literal) - return unless entry =~ /[#{symbols}]/ - MSG % {symbols, literal} + MSG % {symbols, literal} if entry =~ /[#{symbols}]/ end end end diff --git a/src/ameba/rule/lint/rand_zero.cr b/src/ameba/rule/lint/rand_zero.cr index efd8ab7b6..ad1843caf 100644 --- a/src/ameba/rule/lint/rand_zero.cr +++ b/src/ameba/rule/lint/rand_zero.cr @@ -33,7 +33,7 @@ module Ameba::Rule::Lint return unless node.name == "rand" && node.args.size == 1 && (arg = node.args.first) && - (arg.is_a? Crystal::NumberLiteral) && + arg.is_a?(Crystal::NumberLiteral) && (value = arg.value) && value.in?("0", "1") diff --git a/src/ameba/rule/lint/redundant_string_coercion.cr b/src/ameba/rule/lint/redundant_string_coercion.cr index 4e85de492..f451e9eac 100644 --- a/src/ameba/rule/lint/redundant_string_coercion.cr +++ b/src/ameba/rule/lint/redundant_string_coercion.cr @@ -30,7 +30,9 @@ module Ameba::Rule::Lint MSG = "Redundant use of `Object#to_s` in interpolation" def test(source, node : Crystal::StringInterpolation) - string_coercion_nodes(node).each { |n| issue_for n.name_location, n.end_location, MSG } + string_coercion_nodes(node).each do |n| + issue_for n.name_location, n.end_location, MSG + end end private def string_coercion_nodes(node) diff --git a/src/ameba/rule/lint/shadowed_exception.cr b/src/ameba/rule/lint/shadowed_exception.cr index a937f8bb5..84f2f4356 100644 --- a/src/ameba/rule/lint/shadowed_exception.cr +++ b/src/ameba/rule/lint/shadowed_exception.cr @@ -41,11 +41,10 @@ module Ameba::Rule::Lint MSG = "Exception handler has shadowed exceptions: %s" def test(source, node : Crystal::ExceptionHandler) - return unless excs = node.rescues + return unless excs = node.rescues.try &.map(&.types) + return if (excs = shadowed excs).empty? - if (excs = shadowed excs.map(&.types)).any? - issue_for node, MSG % excs.join(", ") - end + issue_for node, MSG % excs.join(", ") end private def shadowed(exceptions, exception_found = false) diff --git a/src/ameba/rule/metrics/cyclomatic_complexity.cr b/src/ameba/rule/metrics/cyclomatic_complexity.cr index 167ebfe77..9ce2bce4f 100644 --- a/src/ameba/rule/metrics/cyclomatic_complexity.cr +++ b/src/ameba/rule/metrics/cyclomatic_complexity.cr @@ -20,9 +20,8 @@ module Ameba::Rule::Metrics complexity = AST::CountingVisitor.new(node).count if complexity > max_complexity && (location = node.name_location) - issue_for location, def_name_end_location(node), MSG % { - complexity, max_complexity, - } + issue_for location, def_name_end_location(node), + MSG % {complexity, max_complexity} end end diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index b8aa3f342..c6dbc2b72 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -35,12 +35,10 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == ANY_NAME && (obj = node.obj) - return unless obj.is_a?(Crystal::Call) - return if obj.block.nil? || !node.block.nil? + return unless obj.is_a?(Crystal::Call) && obj.block && node.block.nil? + return unless filter_names.includes?(obj.name) - if filter_names.includes?(obj.name) - issue_for obj.name_location, node.name_end_location, MSG % obj.name - end + issue_for obj.name_location, node.name_end_location, MSG % obj.name end end end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index 78f3302a5..ced0a1e3a 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -44,13 +44,12 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless CALL_NAMES.includes?(node.name) && (obj = node.obj) - return unless obj.is_a?(Crystal::Call) - return if obj.block.nil? || !node.block.nil? || node.args.any? + return unless obj.is_a?(Crystal::Call) && obj.block + return if !node.block.nil? || node.args.any? + return unless filter_names.includes?(obj.name) - if filter_names.includes?(obj.name) - message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE - issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name} - end + message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE + issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name} end end end diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index 6a2814326..0339f9989 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -50,12 +50,10 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == SIZE_NAME && (obj = node.obj) - return unless obj.is_a?(Crystal::Call) - return if obj.block.nil? + return unless obj.is_a?(Crystal::Call) && obj.block + return unless filter_names.includes?(obj.name) - if filter_names.includes?(obj.name) - issue_for obj.name_location, node.name_end_location, MSG % obj.name - end + issue_for obj.name_location, node.name_end_location, MSG % obj.name end end end diff --git a/src/ameba/rule/style/constant_names.cr b/src/ameba/rule/style/constant_names.cr index fdd7caada..180ed8be1 100644 --- a/src/ameba/rule/style/constant_names.cr +++ b/src/ameba/rule/style/constant_names.cr @@ -29,7 +29,7 @@ module Ameba::Rule::Style MSG = "Constant name should be screaming-cased: %s, not %s" def test(source, node : Crystal::Assign) - if (target = node.target).is_a? Crystal::Path + if (target = node.target).is_a?(Crystal::Path) name = target.names.first expected = name.upcase diff --git a/src/ameba/rule/style/is_a_nil.cr b/src/ameba/rule/style/is_a_nil.cr index cbf4aced3..4848cd2f9 100644 --- a/src/ameba/rule/style/is_a_nil.cr +++ b/src/ameba/rule/style/is_a_nil.cr @@ -4,7 +4,7 @@ module Ameba::Rule::Style # This is considered bad: # # ``` - # var.is_a? Nil + # var.is_a?(Nil) # ``` # # And needs to be written as: @@ -31,7 +31,8 @@ module Ameba::Rule::Style return if node.nil_check? const = node.const - return unless const.is_a?(Crystal::Path) && const.names == PATH_NIL_NAMES + return unless const.is_a?(Crystal::Path) + return unless const.names == PATH_NIL_NAMES issue_for const, MSG end diff --git a/src/ameba/rule/style/negated_conditions_in_unless.cr b/src/ameba/rule/style/negated_conditions_in_unless.cr index 7225c5df6..2780e228c 100644 --- a/src/ameba/rule/style/negated_conditions_in_unless.cr +++ b/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? { |e| negated_condition?(e) } when Crystal::Not true else From a892cd43b076a9d1332c4c3262867621fa616d53 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 16:29:11 +0100 Subject: [PATCH 07/37] Add Performance/JoinAfterMap rule --- .../rule/performance/join_after_map_spec.cr | 58 +++++++++++++++++++ src/ameba/rule/performance/join_after_map.cr | 48 +++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 spec/ameba/rule/performance/join_after_map_spec.cr create mode 100644 src/ameba/rule/performance/join_after_map.cr diff --git a/spec/ameba/rule/performance/join_after_map_spec.cr b/spec/ameba/rule/performance/join_after_map_spec.cr new file mode 100644 index 000000000..46f5d2501 --- /dev/null +++ b/spec/ameba/rule/performance/join_after_map_spec.cr @@ -0,0 +1,58 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = JoinAfterMap.new + + describe JoinAfterMap do + it "passes if there is no potential performance improvements" do + source = Source.new %( + (1..3).join(&.to_s) + (1..3).join('.', &.to_s) + ) + subject.catch(source).should be_valid + end + + it "reports if there is map followed by join without a block" do + source = Source.new %( + (1..3).map(&.to_s).join + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is map followed by join without a block (with argument)" do + source = Source.new %( + (1..3).map(&.to_s).join('.') + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is map followed by join with a block" do + source = Source.new %( + (1..3).map(&.to_s).join(&.itself) + ) + subject.catch(source).should_not be_valid + end + + context "macro" do + it "doesn't report in macro scope" do + source = Source.new %( + {{ [1, 2, 3].map(&.to_s).join }} + ) + subject.catch(source).should be_valid + end + end + + it "reports rule, pos and message" do + s = Source.new %( + (1..3).map(&.to_s).join + ), "source.cr" + subject.catch(s).should_not be_valid + issue = s.issues.first + + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:8" + issue.end_location.to_s.should eq "source.cr:1:24" + issue.message.should eq "Use `join(separator) {...}` instead of `map {...}.join(separator)`" + end + end +end diff --git a/src/ameba/rule/performance/join_after_map.cr b/src/ameba/rule/performance/join_after_map.cr new file mode 100644 index 000000000..5b374b670 --- /dev/null +++ b/src/ameba/rule/performance/join_after_map.cr @@ -0,0 +1,48 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of `join` calls that follow `map`. + # + # For example, this is considered inefficient: + # + # ``` + # (1..3).map(&.to_s).join('.') + # ``` + # + # And can be written as this: + # + # ``` + # (1..3).join('.', &.to_s) + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/JoinAfterMap + # Enabled: true + # ``` + struct JoinAfterMap < Base + MAP_NAME = "map" + JOIN_NAME = "join" + MSG = "Use `join(separator) {...}` instead of `map {...}.join(separator)`" + + properties do + description "Identifies usage of `join` calls that follow `map`." + end + + def test(source) + AST::NodeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroExpression, + Crystal::MacroIf, + Crystal::MacroFor, + ] + end + + def test(source, node : Crystal::Call) + return unless node.name == JOIN_NAME && (obj = node.obj) + return unless obj.is_a?(Crystal::Call) && obj.block + return unless obj.name == MAP_NAME + + issue_for obj.name_location, node.name_end_location, MSG + end + end +end From 5e58a60ade0bb708f3086f405309b728c3f9deda Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 17:39:32 +0100 Subject: [PATCH 08/37] Move constant definitions after `properties` call --- src/ameba/rule/performance/any_after_filter.cr | 6 +++--- src/ameba/rule/performance/first_last_after_filter.cr | 8 ++++---- src/ameba/rule/performance/join_after_map.cr | 8 ++++---- src/ameba/rule/performance/size_after_filter.cr | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index c6dbc2b72..da9662eef 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -25,14 +25,14 @@ module Ameba::Rule::Performance # - reject # ``` struct AnyAfterFilter < Base - ANY_NAME = "any?" - MSG = "Use `any? {...}` instead of `%s {...}.any?`" - properties do filter_names : Array(String) = %w(select reject) description "Identifies usage of `any?` calls that follow filters." end + ANY_NAME = "any?" + MSG = "Use `any? {...}` instead of `%s {...}.any?`" + def test(source, node : Crystal::Call) return unless node.name == ANY_NAME && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block && node.block.nil? diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index ced0a1e3a..4b251ad8b 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -24,15 +24,15 @@ module Ameba::Rule::Performance # - select # ``` struct FirstLastAfterFilter < Base - CALL_NAMES = %w(first last first? last?) - MSG = "Use `find {...}` instead of `%s {...}.%s`" - MSG_REVERSE = "Use `reverse_each.find {...}` instead of `%s {...}.%s`" - properties do filter_names : Array(String) = %w(select) description "Identifies usage of `first/last/first?/last?` calls that follow filters." end + CALL_NAMES = %w(first last first? last?) + MSG = "Use `find {...}` instead of `%s {...}.%s`" + MSG_REVERSE = "Use `reverse_each.find {...}` instead of `%s {...}.%s`" + def test(source) AST::NodeVisitor.new self, source, skip: [ Crystal::Macro, diff --git a/src/ameba/rule/performance/join_after_map.cr b/src/ameba/rule/performance/join_after_map.cr index 5b374b670..3cd742631 100644 --- a/src/ameba/rule/performance/join_after_map.cr +++ b/src/ameba/rule/performance/join_after_map.cr @@ -20,14 +20,14 @@ module Ameba::Rule::Performance # Enabled: true # ``` struct JoinAfterMap < Base - MAP_NAME = "map" - JOIN_NAME = "join" - MSG = "Use `join(separator) {...}` instead of `map {...}.join(separator)`" - properties do description "Identifies usage of `join` calls that follow `map`." end + MAP_NAME = "map" + JOIN_NAME = "join" + MSG = "Use `join(separator) {...}` instead of `map {...}.join(separator)`" + def test(source) AST::NodeVisitor.new self, source, skip: [ Crystal::Macro, diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index 0339f9989..e825a8f78 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -31,14 +31,14 @@ module Ameba::Rule::Performance # - reject # ``` struct SizeAfterFilter < Base - SIZE_NAME = "size" - MSG = "Use `count {...}` instead of `%s {...}.size`." - properties do filter_names : Array(String) = %w(select reject) description "Identifies usage of `size` calls that follow filter" end + SIZE_NAME = "size" + MSG = "Use `count {...}` instead of `%s {...}.size`." + def test(source) AST::NodeVisitor.new self, source, skip: [ Crystal::Macro, From fd4b2f309cc01b628d20513b2c28ad72029291a8 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 17:49:45 +0100 Subject: [PATCH 09/37] =?UTF-8?q?Don=E2=80=99t=20allocate=20the=20array=20?= =?UTF-8?q?on=20every=20call?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ameba/rule/lint/comparison_to_boolean.cr | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ameba/rule/lint/comparison_to_boolean.cr b/src/ameba/rule/lint/comparison_to_boolean.cr index 649d61a56..474cbc86f 100644 --- a/src/ameba/rule/lint/comparison_to_boolean.cr +++ b/src/ameba/rule/lint/comparison_to_boolean.cr @@ -25,10 +25,11 @@ module Ameba::Rule::Lint description "Disallows comparison to booleans" end - MSG = "Comparison to a boolean is pointless" + MSG = "Comparison to a boolean is pointless" + OP_NAMES = %w(== != ===) def test(source, node : Crystal::Call) - comparison = %w(== != ===).includes?(node.name) + comparison = node.name.in?(OP_NAMES) to_boolean = node.args.first?.try &.is_a?(Crystal::BoolLiteral) || node.obj.is_a?(Crystal::BoolLiteral) From af5d825015e9256c674734550ff766e7f348b8e7 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 18:12:10 +0100 Subject: [PATCH 10/37] Followup to #185 --- src/ameba/ast/variabling/variable.cr | 4 ++-- src/ameba/ast/visitors/flow_expression_visitor.cr | 1 - src/ameba/ast/visitors/node_visitor.cr | 4 ++-- .../ast/visitors/redundant_control_expression_visitor.cr | 2 -- src/ameba/ast/visitors/scope_visitor.cr | 2 -- src/ameba/formatter/dot_formatter.cr | 4 ++-- src/ameba/inline_comments.cr | 2 +- src/ameba/rule/base.cr | 4 ++-- src/ameba/rule/layout/trailing_blank_lines.cr | 6 +++--- src/ameba/rule/lint/comparison_to_boolean.cr | 2 +- src/ameba/rule/lint/empty_expression.cr | 2 +- src/ameba/rule/lint/percent_array.cr | 1 + src/ameba/rule/lint/shadowed_exception.cr | 2 +- src/ameba/rule/performance/any_after_filter.cr | 4 ++-- src/ameba/rule/performance/first_last_after_filter.cr | 6 +++--- src/ameba/rule/performance/size_after_filter.cr | 4 ++-- src/ameba/rule/style/large_numbers.cr | 6 +++--- src/ameba/rule/style/predicate_name.cr | 2 +- src/ameba/rule/style/redundant_begin.cr | 3 +++ src/ameba/rule/style/redundant_next.cr | 1 + src/ameba/rule/style/redundant_return.cr | 1 + 21 files changed, 32 insertions(+), 31 deletions(-) diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 526b417f1..aa9257ac6 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -175,7 +175,7 @@ module Ameba::AST end def references?(node : Crystal::Var) - @macro_literals.any? { |literal| literal.value.includes? node.name } + @macro_literals.any?(&.value.includes?(node.name)) end def visit(node : Crystal::ASTNode) @@ -190,7 +190,7 @@ module Ameba::AST private def update_assign_reference! if @assign_before_reference.nil? && references.size <= assignments.size && - assignments.none? { |ass| ass.op_assign? } + assignments.none?(&.op_assign?) @assign_before_reference = assignments.find { |ass| !ass.in_branch? }.try &.node end end diff --git a/src/ameba/ast/visitors/flow_expression_visitor.cr b/src/ameba/ast/visitors/flow_expression_visitor.cr index f75e0d647..745927050 100644 --- a/src/ameba/ast/visitors/flow_expression_visitor.cr +++ b/src/ameba/ast/visitors/flow_expression_visitor.cr @@ -18,7 +18,6 @@ module Ameba::AST if flow_expression?(node, in_loop?) @rule.test @source, node, FlowExpression.new(node, in_loop?) end - true end diff --git a/src/ameba/ast/visitors/node_visitor.cr b/src/ameba/ast/visitors/node_visitor.cr index 47bb343da..100c2bb49 100644 --- a/src/ameba/ast/visitors/node_visitor.cr +++ b/src/ameba/ast/visitors/node_visitor.cr @@ -40,7 +40,7 @@ module Ameba::AST @skip : Array(Crystal::ASTNode.class)? def initialize(@rule, @source, skip = nil) - @skip = skip.try &.map { |el| el.as(Crystal::ASTNode.class) } + @skip = skip.try &.map(&.as(Crystal::ASTNode.class)) super @rule, @source end @@ -54,7 +54,7 @@ module Ameba::AST {% end %} def visit(node) - return true unless (skip = @skip) + return true unless skip = @skip !skip.includes?(node.class) end end diff --git a/src/ameba/ast/visitors/redundant_control_expression_visitor.cr b/src/ameba/ast/visitors/redundant_control_expression_visitor.cr index a1ef1c3df..100f7f21d 100644 --- a/src/ameba/ast/visitors/redundant_control_expression_visitor.cr +++ b/src/ameba/ast/visitors/redundant_control_expression_visitor.cr @@ -28,8 +28,6 @@ module Ameba::AST when Crystal::Case then traverse_case node when Crystal::BinaryOp then traverse_binary_op node when Crystal::ExceptionHandler then traverse_exception_handler node - else - # ok end end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 084e83aea..619278800 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -7,7 +7,6 @@ module Ameba::AST RECORD_NODE_NAME = "record" @scope_queue = [] of Scope - @current_scope : Scope def initialize(@rule, @source) @@ -169,7 +168,6 @@ module Ameba::AST variable.reference(variable.node, @current_scope).explicit = false end end - true when @current_scope.top_level? && record_macro?(node) false diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr index 54fe53ac9..d0f43f7a2 100644 --- a/src/ameba/formatter/dot_formatter.cr +++ b/src/ameba/formatter/dot_formatter.cr @@ -89,11 +89,11 @@ module Ameba::Formatter private def final_message(sources, failed_sources) total = sources.size - failures = failed_sources.map { |f| f.issues.size }.sum + failures = failed_sources.sum(&.issues.size) color = failures == 0 ? :green : :red s = failures != 1 ? "s" : "" - "#{total} inspected, #{failures} failure#{s}.\n".colorize color + "#{total} inspected, #{failures} failure#{s}.\n".colorize(color) end end end diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr index 7d1ec14d9..ae90083cb 100644 --- a/src/ameba/inline_comments.cr +++ b/src/ameba/inline_comments.cr @@ -38,7 +38,7 @@ module Ameba # ``` # def location_disabled?(location, rule) - return false if Rule::SPECIAL.includes?(rule.name) + return false if rule.name.in?(Rule::SPECIAL) return false unless line_number = location.try &.line_number.try &.- 1 return false unless line = lines[line_number]? diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index de4c363ff..3292b9952 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -107,11 +107,11 @@ module Ameba::Rule # ``` # def special? - SPECIAL.includes? name + name.in?(SPECIAL) end def ==(other) - name == other.try &.name + name == other.try(&.name) end def hash diff --git a/src/ameba/rule/layout/trailing_blank_lines.cr b/src/ameba/rule/layout/trailing_blank_lines.cr index bf2db9d67..7858687fc 100644 --- a/src/ameba/rule/layout/trailing_blank_lines.cr +++ b/src/ameba/rule/layout/trailing_blank_lines.cr @@ -23,9 +23,9 @@ module Ameba::Rule::Layout source_lines_size = source_lines.size return if source_lines_size == 1 && last_source_line.empty? - last_line_not_empty = !last_source_line.empty? - if source_lines_size >= 1 && (source_lines.last(2).join.strip.empty? || last_line_not_empty) - issue_for({source_lines_size - 1, 1}, last_line_not_empty ? MSG_FINAL_NEWLINE : MSG) + last_line_empty = last_source_line.empty? + if source_lines_size >= 1 && (source_lines.last(2).join.blank? || !last_line_empty) + issue_for({source_lines_size - 1, 1}, last_line_empty ? MSG : MSG_FINAL_NEWLINE) end end end diff --git a/src/ameba/rule/lint/comparison_to_boolean.cr b/src/ameba/rule/lint/comparison_to_boolean.cr index 474cbc86f..6ea68b88d 100644 --- a/src/ameba/rule/lint/comparison_to_boolean.cr +++ b/src/ameba/rule/lint/comparison_to_boolean.cr @@ -30,7 +30,7 @@ module Ameba::Rule::Lint def test(source, node : Crystal::Call) comparison = node.name.in?(OP_NAMES) - to_boolean = node.args.first?.try &.is_a?(Crystal::BoolLiteral) || + to_boolean = node.args.first?.try(&.is_a?(Crystal::BoolLiteral)) || node.obj.is_a?(Crystal::BoolLiteral) issue_for node, MSG if comparison && to_boolean diff --git a/src/ameba/rule/lint/empty_expression.cr b/src/ameba/rule/lint/empty_expression.cr index e3fae35ac..52e5b266d 100644 --- a/src/ameba/rule/lint/empty_expression.cr +++ b/src/ameba/rule/lint/empty_expression.cr @@ -31,8 +31,8 @@ module Ameba::Rule::Lint include AST::Util properties do - description "Disallows empty expressions" enabled false + description "Disallows empty expressions" end MSG = "Avoid empty expression %s" diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr index c8ea02a30..ec0bafcfe 100644 --- a/src/ameba/rule/lint/percent_array.cr +++ b/src/ameba/rule/lint/percent_array.cr @@ -26,6 +26,7 @@ module Ameba::Rule::Lint struct PercentArrays < Base properties do description "Disallows some unwanted symbols in percent array literals" + string_array_unwanted_symbols %(,") symbol_array_unwanted_symbols %(,:) end diff --git a/src/ameba/rule/lint/shadowed_exception.cr b/src/ameba/rule/lint/shadowed_exception.cr index 84f2f4356..54a620085 100644 --- a/src/ameba/rule/lint/shadowed_exception.cr +++ b/src/ameba/rule/lint/shadowed_exception.cr @@ -51,7 +51,7 @@ module Ameba::Rule::Lint previous_exceptions = [] of String exceptions.reduce([] of String) do |shadowed, excs| - excs = excs ? excs.map(&.to_s) : ["Exception"] + excs = excs.try(&.map(&.to_s)) || %w[Exception] if exception_found shadowed.concat excs diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index da9662eef..ae1e1c83f 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -26,8 +26,8 @@ module Ameba::Rule::Performance # ``` struct AnyAfterFilter < Base properties do - filter_names : Array(String) = %w(select reject) description "Identifies usage of `any?` calls that follow filters." + filter_names : Array(String) = %w(select reject) end ANY_NAME = "any?" @@ -36,7 +36,7 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == ANY_NAME && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block && node.block.nil? - return unless filter_names.includes?(obj.name) + return unless obj.name.in?(filter_names) issue_for obj.name_location, node.name_end_location, MSG % obj.name end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index 4b251ad8b..79efe463d 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -25,8 +25,8 @@ module Ameba::Rule::Performance # ``` struct FirstLastAfterFilter < Base properties do - filter_names : Array(String) = %w(select) description "Identifies usage of `first/last/first?/last?` calls that follow filters." + filter_names : Array(String) = %w(select) end CALL_NAMES = %w(first last first? last?) @@ -43,10 +43,10 @@ module Ameba::Rule::Performance end def test(source, node : Crystal::Call) - return unless CALL_NAMES.includes?(node.name) && (obj = node.obj) + return unless node.name.in?(CALL_NAMES) && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block return if !node.block.nil? || node.args.any? - return unless filter_names.includes?(obj.name) + return unless obj.name.in?(filter_names) message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name} diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index e825a8f78..0a9238e37 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -32,8 +32,8 @@ module Ameba::Rule::Performance # ``` struct SizeAfterFilter < Base properties do - filter_names : Array(String) = %w(select reject) description "Identifies usage of `size` calls that follow filter" + filter_names : Array(String) = %w(select reject) end SIZE_NAME = "size" @@ -51,7 +51,7 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name == SIZE_NAME && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block - return unless filter_names.includes?(obj.name) + return unless obj.name.in?(filter_names) issue_for obj.name_location, node.name_end_location, MSG % obj.name end diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index caba735f9..02ad38879 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -28,9 +28,9 @@ module Ameba::Rule::Style # ``` struct LargeNumbers < Base properties do + enabled false description "Disallows usage of large numbers without underscore" int_min_digits 5 - enabled false end MSG = "Large numbers should be written with underscores: %s" @@ -82,7 +82,7 @@ module Ameba::Rule::Style end private def parse_sign(value) - if "+-".includes?(value[0]) + if value[0].in?('+', '-') sign = value[0] value = value[1..-1] end @@ -90,7 +90,7 @@ module Ameba::Rule::Style end private def parse_suffix(value) - if pos = (value =~ /e/ || value =~ /_?(i|u|f)/) + if pos = (value =~ /(e|_?(i|u|f))/) suffix = value[pos..-1] value = value[0..pos - 1] end diff --git a/src/ameba/rule/style/predicate_name.cr b/src/ameba/rule/style/predicate_name.cr index a7db3ef65..e453f5ad5 100644 --- a/src/ameba/rule/style/predicate_name.cr +++ b/src/ameba/rule/style/predicate_name.cr @@ -30,8 +30,8 @@ module Ameba::Rule::Style # ``` struct PredicateName < Base properties do - description "Disallows tautological predicate names" enabled false + description "Disallows tautological predicate names" end MSG = "Favour method name '%s?' over '%s'" diff --git a/src/ameba/rule/style/redundant_begin.cr b/src/ameba/rule/style/redundant_begin.cr index db6902d8e..cb6e56525 100644 --- a/src/ameba/rule/style/redundant_begin.cr +++ b/src/ameba/rule/style/redundant_begin.cr @@ -57,6 +57,7 @@ module Ameba::Rule::Style # ``` struct RedundantBegin < Base include AST::Util + properties do description "Disallows redundant begin blocks" end @@ -102,6 +103,7 @@ module Ameba::Rule::Style private def def_redundant_begin?(code) lexer = Crystal::Lexer.new code in_body = in_argument_list = false + loop do token = lexer.next_token @@ -117,6 +119,7 @@ module Ameba::Rule::Style when :NEWLINE in_body = true unless in_argument_list when :SPACE + # ignore else return false if in_body end diff --git a/src/ameba/rule/style/redundant_next.cr b/src/ameba/rule/style/redundant_next.cr index fdf62d435..15920a60a 100644 --- a/src/ameba/rule/style/redundant_next.cr +++ b/src/ameba/rule/style/redundant_next.cr @@ -99,6 +99,7 @@ module Ameba::Rule::Style struct RedundantNext < Base properties do description "Reports redundant next expressions" + allow_multi_next true allow_empty_next true end diff --git a/src/ameba/rule/style/redundant_return.cr b/src/ameba/rule/style/redundant_return.cr index be226bbbd..3c900283e 100644 --- a/src/ameba/rule/style/redundant_return.cr +++ b/src/ameba/rule/style/redundant_return.cr @@ -96,6 +96,7 @@ module Ameba::Rule::Style struct RedundantReturn < Base properties do description "Reports redundant return expressions" + allow_multi_return true allow_empty_return true end From 928a260d5187410234bffd66742cefbf4f45b958 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 18:24:49 +0100 Subject: [PATCH 11/37] Remove extraneous blank lines --- src/ameba/ast/branch.cr | 1 - src/ameba/ast/variabling/assignment.cr | 2 -- src/ameba/ast/variabling/variable.cr | 3 --- src/ameba/ast/visitors/base_visitor.cr | 1 - src/ameba/formatter/explain_formatter.cr | 1 - src/ameba/glob_utils.cr | 2 -- src/ameba/inline_comments.cr | 2 -- src/ameba/rule/base.cr | 5 ----- src/ameba/runner.cr | 4 ---- src/ameba/severity.cr | 1 - src/ameba/source.cr | 1 - src/ameba/tokenizer.cr | 3 --- 12 files changed, 26 deletions(-) diff --git a/src/ameba/ast/branch.cr b/src/ameba/ast/branch.cr index dcca21aa8..647ebafd5 100644 --- a/src/ameba/ast/branch.cr +++ b/src/ameba/ast/branch.cr @@ -44,7 +44,6 @@ module Ameba::AST # end # end # ``` - # def in_loop? @parent.loop? end diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index 626e847f3..e8bf04365 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/src/ameba/ast/variabling/assignment.cr @@ -28,7 +28,6 @@ module Ameba::AST # ``` # Assignment.new(node, variable, scope) # ``` - # def initialize(@node, @variable, @scope) if scope = @variable.scope @branch = Branch.of(@node, scope) @@ -95,7 +94,6 @@ module Ameba::AST # puts(b) # end # ``` - # def transformed? return false unless (assign = node).is_a?(Crystal::Assign) return false unless (value = assign.value).is_a?(Crystal::Call) diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index aa9257ac6..ea741e820 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -27,7 +27,6 @@ module Ameba::AST # ``` # Variable.new(node, scope) # ``` - # def initialize(@node, @scope) end @@ -45,7 +44,6 @@ module Ameba::AST # variable.assign(node2) # variable.assignment.size # => 2 # ``` - # def assign(node, scope) assignments << Assignment.new(node, self, scope) @@ -69,7 +67,6 @@ module Ameba::AST # variable = Variable.new(node, scope) # variable.reference(var_node, some_scope) # ``` - # def reference(node : Crystal::Var, scope : Scope) Reference.new(node, scope).tap do |reference| references << reference diff --git a/src/ameba/ast/visitors/base_visitor.cr b/src/ameba/ast/visitors/base_visitor.cr index 20a2ef5af..297bbe5db 100644 --- a/src/ameba/ast/visitors/base_visitor.cr +++ b/src/ameba/ast/visitors/base_visitor.cr @@ -15,7 +15,6 @@ module Ameba::AST # ``` # visitor = Ameba::AST::NodeVisitor.new(rule, source) # ``` - # def initialize(@rule, @source) @source.ast.accept self end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index 87fb15b6d..4724b087d 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -20,7 +20,6 @@ module Ameba::Formatter # ExplainFormatter.new output, # {file: path, line: line_number, column: column_number} # ``` - # def initialize(@output, loc) @location = Crystal::Location.new(loc[:file], loc[:line], loc[:column]) end diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr index b014bf389..a4a035495 100644 --- a/src/ameba/glob_utils.cr +++ b/src/ameba/glob_utils.cr @@ -7,7 +7,6 @@ module Ameba # ``` # find_files_by_globs(["**/*.cr", "!lib"]) # ``` - # def find_files_by_globs(globs) rejected = rejected_globs(globs) selected = globs - rejected @@ -20,7 +19,6 @@ module Ameba # ``` # expand(["spec/*.cr", "src"]) # => all files in src folder + first level specs # ``` - # def expand(globs) globs.flat_map do |glob| glob += "/**/*.cr" if File.directory?(glob) diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr index ae90083cb..65f70a248 100644 --- a/src/ameba/inline_comments.cr +++ b/src/ameba/inline_comments.cr @@ -36,7 +36,6 @@ module Ameba # Time.epoch(1483859302) # end # ``` - # def location_disabled?(location, rule) return false if rule.name.in?(Rule::SPECIAL) return false unless line_number = location.try &.line_number.try &.- 1 @@ -65,7 +64,6 @@ module Ameba # line = "# # ameba:disable Rule1, Rule2" # parse_inline_directive(line) # => nil # ``` - # def parse_inline_directive(line) if directive = COMMENT_DIRECTIVE_REGEX.match(line) return if commented_out?(line.gsub(directive[0], "")) diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index 3292b9952..e499452f5 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -50,7 +50,6 @@ module Ameba::Rule # source = MyRule.new.catch(source) # source.valid? # ``` - # def catch(source : Source) source.tap { |s| test s } end @@ -65,7 +64,6 @@ module Ameba::Rule # # MyRule.new.name # => "MyRule" # ``` - # def name {{@type}}.rule_name end @@ -79,7 +77,6 @@ module Ameba::Rule # # MyGroup::MyRule.new.group # => "MyGroup" # ``` - # def group {{@type}}.group_name end @@ -91,7 +88,6 @@ module Ameba::Rule # ``` # my_rule.excluded?(source) # => true or false # ``` - # def excluded?(source) excluded.try &.any? do |path| source.matches_path?(path) || @@ -105,7 +101,6 @@ module Ameba::Rule # ``` # my_rule.special? # => true or false # ``` - # def special? name.in?(SPECIAL) end diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index e742dab2c..9e6d29657 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -38,7 +38,6 @@ module Ameba # # Ameba::Runner.new config # ``` - # def initialize(config : Config) @sources = config.sources @formatter = config.formatter @@ -64,7 +63,6 @@ module Ameba # runner = Ameba::Runner.new config # runner.run # => returns runner again # ``` - # def run @formatter.started @sources channels = @sources.map { Channel(Exception?).new } @@ -112,7 +110,6 @@ module Ameba # runner.run # runner.explain({file: file, line: l, column: c}) # ``` - # def explain(location, output = STDOUT) Formatter::ExplainFormatter.new(output, location).finished @sources end @@ -125,7 +122,6 @@ module Ameba # runner.run # runner.success? # => true or false # ``` - # def success? @sources.all? do |source| source.issues diff --git a/src/ameba/severity.cr b/src/ameba/severity.cr index 3b0e4327c..6444a79db 100644 --- a/src/ameba/severity.cr +++ b/src/ameba/severity.cr @@ -23,7 +23,6 @@ module Ameba # Severity.parse("convention") # => Severity::Convention # Severity.parse("foo-bar") # => Exception: Incorrect severity name # ``` - # def self.parse(name : String) super name rescue ArgumentError diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 70d38627a..2e3f2af4f 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -19,7 +19,6 @@ module Ameba # path = "./src/source.cr" # Ameba::Source.new File.read(path), path # ``` - # def initialize(@code, @path = "") end diff --git a/src/ameba/tokenizer.cr b/src/ameba/tokenizer.cr index e79d12cb4..5703a95ab 100644 --- a/src/ameba/tokenizer.cr +++ b/src/ameba/tokenizer.cr @@ -18,7 +18,6 @@ module Ameba # source = Ameba::Source.new code, path # Ameba::Tokenizer.new(source) # ``` - # def initialize(source) @lexer = Crystal::Lexer.new source.code @lexer.count_whitespace = true @@ -33,7 +32,6 @@ module Ameba # lexer = Crystal::Lexer.new(code) # Ameba::Tokenizer.new(lexer) # ``` - # def initialize(@lexer : Crystal::Lexer) end @@ -44,7 +42,6 @@ module Ameba # puts token # end # ``` - # def run(&block : Crystal::Token -> _) run_normal_state @lexer, &block true From 6898aa8976d4bd9e2172876835d1f0621402326e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 17 Jan 2021 21:31:53 +0100 Subject: [PATCH 12/37] Fix issue #187 (#189) * Avoid using `tap` with structs * Remove extraneous blank lines * Readability --- src/ameba.cr | 2 -- src/ameba/ast/util.cr | 1 - src/ameba/cli/cmd.cr | 10 ++++++++-- src/ameba/config.cr | 5 ----- src/ameba/rule/base.cr | 9 +++++---- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/ameba.cr b/src/ameba.cr index 4649b00c1..f16c62e60 100644 --- a/src/ameba.cr +++ b/src/ameba.cr @@ -20,7 +20,6 @@ require "./ameba/formatter/*" # # Ameba.run config # ``` -# module Ameba extend self @@ -35,7 +34,6 @@ module Ameba # Ameba.run # Ameba.run config # ``` - # def run(config = Config.load) Runner.new(config).run end diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 28f55dd0a..0690e31b1 100644 --- a/src/ameba/ast/util.cr +++ b/src/ameba/ast/util.cr @@ -97,7 +97,6 @@ module Ameba::AST::Util # ``` # # That's because not all branches return(i.e. `else` is missing). - # def flow_expression?(node, in_loop = false) return true if flow_command? node, in_loop diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index d32543c45..25d01e76b 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -121,10 +121,16 @@ module Ameba::Cli private def configure_rules(config, opts) case when only = opts.only - config.rules.map! &.tap(&.enabled = false) + config.rules.map! do |rule| + rule.enabled = false + rule + end config.update_rules(only, enabled: true) when opts.all? - config.rules.map! &.tap(&.enabled = true) + config.rules.map! do |rule| + rule.enabled = true + rule + end end config.update_rules(opts.except, enabled: false) end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index c3f7b3390..7661f640a 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -75,7 +75,6 @@ class Ameba::Config # ``` # config = Ameba::Config.load # ``` - # def self.load(path = PATH, colors = true) Colorize.enabled = colors content = File.exists?(path) ? File.read path : "{}" @@ -97,7 +96,6 @@ class Ameba::Config # config.excluded = ["spec"] # config.sources # => list of sources pointing to files found by the wildcards # ``` - # def sources (find_files_by_globs(globs) - find_files_by_globs(excluded)) .map { |path| Source.new File.read(path), path } @@ -122,7 +120,6 @@ class Ameba::Config # config = Ameba::Config.load # config.formatter = :progress # ``` - # def formatter=(name : String | Symbol) if f = AVAILABLE_FORMATTERS[name]? @formatter = f.new @@ -137,7 +134,6 @@ class Ameba::Config # config = Ameba::Config.load # config.update_rule "MyRuleName", enabled: false # ``` - # def update_rule(name, enabled = true, excluded = nil) index = @rules.index { |rule| rule.name == name } raise ArgumentError.new("Rule `#{name}` does not exist") unless index @@ -160,7 +156,6 @@ class Ameba::Config # ``` # config.update_rules %w(Group1 Group2), enabled: true # ``` - # def update_rules(names, **args) names.try &.each do |name| if group = @rule_groups[name]? diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index e499452f5..ea31a2ca9 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -26,7 +26,6 @@ module Ameba::Rule # Enforces rules to implement an abstract `#test` method which # is designed to test the source passed in. If source has issues # that are tested by this rule, it should add an issue. - # abstract struct Base include Config::RuleConfig @@ -51,7 +50,7 @@ module Ameba::Rule # source.valid? # ``` def catch(source : Source) - source.tap { |s| test s } + source.tap { test source } end # Returns a name of this rule, which is basically a class name. @@ -151,8 +150,11 @@ module Ameba::Rule # ``` def self.parsed_doc source = File.read(path_to_source_file) - nodes = Crystal::Parser.new(source).tap(&.wants_doc = true).parse + nodes = Crystal::Parser.new(source) + .tap(&.wants_doc = true) + .parse type_name = rule_name.split('/').last? + DocFinder.new(nodes, type_name).doc end @@ -185,7 +187,6 @@ module Ameba::Rule # ``` # Ameba::Rule.rules # => [Rule1, Rule2, ....] # ``` - # def self.rules Base.subclasses end From e9ec91654c2c5f0ad4a1d11bae61032f9f069bea Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Sun, 17 Jan 2021 17:41:34 +0200 Subject: [PATCH 13/37] New Rule: Lint/DuplicatedRequire closes https://github.com/crystal-ameba/ameba/issues/176 --- .../visitors/top_level_nodes_visitor_spec.cr | 17 +++++++ .../rule/lint/duplicated_require_spec.cr | 48 +++++++++++++++++++ .../ast/visitors/top_level_nodes_visitor.cr | 28 +++++++++++ src/ameba/rule/lint/duplicated_require.cr | 31 ++++++++++++ 4 files changed, 124 insertions(+) create mode 100644 spec/ameba/ast/visitors/top_level_nodes_visitor_spec.cr create mode 100644 spec/ameba/rule/lint/duplicated_require_spec.cr create mode 100644 src/ameba/ast/visitors/top_level_nodes_visitor.cr create mode 100644 src/ameba/rule/lint/duplicated_require.cr diff --git a/spec/ameba/ast/visitors/top_level_nodes_visitor_spec.cr b/spec/ameba/ast/visitors/top_level_nodes_visitor_spec.cr new file mode 100644 index 000000000..0575ff001 --- /dev/null +++ b/spec/ameba/ast/visitors/top_level_nodes_visitor_spec.cr @@ -0,0 +1,17 @@ +require "../../../spec_helper" + +module Ameba::AST + describe TopLevelNodesVisitor do + describe "#require_nodes" do + it "returns require node" do + source = Source.new %( + require "foo" + def bar; end + ) + visitor = TopLevelNodesVisitor.new(source.ast) + visitor.require_nodes.size.should eq 1 + visitor.require_nodes.first.to_s.should eq %q(require "foo") + end + end + end +end diff --git a/spec/ameba/rule/lint/duplicated_require_spec.cr b/spec/ameba/rule/lint/duplicated_require_spec.cr new file mode 100644 index 000000000..94122d0e7 --- /dev/null +++ b/spec/ameba/rule/lint/duplicated_require_spec.cr @@ -0,0 +1,48 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + subject = DuplicatedRequire.new + + describe DuplicatedRequire do + it "passes if there are no duplicated requires" do + source = Source.new %( + require "math" + require "big" + require "big/big_decimal" + ) + subject.catch(source).should be_valid + end + + it "reports if there are a duplicated requires" do + source = Source.new %( + require "big" + require "math" + require "big" + ) + subject.catch(source).should_not be_valid + end + + it "reports rule, pos and message" do + source = Source.new %( + require "./thing" + require "./thing" + require "./another_thing" + require "./another_thing" + ), "source.cr" + + subject.catch(source).should_not be_valid + + issue = source.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:2:1" + issue.end_location.to_s.should eq "" + issue.message.should eq "Duplicated require of `./thing`" + + issue = source.issues.last + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:4:1" + issue.end_location.to_s.should eq "" + issue.message.should eq "Duplicated require of `./another_thing`" + end + end +end diff --git a/src/ameba/ast/visitors/top_level_nodes_visitor.cr b/src/ameba/ast/visitors/top_level_nodes_visitor.cr new file mode 100644 index 000000000..6e606cae8 --- /dev/null +++ b/src/ameba/ast/visitors/top_level_nodes_visitor.cr @@ -0,0 +1,28 @@ +module Ameba::AST + # AST Visitor that visits certain nodes at a top level, which + # can characterize the source (i.e. require statements, modules etc.) + class TopLevelNodesVisitor < Crystal::Visitor + getter require_nodes = [] of Crystal::Require + + # Creates a new instance of visitor + def initialize(@scope : Crystal::ASTNode) + @scope.accept(self) + end + + # :nodoc: + def visit(node : Crystal::Require) + require_nodes << node + end + + # If a top level node is Crystal::Expressions 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. + def visit(node : Crystal::ASTNode) + false + end + end +end diff --git a/src/ameba/rule/lint/duplicated_require.cr b/src/ameba/rule/lint/duplicated_require.cr new file mode 100644 index 000000000..a8b95068e --- /dev/null +++ b/src/ameba/rule/lint/duplicated_require.cr @@ -0,0 +1,31 @@ +module Ameba::Rule::Lint + # A rule that reports duplicated require statements. + # + # ``` + # require "./thing" + # require "./stuff" + # require "./thing" # duplicated require + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/DuplicatedRequire: + # Enabled: true + # ``` + struct DuplicatedRequire < Base + properties do + description "Reports duplicated require statements" + end + + MSG = "Duplicated require of `%s`" + + def test(source) + nodes = AST::TopLevelNodesVisitor.new(source.ast).require_nodes + nodes.each_with_object([] of String) do |node, processed_require_strings| + issue_for(node, MSG % node.string) if processed_require_strings.includes?(node.string) + processed_require_strings << node.string + end + end + end +end From d2fa75280f00f9459b6bd32e402b28a99c0f1ad0 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 18 Jan 2021 16:42:50 +0100 Subject: [PATCH 14/37] Extend JoinAfterMap to check also calls to `sum/product` and rename it to MapInsteadOfBlock (#190) --- ...p_spec.cr => map_instead_of_block_spec.cr} | 9 ++++---- ...n_after_map.cr => map_instead_of_block.cr} | 22 +++++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) rename spec/ameba/rule/performance/{join_after_map_spec.cr => map_instead_of_block_spec.cr} (87%) rename src/ameba/rule/performance/{join_after_map.cr => map_instead_of_block.cr} (54%) diff --git a/spec/ameba/rule/performance/join_after_map_spec.cr b/spec/ameba/rule/performance/map_instead_of_block_spec.cr similarity index 87% rename from spec/ameba/rule/performance/join_after_map_spec.cr rename to spec/ameba/rule/performance/map_instead_of_block_spec.cr index 46f5d2501..6a8c468a7 100644 --- a/spec/ameba/rule/performance/join_after_map_spec.cr +++ b/spec/ameba/rule/performance/map_instead_of_block_spec.cr @@ -1,13 +1,14 @@ require "../../../spec_helper" module Ameba::Rule::Performance - subject = JoinAfterMap.new + subject = MapInsteadOfBlock.new - describe JoinAfterMap do + describe MapInsteadOfBlock do it "passes if there is no potential performance improvements" do source = Source.new %( (1..3).join(&.to_s) - (1..3).join('.', &.to_s) + (1..3).sum(&.*(2)) + (1..3).product(&.*(2)) ) subject.catch(source).should be_valid end @@ -52,7 +53,7 @@ module Ameba::Rule::Performance issue.rule.should_not be_nil issue.location.to_s.should eq "source.cr:1:8" issue.end_location.to_s.should eq "source.cr:1:24" - issue.message.should eq "Use `join(separator) {...}` instead of `map {...}.join(separator)`" + issue.message.should eq "Use `join {...}` instead of `map {...}.join`" end end end diff --git a/src/ameba/rule/performance/join_after_map.cr b/src/ameba/rule/performance/map_instead_of_block.cr similarity index 54% rename from src/ameba/rule/performance/join_after_map.cr rename to src/ameba/rule/performance/map_instead_of_block.cr index 3cd742631..334eda2f9 100644 --- a/src/ameba/rule/performance/join_after_map.cr +++ b/src/ameba/rule/performance/map_instead_of_block.cr @@ -1,32 +1,35 @@ module Ameba::Rule::Performance - # This rule is used to identify usage of `join` calls that follow `map`. + # This rule is used to identify usage of `join/sum/product` calls + # that follow `map`. # # For example, this is considered inefficient: # # ``` # (1..3).map(&.to_s).join('.') + # (1..3).map(&.*(2)).sum # ``` # # And can be written as this: # # ``` # (1..3).join('.', &.to_s) + # (1..3).sum(&.*(2)) # ``` # # YAML configuration example: # # ``` - # Performance/JoinAfterMap + # Performance/MapInsteadOfBlock # Enabled: true # ``` - struct JoinAfterMap < Base + struct MapInsteadOfBlock < Base properties do - description "Identifies usage of `join` calls that follow `map`." + description "Identifies usage of `join/sum/product` calls that follow `map`." end - MAP_NAME = "map" - JOIN_NAME = "join" - MSG = "Use `join(separator) {...}` instead of `map {...}.join(separator)`" + CALL_NAMES = %w(join sum product) + MAP_NAME = "map" + MSG = "Use `%s {...}` instead of `map {...}.%s`" def test(source) AST::NodeVisitor.new self, source, skip: [ @@ -38,11 +41,12 @@ module Ameba::Rule::Performance end def test(source, node : Crystal::Call) - return unless node.name == JOIN_NAME && (obj = node.obj) + return unless node.name.in?(CALL_NAMES) && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block return unless obj.name == MAP_NAME - issue_for obj.name_location, node.name_end_location, MSG + issue_for obj.name_location, node.name_end_location, + MSG % {node.name, node.name} end end end From b7286dc6738d1790fe655254429b63933e11557f Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 18 Jan 2021 18:04:12 +0100 Subject: [PATCH 15/37] Add Performance/CompactAfterMap rule --- .../performance/compact_after_map_spec.cr | 50 +++++++++++++++++++ .../rule/performance/compact_after_map.cr | 48 ++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 spec/ameba/rule/performance/compact_after_map_spec.cr create mode 100644 src/ameba/rule/performance/compact_after_map.cr diff --git a/spec/ameba/rule/performance/compact_after_map_spec.cr b/spec/ameba/rule/performance/compact_after_map_spec.cr new file mode 100644 index 000000000..ad34c79d1 --- /dev/null +++ b/spec/ameba/rule/performance/compact_after_map_spec.cr @@ -0,0 +1,50 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = CompactAfterMap.new + + describe CompactAfterMap do + it "passes if there is no potential performance improvements" do + source = Source.new %( + (1..3).compact_map(&.itself) + ) + subject.catch(source).should be_valid + end + + it "passes if there is map followed by a bang call" do + source = Source.new %( + (1..3).map(&.itself).compact! + ) + subject.catch(source).should be_valid + end + + it "reports if there is map followed by compact call" do + source = Source.new %( + (1..3).map(&.itself).compact + ) + subject.catch(source).should_not be_valid + end + + context "macro" do + it "doesn't report in macro scope" do + source = Source.new %( + {{ [1, 2, 3].map(&.to_s).compact }} + ) + subject.catch(source).should be_valid + end + end + + it "reports rule, pos and message" do + s = Source.new %( + (1..3).map(&.itself).compact + ), "source.cr" + subject.catch(s).should_not be_valid + issue = s.issues.first + + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:8" + issue.end_location.to_s.should eq "source.cr:1:29" + issue.message.should eq "Use `compact_map {...}` instead of `map {...}.compact`" + end + end +end diff --git a/src/ameba/rule/performance/compact_after_map.cr b/src/ameba/rule/performance/compact_after_map.cr new file mode 100644 index 000000000..f45c32060 --- /dev/null +++ b/src/ameba/rule/performance/compact_after_map.cr @@ -0,0 +1,48 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of `compact` calls that follow `map`. + # + # For example, this is considered inefficient: + # + # ``` + # %w[Alice Bob].map(&.match(/^A./)).compact + # ``` + # + # And can be written as this: + # + # ``` + # %w[Alice Bob].compact_map(&.match(/^A./)) + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/CompactAfterMap + # Enabled: true + # ``` + struct CompactAfterMap < Base + properties do + description "Identifies usage of `compact` calls that follow `map`." + end + + COMPACT_NAME = "compact" + MAP_NAME = "map" + MSG = "Use `compact_map {...}` instead of `map {...}.compact`" + + def test(source) + AST::NodeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroExpression, + Crystal::MacroIf, + Crystal::MacroFor, + ] + end + + def test(source, node : Crystal::Call) + return unless node.name == COMPACT_NAME && (obj = node.obj) + return unless obj.is_a?(Crystal::Call) && obj.block + return unless obj.name == MAP_NAME + + issue_for obj.name_location, node.name_end_location, MSG + end + end +end From 23b4b4c4f035f4d3408d0bccc577dfc371a9328e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 18 Jan 2021 18:04:54 +0100 Subject: [PATCH 16/37] Add Performance/FlattenAfterMap rule --- .../performance/flatten_after_map_spec.cr | 43 +++++++++++++++++ .../rule/performance/flatten_after_map.cr | 48 +++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 spec/ameba/rule/performance/flatten_after_map_spec.cr create mode 100644 src/ameba/rule/performance/flatten_after_map.cr diff --git a/spec/ameba/rule/performance/flatten_after_map_spec.cr b/spec/ameba/rule/performance/flatten_after_map_spec.cr new file mode 100644 index 000000000..71666c316 --- /dev/null +++ b/spec/ameba/rule/performance/flatten_after_map_spec.cr @@ -0,0 +1,43 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = FlattenAfterMap.new + + describe FlattenAfterMap do + it "passes if there is no potential performance improvements" do + source = Source.new %( + %w[Alice Bob].flat_map(&.chars) + ) + subject.catch(source).should be_valid + end + + it "reports if there is map followed by flatten call" do + source = Source.new %( + %w[Alice Bob].map(&.chars).flatten + ) + subject.catch(source).should_not be_valid + end + + context "macro" do + it "doesn't report in macro scope" do + source = Source.new %( + {{ %w[Alice Bob].map(&.chars).flatten }} + ) + subject.catch(source).should be_valid + end + end + + it "reports rule, pos and message" do + s = Source.new %( + %w[Alice Bob].map(&.chars).flatten + ), "source.cr" + subject.catch(s).should_not be_valid + issue = s.issues.first + + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:15" + issue.end_location.to_s.should eq "source.cr:1:35" + issue.message.should eq "Use `flat_map {...}` instead of `map {...}.flatten`" + end + end +end diff --git a/src/ameba/rule/performance/flatten_after_map.cr b/src/ameba/rule/performance/flatten_after_map.cr new file mode 100644 index 000000000..c89b77032 --- /dev/null +++ b/src/ameba/rule/performance/flatten_after_map.cr @@ -0,0 +1,48 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of `flatten` calls that follow `map`. + # + # For example, this is considered inefficient: + # + # ``` + # %w[Alice Bob].map(&.chars).flatten + # ``` + # + # And can be written as this: + # + # ``` + # %w[Alice Bob].flat_map(&.chars) + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/FlattenAfterMap + # Enabled: true + # ``` + struct FlattenAfterMap < Base + properties do + description "Identifies usage of `flatten` calls that follow `map`." + end + + FLATTEN_NAME = "flatten" + MAP_NAME = "map" + MSG = "Use `flat_map {...}` instead of `map {...}.flatten`" + + def test(source) + AST::NodeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroExpression, + Crystal::MacroIf, + Crystal::MacroFor, + ] + end + + def test(source, node : Crystal::Call) + return unless node.name == FLATTEN_NAME && (obj = node.obj) + return unless obj.is_a?(Crystal::Call) && obj.block + return unless obj.name == MAP_NAME + + issue_for obj.name_location, node.name_end_location, MSG + end + end +end From ad8f570a047b37577095e924317f67ee6425478a Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 18 Jan 2021 16:45:35 +0100 Subject: [PATCH 17/37] Change Rule to class --- spec/spec_helper.cr | 14 ++++++------ src/ameba/cli/cmd.cr | 10 ++------- src/ameba/config.cr | 22 ++++++++++--------- src/ameba/rule/base.cr | 10 ++++----- src/ameba/rule/layout/line_length.cr | 2 +- src/ameba/rule/layout/trailing_blank_lines.cr | 2 +- src/ameba/rule/layout/trailing_whitespace.cr | 2 +- src/ameba/rule/lint/bad_directive.cr | 2 +- src/ameba/rule/lint/comparison_to_boolean.cr | 2 +- src/ameba/rule/lint/debugger_statement.cr | 2 +- src/ameba/rule/lint/duplicated_require.cr | 2 +- src/ameba/rule/lint/empty_ensure.cr | 2 +- src/ameba/rule/lint/empty_expression.cr | 2 +- src/ameba/rule/lint/empty_loop.cr | 2 +- src/ameba/rule/lint/hash_duplicated_key.cr | 2 +- src/ameba/rule/lint/literal_in_condition.cr | 2 +- .../rule/lint/literal_in_interpolation.cr | 2 +- src/ameba/rule/lint/percent_array.cr | 2 +- src/ameba/rule/lint/rand_zero.cr | 2 +- .../rule/lint/redundant_string_coercion.cr | 2 +- src/ameba/rule/lint/redundant_with_index.cr | 2 +- src/ameba/rule/lint/redundant_with_object.cr | 2 +- src/ameba/rule/lint/shadowed_argument.cr | 2 +- src/ameba/rule/lint/shadowed_exception.cr | 2 +- .../rule/lint/shadowing_local_outer_var.cr | 2 +- src/ameba/rule/lint/shared_var_in_fiber.cr | 2 +- src/ameba/rule/lint/syntax.cr | 2 +- .../rule/lint/unneeded_disable_directive.cr | 2 +- src/ameba/rule/lint/unreachable_code.cr | 2 +- src/ameba/rule/lint/unused_argument.cr | 2 +- src/ameba/rule/lint/useless_assign.cr | 2 +- .../rule/lint/useless_condition_in_when.cr | 2 +- .../rule/metrics/cyclomatic_complexity.cr | 2 +- .../rule/performance/any_after_filter.cr | 2 +- .../rule/performance/compact_after_map.cr | 2 +- .../performance/first_last_after_filter.cr | 2 +- .../rule/performance/flatten_after_map.cr | 2 +- .../rule/performance/map_instead_of_block.cr | 2 +- .../rule/performance/size_after_filter.cr | 2 +- src/ameba/rule/style/constant_names.cr | 2 +- src/ameba/rule/style/is_a_nil.cr | 2 +- src/ameba/rule/style/large_numbers.cr | 2 +- src/ameba/rule/style/method_names.cr | 2 +- .../style/negated_conditions_in_unless.cr | 2 +- src/ameba/rule/style/predicate_name.cr | 2 +- src/ameba/rule/style/redundant_begin.cr | 2 +- src/ameba/rule/style/redundant_next.cr | 2 +- src/ameba/rule/style/redundant_return.cr | 2 +- src/ameba/rule/style/type_names.cr | 2 +- src/ameba/rule/style/unless_else.cr | 2 +- src/ameba/rule/style/variable_names.cr | 2 +- src/ameba/rule/style/while_true.cr | 2 +- 52 files changed, 74 insertions(+), 78 deletions(-) diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index a1739becc..3f106efbb 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -3,7 +3,7 @@ require "../src/ameba" module Ameba # Dummy Rule which does nothing. - struct DummyRule < Rule::Base + class DummyRule < Rule::Base properties do description : String = "Dummy rule that does nothing." end @@ -35,7 +35,7 @@ module Ameba end end - struct NamedRule < Rule::Base + class NamedRule < Rule::Base properties do description "A rule with a custom name." end @@ -45,7 +45,7 @@ module Ameba end end - struct ErrorRule < Rule::Base + class ErrorRule < Rule::Base properties do description "Always adds an error at 1:1" end @@ -55,7 +55,7 @@ module Ameba end end - struct ScopeRule < Rule::Base + class ScopeRule < Rule::Base @[YAML::Field(ignore: true)] getter scopes = [] of AST::Scope @@ -68,7 +68,7 @@ module Ameba end end - struct FlowExpressionRule < Rule::Base + class FlowExpressionRule < Rule::Base @[YAML::Field(ignore: true)] getter expressions = [] of AST::FlowExpression @@ -81,7 +81,7 @@ module Ameba end end - struct RedundantControlExpressionRule < Rule::Base + class RedundantControlExpressionRule < Rule::Base @[YAML::Field(ignore: true)] getter nodes = [] of Crystal::ASTNode @@ -95,7 +95,7 @@ module Ameba end # A rule that always raises an error - struct RaiseRule < Rule::Base + class RaiseRule < Rule::Base property should_raise = false properties do diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 25d01e76b..3f80899eb 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -121,16 +121,10 @@ module Ameba::Cli private def configure_rules(config, opts) case when only = opts.only - config.rules.map! do |rule| - rule.enabled = false - rule - end + config.rules.each(&.enabled = false) config.update_rules(only, enabled: true) when opts.all? - config.rules.map! do |rule| - rule.enabled = true - rule - end + config.rules.each(&.enabled = true) end config.update_rules(opts.except, enabled: false) end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 7661f640a..381f87255 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -135,13 +135,12 @@ class Ameba::Config # config.update_rule "MyRuleName", enabled: false # ``` def update_rule(name, enabled = true, excluded = nil) - index = @rules.index { |rule| rule.name == name } - raise ArgumentError.new("Rule `#{name}` does not exist") unless index + rule = @rules.find(&.name.==(name)) + raise ArgumentError.new("Rule `#{name}` does not exist") unless rule - rule = @rules[index] - rule.enabled = enabled - rule.excluded = excluded - @rules[index] = rule + rule + .tap(&.enabled = enabled) + .tap(&.excluded = excluded) end # Updates rules properties. @@ -156,12 +155,15 @@ class Ameba::Config # ``` # config.update_rules %w(Group1 Group2), enabled: true # ``` - def update_rules(names, **args) + def update_rules(names, enabled = true, excluded = nil) names.try &.each do |name| - if group = @rule_groups[name]? - group.each { |rule| update_rule(rule.name, **args) } + if rules = @rule_groups[name]? + rules.each do |rule| + rule.enabled = enabled + rule.excluded = excluded + end else - update_rule name, **args + update_rule name, enabled, excluded end end end diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index ea31a2ca9..414c469ab 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -10,7 +10,7 @@ module Ameba::Rule # inherits from this struct: # # ``` - # struct MyRule < Ameba::Rule::Base + # class MyRule < Ameba::Rule::Base # def test(source) # if invalid?(source) # issue_for line, column, "Something wrong." @@ -26,7 +26,7 @@ module Ameba::Rule # Enforces rules to implement an abstract `#test` method which # is designed to test the source passed in. If source has issues # that are tested by this rule, it should add an issue. - abstract struct Base + abstract class Base include Config::RuleConfig # This method is designed to test the source passed in. If source has issues @@ -56,7 +56,7 @@ module Ameba::Rule # Returns a name of this rule, which is basically a class name. # # ``` - # struct MyRule < Ameba::Rule::Base + # class MyRule < Ameba::Rule::Base # def test(source) # end # end @@ -70,7 +70,7 @@ module Ameba::Rule # Returns a group this rule belong to. # # ``` - # struct MyGroup::MyRule < Ameba::Rule::Base + # class MyGroup::MyRule < Ameba::Rule::Base # # ... # end # @@ -140,7 +140,7 @@ module Ameba::Rule # module Ameba # # This is a test rule. # # Does nothing. - # struct MyRule < Ameba::Rule::Base + # class MyRule < Ameba::Rule::Base # def test(source) # end # end diff --git a/src/ameba/rule/layout/line_length.cr b/src/ameba/rule/layout/line_length.cr index df8430701..41eb76fa4 100644 --- a/src/ameba/rule/layout/line_length.cr +++ b/src/ameba/rule/layout/line_length.cr @@ -8,7 +8,7 @@ module Ameba::Rule::Layout # Enabled: true # MaxLength: 100 # ``` - struct LineLength < Base + class LineLength < Base properties do enabled false description "Disallows lines longer than `MaxLength` number of symbols" diff --git a/src/ameba/rule/layout/trailing_blank_lines.cr b/src/ameba/rule/layout/trailing_blank_lines.cr index 7858687fc..a133ebb8c 100644 --- a/src/ameba/rule/layout/trailing_blank_lines.cr +++ b/src/ameba/rule/layout/trailing_blank_lines.cr @@ -7,7 +7,7 @@ module Ameba::Rule::Layout # Layout/TrailingBlankLines: # Enabled: true # ``` - struct TrailingBlankLines < Base + class TrailingBlankLines < Base properties do description "Disallows trailing blank lines" end diff --git a/src/ameba/rule/layout/trailing_whitespace.cr b/src/ameba/rule/layout/trailing_whitespace.cr index 77456c1a7..a80a56ec1 100644 --- a/src/ameba/rule/layout/trailing_whitespace.cr +++ b/src/ameba/rule/layout/trailing_whitespace.cr @@ -7,7 +7,7 @@ module Ameba::Rule::Layout # Layout/TrailingWhitespace: # Enabled: true # ``` - struct TrailingWhitespace < Base + class TrailingWhitespace < Base properties do description "Disallows trailing whitespaces" end diff --git a/src/ameba/rule/lint/bad_directive.cr b/src/ameba/rule/lint/bad_directive.cr index 1fcfd896a..3d70c2d50 100644 --- a/src/ameba/rule/lint/bad_directive.cr +++ b/src/ameba/rule/lint/bad_directive.cr @@ -17,7 +17,7 @@ module Ameba::Rule::Lint # Lint/BadDirective: # Enabled: true # ``` - struct BadDirective < Base + class BadDirective < Base properties do description "Reports bad comment directives" end diff --git a/src/ameba/rule/lint/comparison_to_boolean.cr b/src/ameba/rule/lint/comparison_to_boolean.cr index 6ea68b88d..d95d68d70 100644 --- a/src/ameba/rule/lint/comparison_to_boolean.cr +++ b/src/ameba/rule/lint/comparison_to_boolean.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Lint # Lint/ComparisonToBoolean: # Enabled: true # ``` - struct ComparisonToBoolean < Base + class ComparisonToBoolean < Base properties do enabled false description "Disallows comparison to booleans" diff --git a/src/ameba/rule/lint/debugger_statement.cr b/src/ameba/rule/lint/debugger_statement.cr index b5131384c..da478cbb4 100644 --- a/src/ameba/rule/lint/debugger_statement.cr +++ b/src/ameba/rule/lint/debugger_statement.cr @@ -10,7 +10,7 @@ module Ameba::Rule::Lint # Lint/DebuggerStatement: # Enabled: true # ``` - struct DebuggerStatement < Base + class DebuggerStatement < Base properties do description "Disallows calls to debugger" end diff --git a/src/ameba/rule/lint/duplicated_require.cr b/src/ameba/rule/lint/duplicated_require.cr index a8b95068e..0e33dbb03 100644 --- a/src/ameba/rule/lint/duplicated_require.cr +++ b/src/ameba/rule/lint/duplicated_require.cr @@ -13,7 +13,7 @@ module Ameba::Rule::Lint # Lint/DuplicatedRequire: # Enabled: true # ``` - struct DuplicatedRequire < Base + class DuplicatedRequire < Base properties do description "Reports duplicated require statements" end diff --git a/src/ameba/rule/lint/empty_ensure.cr b/src/ameba/rule/lint/empty_ensure.cr index 94deeb369..e363f824b 100644 --- a/src/ameba/rule/lint/empty_ensure.cr +++ b/src/ameba/rule/lint/empty_ensure.cr @@ -38,7 +38,7 @@ module Ameba::Rule::Lint # Lint/EmptyEnsure # Enabled: true # ``` - struct EmptyEnsure < Base + class EmptyEnsure < Base properties do description "Disallows empty ensure statement" end diff --git a/src/ameba/rule/lint/empty_expression.cr b/src/ameba/rule/lint/empty_expression.cr index 52e5b266d..89ca5fdcf 100644 --- a/src/ameba/rule/lint/empty_expression.cr +++ b/src/ameba/rule/lint/empty_expression.cr @@ -27,7 +27,7 @@ module Ameba::Rule::Lint # Lint/EmptyExpression: # Enabled: true # ``` - struct EmptyExpression < Base + class EmptyExpression < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/empty_loop.cr b/src/ameba/rule/lint/empty_loop.cr index 19eafd2fe..b58e23664 100644 --- a/src/ameba/rule/lint/empty_loop.cr +++ b/src/ameba/rule/lint/empty_loop.cr @@ -37,7 +37,7 @@ module Ameba::Rule::Lint # Lint/EmptyLoop: # Enabled: true # ``` - struct EmptyLoop < Base + class EmptyLoop < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/hash_duplicated_key.cr b/src/ameba/rule/lint/hash_duplicated_key.cr index 6a581b4f5..c8ffcb4e6 100644 --- a/src/ameba/rule/lint/hash_duplicated_key.cr +++ b/src/ameba/rule/lint/hash_duplicated_key.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Lint # Lint/HashDuplicatedKey: # Enabled: true # ``` - struct HashDuplicatedKey < Base + class HashDuplicatedKey < Base properties do description "Disallows duplicated keys in hash literals" end diff --git a/src/ameba/rule/lint/literal_in_condition.cr b/src/ameba/rule/lint/literal_in_condition.cr index 642350d6f..fc3ce23a2 100644 --- a/src/ameba/rule/lint/literal_in_condition.cr +++ b/src/ameba/rule/lint/literal_in_condition.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Lint # Lint/LiteralInCondition: # Enabled: true # ``` - struct LiteralInCondition < Base + class LiteralInCondition < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/literal_in_interpolation.cr b/src/ameba/rule/lint/literal_in_interpolation.cr index 6c96c1c87..a8683d128 100644 --- a/src/ameba/rule/lint/literal_in_interpolation.cr +++ b/src/ameba/rule/lint/literal_in_interpolation.cr @@ -15,7 +15,7 @@ module Ameba::Rule::Lint # Lint/LiteralInInterpolation # Enabled: true # ``` - struct LiteralInInterpolation < Base + class LiteralInInterpolation < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr index ec0bafcfe..3109fa151 100644 --- a/src/ameba/rule/lint/percent_array.cr +++ b/src/ameba/rule/lint/percent_array.cr @@ -23,7 +23,7 @@ module Ameba::Rule::Lint # StringArrayUnwantedSymbols: ',"' # SymbolArrayUnwantedSymbols: ',:' # ``` - struct PercentArrays < Base + class PercentArrays < Base properties do description "Disallows some unwanted symbols in percent array literals" diff --git a/src/ameba/rule/lint/rand_zero.cr b/src/ameba/rule/lint/rand_zero.cr index ad1843caf..565450bf0 100644 --- a/src/ameba/rule/lint/rand_zero.cr +++ b/src/ameba/rule/lint/rand_zero.cr @@ -22,7 +22,7 @@ module Ameba::Rule::Lint # Lint/RandZero: # Enabled: true # ``` - struct RandZero < Base + class RandZero < Base properties do description "Disallows rand zero calls" end diff --git a/src/ameba/rule/lint/redundant_string_coercion.cr b/src/ameba/rule/lint/redundant_string_coercion.cr index f451e9eac..0273e6d9b 100644 --- a/src/ameba/rule/lint/redundant_string_coercion.cr +++ b/src/ameba/rule/lint/redundant_string_coercion.cr @@ -20,7 +20,7 @@ module Ameba::Rule::Lint # Lint/RedundantStringCoersion # Enabled: true # ``` - struct RedundantStringCoercion < Base + class RedundantStringCoercion < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/redundant_with_index.cr b/src/ameba/rule/lint/redundant_with_index.cr index 35d3ffc91..6f6229861 100644 --- a/src/ameba/rule/lint/redundant_with_index.cr +++ b/src/ameba/rule/lint/redundant_with_index.cr @@ -26,7 +26,7 @@ module Ameba::Rule::Lint # Lint/RedundantWithIndex: # Enabled: true # ``` - struct RedundantWithIndex < Base + class RedundantWithIndex < Base properties do description "Disallows redundant `with_index` calls" end diff --git a/src/ameba/rule/lint/redundant_with_object.cr b/src/ameba/rule/lint/redundant_with_object.cr index 585e63849..9acf0bf8d 100644 --- a/src/ameba/rule/lint/redundant_with_object.cr +++ b/src/ameba/rule/lint/redundant_with_object.cr @@ -27,7 +27,7 @@ module Ameba::Rule::Lint # Lint/RedundantWithObject: # Enabled: true # ``` - struct RedundantWithObject < Base + class RedundantWithObject < Base properties do description "Disallows redundant `with_object` calls" end diff --git a/src/ameba/rule/lint/shadowed_argument.cr b/src/ameba/rule/lint/shadowed_argument.cr index 444c24e45..c21c1daed 100644 --- a/src/ameba/rule/lint/shadowed_argument.cr +++ b/src/ameba/rule/lint/shadowed_argument.cr @@ -35,7 +35,7 @@ module Ameba::Rule::Lint # Lint/ShadowedArgument: # Enabled: true # ``` - struct ShadowedArgument < Base + class ShadowedArgument < Base properties do description "Disallows shadowed arguments" end diff --git a/src/ameba/rule/lint/shadowed_exception.cr b/src/ameba/rule/lint/shadowed_exception.cr index 54a620085..6e10de9d3 100644 --- a/src/ameba/rule/lint/shadowed_exception.cr +++ b/src/ameba/rule/lint/shadowed_exception.cr @@ -33,7 +33,7 @@ module Ameba::Rule::Lint # Lint/ShadowedException: # Enabled: true # ``` - struct ShadowedException < Base + class ShadowedException < Base properties do description "Disallows rescued exception that get shadowed" end diff --git a/src/ameba/rule/lint/shadowing_local_outer_var.cr b/src/ameba/rule/lint/shadowing_local_outer_var.cr index dbfc4da05..8f70a2858 100644 --- a/src/ameba/rule/lint/shadowing_local_outer_var.cr +++ b/src/ameba/rule/lint/shadowing_local_outer_var.cr @@ -30,7 +30,7 @@ module Ameba::Rule::Lint # Lint/ShadowingOuterLocalVar: # Enabled: true # ``` - struct ShadowingOuterLocalVar < Base + class ShadowingOuterLocalVar < Base properties do description "Disallows the usage of the same name as outer local variables" \ " for block or proc arguments." diff --git a/src/ameba/rule/lint/shared_var_in_fiber.cr b/src/ameba/rule/lint/shared_var_in_fiber.cr index 56cc465ba..2e57c7125 100644 --- a/src/ameba/rule/lint/shared_var_in_fiber.cr +++ b/src/ameba/rule/lint/shared_var_in_fiber.cr @@ -49,7 +49,7 @@ module Ameba::Rule::Lint # Lint/SharedVarInFiber: # Enabled: true # ``` - struct SharedVarInFiber < Base + class SharedVarInFiber < Base properties do description "Disallows shared variables in fibers." end diff --git a/src/ameba/rule/lint/syntax.cr b/src/ameba/rule/lint/syntax.cr index 6c4efa48f..6fbaddda2 100644 --- a/src/ameba/rule/lint/syntax.cr +++ b/src/ameba/rule/lint/syntax.cr @@ -18,7 +18,7 @@ module Ameba::Rule::Lint # rescue e : Exception # end # ``` - struct Syntax < Base + class Syntax < Base properties do description "Reports invalid Crystal syntax" severity Ameba::Severity::Error diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr index 7016f7699..d6d8fc2c2 100644 --- a/src/ameba/rule/lint/unneeded_disable_directive.cr +++ b/src/ameba/rule/lint/unneeded_disable_directive.cr @@ -24,7 +24,7 @@ module Ameba::Rule::Lint # Lint/UnneededDisableDirective # Enabled: true # ``` - struct UnneededDisableDirective < Base + class UnneededDisableDirective < Base properties do description "Reports unneeded disable directives in comments" end diff --git a/src/ameba/rule/lint/unreachable_code.cr b/src/ameba/rule/lint/unreachable_code.cr index 69f3731f2..dc7e34647 100644 --- a/src/ameba/rule/lint/unreachable_code.cr +++ b/src/ameba/rule/lint/unreachable_code.cr @@ -41,7 +41,7 @@ module Ameba::Rule::Lint # Lint/UnreachableCode: # Enabled: true # ``` - struct UnreachableCode < Base + class UnreachableCode < Base include AST::Util properties do diff --git a/src/ameba/rule/lint/unused_argument.cr b/src/ameba/rule/lint/unused_argument.cr index 9615ec4cc..16d860492 100644 --- a/src/ameba/rule/lint/unused_argument.cr +++ b/src/ameba/rule/lint/unused_argument.cr @@ -24,7 +24,7 @@ module Ameba::Rule::Lint # IgnoreBlocks: false # IgnoreProcs: false # ``` - struct UnusedArgument < Base + class UnusedArgument < Base properties do description "Disallows unused arguments" diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index 23f69d4f2..5b06763cd 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/src/ameba/rule/lint/useless_assign.cr @@ -25,7 +25,7 @@ module Ameba::Rule::Lint # Lint/UselessAssign: # Enabled: true # ``` - struct UselessAssign < Base + class UselessAssign < Base properties do description "Disallows useless variable assignments" end diff --git a/src/ameba/rule/lint/useless_condition_in_when.cr b/src/ameba/rule/lint/useless_condition_in_when.cr index f9f40ca65..7cf7784dd 100644 --- a/src/ameba/rule/lint/useless_condition_in_when.cr +++ b/src/ameba/rule/lint/useless_condition_in_when.cr @@ -30,7 +30,7 @@ module Ameba::Rule::Lint # Lint/UselessConditionInWhen: # Enabled: true # ``` - struct UselessConditionInWhen < Base + class UselessConditionInWhen < Base properties do description "Disallows useless conditions in when" end diff --git a/src/ameba/rule/metrics/cyclomatic_complexity.cr b/src/ameba/rule/metrics/cyclomatic_complexity.cr index 9ce2bce4f..1047df670 100644 --- a/src/ameba/rule/metrics/cyclomatic_complexity.cr +++ b/src/ameba/rule/metrics/cyclomatic_complexity.cr @@ -8,7 +8,7 @@ module Ameba::Rule::Metrics # Enabled: true # MaxComplexity: 10 # ``` - struct CyclomaticComplexity < Base + class CyclomaticComplexity < Base properties do description "Disallows methods with a cyclomatic complexity higher than `MaxComplexity`" max_complexity 10 diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index ae1e1c83f..bdc9c1e7f 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -24,7 +24,7 @@ module Ameba::Rule::Performance # - select # - reject # ``` - struct AnyAfterFilter < Base + class AnyAfterFilter < Base properties do description "Identifies usage of `any?` calls that follow filters." filter_names : Array(String) = %w(select reject) diff --git a/src/ameba/rule/performance/compact_after_map.cr b/src/ameba/rule/performance/compact_after_map.cr index f45c32060..77f542cf2 100644 --- a/src/ameba/rule/performance/compact_after_map.cr +++ b/src/ameba/rule/performance/compact_after_map.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Performance # Performance/CompactAfterMap # Enabled: true # ``` - struct CompactAfterMap < Base + class CompactAfterMap < Base properties do description "Identifies usage of `compact` calls that follow `map`." end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index 79efe463d..4333bd6d5 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -23,7 +23,7 @@ module Ameba::Rule::Performance # FilterNames: # - select # ``` - struct FirstLastAfterFilter < Base + class FirstLastAfterFilter < Base properties do description "Identifies usage of `first/last/first?/last?` calls that follow filters." filter_names : Array(String) = %w(select) diff --git a/src/ameba/rule/performance/flatten_after_map.cr b/src/ameba/rule/performance/flatten_after_map.cr index c89b77032..631c656d0 100644 --- a/src/ameba/rule/performance/flatten_after_map.cr +++ b/src/ameba/rule/performance/flatten_after_map.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Performance # Performance/FlattenAfterMap # Enabled: true # ``` - struct FlattenAfterMap < Base + class FlattenAfterMap < Base properties do description "Identifies usage of `flatten` calls that follow `map`." end diff --git a/src/ameba/rule/performance/map_instead_of_block.cr b/src/ameba/rule/performance/map_instead_of_block.cr index 334eda2f9..5e8036173 100644 --- a/src/ameba/rule/performance/map_instead_of_block.cr +++ b/src/ameba/rule/performance/map_instead_of_block.cr @@ -22,7 +22,7 @@ module Ameba::Rule::Performance # Performance/MapInsteadOfBlock # Enabled: true # ``` - struct MapInsteadOfBlock < Base + class MapInsteadOfBlock < Base properties do description "Identifies usage of `join/sum/product` calls that follow `map`." end diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index 0a9238e37..f7a2fbbb3 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -30,7 +30,7 @@ module Ameba::Rule::Performance # - select # - reject # ``` - struct SizeAfterFilter < Base + class SizeAfterFilter < Base properties do description "Identifies usage of `size` calls that follow filter" filter_names : Array(String) = %w(select reject) diff --git a/src/ameba/rule/style/constant_names.cr b/src/ameba/rule/style/constant_names.cr index 180ed8be1..e5182efb7 100644 --- a/src/ameba/rule/style/constant_names.cr +++ b/src/ameba/rule/style/constant_names.cr @@ -21,7 +21,7 @@ module Ameba::Rule::Style # Style/ConstantNames: # Enabled: true # ``` - struct ConstantNames < Base + class ConstantNames < Base properties do description "Enforces constant names to be in screaming case" end diff --git a/src/ameba/rule/style/is_a_nil.cr b/src/ameba/rule/style/is_a_nil.cr index 4848cd2f9..72568b3a4 100644 --- a/src/ameba/rule/style/is_a_nil.cr +++ b/src/ameba/rule/style/is_a_nil.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Style # Style/IsANil: # Enabled: true # ``` - struct IsANil < Base + class IsANil < Base properties do description "Disallows calls to `is_a?(Nil)` in favor of `nil?`" end diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index 02ad38879..e36c4c0fa 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -26,7 +26,7 @@ module Ameba::Rule::Style # Enabled: true # IntMinDigits: 5 # i.e. integers higher than 9999 # ``` - struct LargeNumbers < Base + class LargeNumbers < Base properties do enabled false description "Disallows usage of large numbers without underscore" diff --git a/src/ameba/rule/style/method_names.cr b/src/ameba/rule/style/method_names.cr index c53a0f9af..112ffbd7f 100644 --- a/src/ameba/rule/style/method_names.cr +++ b/src/ameba/rule/style/method_names.cr @@ -37,7 +37,7 @@ module Ameba::Rule::Style # Style/MethodNames: # Enabled: true # ``` - struct MethodNames < Base + class MethodNames < Base properties do description "Enforces method names to be in underscored case" end diff --git a/src/ameba/rule/style/negated_conditions_in_unless.cr b/src/ameba/rule/style/negated_conditions_in_unless.cr index 2780e228c..653a574be 100644 --- a/src/ameba/rule/style/negated_conditions_in_unless.cr +++ b/src/ameba/rule/style/negated_conditions_in_unless.cr @@ -26,7 +26,7 @@ module Ameba::Rule::Style # Style/NegatedConditionsInUnless: # Enabled: true # ``` - struct NegatedConditionsInUnless < Base + class NegatedConditionsInUnless < Base properties do description "Disallows negated conditions in unless" end diff --git a/src/ameba/rule/style/predicate_name.cr b/src/ameba/rule/style/predicate_name.cr index e453f5ad5..24afebc1b 100644 --- a/src/ameba/rule/style/predicate_name.cr +++ b/src/ameba/rule/style/predicate_name.cr @@ -28,7 +28,7 @@ module Ameba::Rule::Style # Style/PredicateName: # Enabled: true # ``` - struct PredicateName < Base + class PredicateName < Base properties do enabled false description "Disallows tautological predicate names" diff --git a/src/ameba/rule/style/redundant_begin.cr b/src/ameba/rule/style/redundant_begin.cr index cb6e56525..3f3009f4b 100644 --- a/src/ameba/rule/style/redundant_begin.cr +++ b/src/ameba/rule/style/redundant_begin.cr @@ -55,7 +55,7 @@ module Ameba::Rule::Style # Style/RedundantBegin: # Enabled: true # ``` - struct RedundantBegin < Base + class RedundantBegin < Base include AST::Util properties do diff --git a/src/ameba/rule/style/redundant_next.cr b/src/ameba/rule/style/redundant_next.cr index 15920a60a..5decb7404 100644 --- a/src/ameba/rule/style/redundant_next.cr +++ b/src/ameba/rule/style/redundant_next.cr @@ -96,7 +96,7 @@ module Ameba::Rule::Style # AllowMultiNext: true # AllowEmptyNext: true # ``` - struct RedundantNext < Base + class RedundantNext < Base properties do description "Reports redundant next expressions" diff --git a/src/ameba/rule/style/redundant_return.cr b/src/ameba/rule/style/redundant_return.cr index 3c900283e..e8ec7dc95 100644 --- a/src/ameba/rule/style/redundant_return.cr +++ b/src/ameba/rule/style/redundant_return.cr @@ -93,7 +93,7 @@ module Ameba::Rule::Style # AllowMutliReturn: true # AllowEmptyReturn: true # ``` - struct RedundantReturn < Base + class RedundantReturn < Base properties do description "Reports redundant return expressions" diff --git a/src/ameba/rule/style/type_names.cr b/src/ameba/rule/style/type_names.cr index 8cc8bb0d3..c05699874 100644 --- a/src/ameba/rule/style/type_names.cr +++ b/src/ameba/rule/style/type_names.cr @@ -51,7 +51,7 @@ module Ameba::Rule::Style # Style/TypeNames: # Enabled: true # ``` - struct TypeNames < Base + class TypeNames < Base properties do description "Enforces type names in camelcase manner" end diff --git a/src/ameba/rule/style/unless_else.cr b/src/ameba/rule/style/unless_else.cr index 3583c79c3..cdb4b966c 100644 --- a/src/ameba/rule/style/unless_else.cr +++ b/src/ameba/rule/style/unless_else.cr @@ -42,7 +42,7 @@ module Ameba::Rule::Style # Style/UnlessElse: # Enabled: true # ``` - struct UnlessElse < Base + class UnlessElse < Base properties do description "Disallows the use of an `else` block with the `unless`" end diff --git a/src/ameba/rule/style/variable_names.cr b/src/ameba/rule/style/variable_names.cr index 3d0efa34f..e8b999c15 100644 --- a/src/ameba/rule/style/variable_names.cr +++ b/src/ameba/rule/style/variable_names.cr @@ -22,7 +22,7 @@ module Ameba::Rule::Style # Style/VariableNames: # Enabled: true # ``` - struct VariableNames < Base + class VariableNames < Base properties do description "Enforces variable names to be in underscored case" end diff --git a/src/ameba/rule/style/while_true.cr b/src/ameba/rule/style/while_true.cr index 06e9f1481..4026c2170 100644 --- a/src/ameba/rule/style/while_true.cr +++ b/src/ameba/rule/style/while_true.cr @@ -25,7 +25,7 @@ module Ameba::Rule::Style # Style/WhileTrue: # Enabled: true # ``` - struct WhileTrue < Base + class WhileTrue < Base properties do description "Disallows while statements with a true literal as condition" end From d3b952f58a73574c7f45604eb4ace773193475e3 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 20 Jan 2021 03:17:00 +0100 Subject: [PATCH 18/37] Add Performance/ChainedCallsWithNoBang rule --- .../chained_calls_with_no_bang_spec.cr | 69 +++++++++++++++++ .../performance/chained_calls_with_no_bang.cr | 76 +++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr create mode 100644 src/ameba/rule/performance/chained_calls_with_no_bang.cr diff --git a/spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr b/spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr new file mode 100644 index 000000000..4259b8661 --- /dev/null +++ b/spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr @@ -0,0 +1,69 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = ChainedCallsWithNoBang.new + + describe ChainedCallsWithNoBang do + it "passes if there is no potential performance improvements" do + source = Source.new %( + (1..3).select { |e| e > 1 }.sort! + (1..3).select { |e| e > 1 }.sort_by!(&.itself) + (1..3).select { |e| e > 1 }.uniq! + (1..3).select { |e| e > 1 }.shuffle! + (1..3).select { |e| e > 1 }.reverse! + (1..3).select { |e| e > 1 }.rotate! + ) + subject.catch(source).should be_valid + end + + it "reports if there is select followed by reverse" do + source = Source.new %( + [1, 2, 3].select { |e| e > 1 }.reverse + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is select followed by reverse followed by other call" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.reverse.size + ) + subject.catch(source).should_not be_valid + end + + context "properties" do + it "allows to configure `call_names`" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.reverse + ) + rule = ChainedCallsWithNoBang.new + rule.call_names = %w(uniq) + rule.catch(source).should be_valid + end + end + + it "reports rule, pos and message" do + source = Source.new path: "source.cr", code: <<-CODE + [1, 2, 3].select { |e| e > 1 }.reverse + CODE + + subject.catch(source).should_not be_valid + source.issues.size.should eq 1 + + issue = source.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:32" + issue.end_location.to_s.should eq "source.cr:1:39" + + issue.message.should eq "Use bang method variant `reverse!` after chained `select` call" + end + + context "macro" do + it "doesn't report in macro scope" do + source = Source.new %( + {{ [1, 2, 3].select { |e| e > 2 }.reverse }} + ) + subject.catch(source).should be_valid + end + end + end +end diff --git a/src/ameba/rule/performance/chained_calls_with_no_bang.cr b/src/ameba/rule/performance/chained_calls_with_no_bang.cr new file mode 100644 index 000000000..8e145f7fe --- /dev/null +++ b/src/ameba/rule/performance/chained_calls_with_no_bang.cr @@ -0,0 +1,76 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of chained calls not utilizing + # the bang method variants. + # + # For example, this is considered inefficient: + # + # ``` + # names = %w[Alice Bob] + # chars = names + # .flat_map(&.chars) + # .uniq + # .sort + # ``` + # + # And can be written as this: + # + # ``` + # names = %w[Alice Bob] + # chars = names + # .flat_map(&.chars) + # .uniq! + # .sort! + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/ChainedCallsWithNoBang + # Enabled: true + # CallNames: + # - uniq + # - sort + # - sort_by + # - shuffle + # - reverse + # - rotate + # ``` + class ChainedCallsWithNoBang < Base + properties do + description "Identifies usage of chained calls not utilizing the bang method variants." + + # All of those have bang method variants returning `self` + # and are not modifying the receiver type (like `compact` does), + # thus are safe to switch to the bang variant. + call_names : Array(String) = %w(uniq sort sort_by shuffle reverse rotate) + end + + # All these methods are allocating a new object + ALLOCATING_METHOD_NAMES = %w( + keys values values_at map map_with_index flat_map compact_map + flatten compact select reject combinations permutations sample + transpose invert group_by chunks tally merge chars clone + captures named_captures + ) + + MSG = "Use bang method variant `%s!` after chained `%s` call" + + def test(source) + AST::NodeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroExpression, + Crystal::MacroIf, + Crystal::MacroFor, + ] + end + + def test(source, node : Crystal::Call) + return unless (obj = node.obj).is_a?(Crystal::Call) + return unless node.name.in?(call_names) + return unless obj.name.in?(call_names) || obj.name.in?(ALLOCATING_METHOD_NAMES) + + issue_for node.name_location, node.name_end_location, + MSG % {node.name, obj.name} + end + end +end From a219a732585d3bd1c6326b94398c3c9fee6985af Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 12:20:50 +0100 Subject: [PATCH 19/37] Remove rotate from the list of call_names In some cases it returns `self` and not a copy. --- src/ameba/rule/performance/chained_calls_with_no_bang.cr | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ameba/rule/performance/chained_calls_with_no_bang.cr b/src/ameba/rule/performance/chained_calls_with_no_bang.cr index 8e145f7fe..68328dfad 100644 --- a/src/ameba/rule/performance/chained_calls_with_no_bang.cr +++ b/src/ameba/rule/performance/chained_calls_with_no_bang.cr @@ -33,7 +33,6 @@ module Ameba::Rule::Performance # - sort_by # - shuffle # - reverse - # - rotate # ``` class ChainedCallsWithNoBang < Base properties do @@ -42,7 +41,7 @@ module Ameba::Rule::Performance # All of those have bang method variants returning `self` # and are not modifying the receiver type (like `compact` does), # thus are safe to switch to the bang variant. - call_names : Array(String) = %w(uniq sort sort_by shuffle reverse rotate) + call_names : Array(String) = %w(uniq sort sort_by shuffle reverse) end # All these methods are allocating a new object From 8babeea80fccfb812023995688d538d5dbd5d4c3 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 12:21:14 +0100 Subject: [PATCH 20/37] Add `repeated_combinations` and `repeated_permutations` --- src/ameba/rule/performance/chained_calls_with_no_bang.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ameba/rule/performance/chained_calls_with_no_bang.cr b/src/ameba/rule/performance/chained_calls_with_no_bang.cr index 68328dfad..4e0169f6e 100644 --- a/src/ameba/rule/performance/chained_calls_with_no_bang.cr +++ b/src/ameba/rule/performance/chained_calls_with_no_bang.cr @@ -47,9 +47,9 @@ module Ameba::Rule::Performance # All these methods are allocating a new object ALLOCATING_METHOD_NAMES = %w( keys values values_at map map_with_index flat_map compact_map - flatten compact select reject combinations permutations sample - transpose invert group_by chunks tally merge chars clone - captures named_captures + flatten compact select reject sample group_by chunks tally merge + combinations repeated_combinations permutations repeated_permutations + transpose invert chars captures named_captures clone ) MSG = "Use bang method variant `%s!` after chained `%s` call" From 6acb8ad2ebd7e22f08571ff3367f122045279f9c Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 17:24:45 +0100 Subject: [PATCH 21/37] ChainedCallsWithNoBang -> ChainedCallWithNoBang --- ...th_no_bang_spec.cr => chained_call_with_no_bang_spec.cr} | 6 +++--- ...d_calls_with_no_bang.cr => chained_call_with_no_bang.cr} | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) rename spec/ameba/rule/performance/{chained_calls_with_no_bang_spec.cr => chained_call_with_no_bang_spec.cr} (94%) rename src/ameba/rule/performance/{chained_calls_with_no_bang.cr => chained_call_with_no_bang.cr} (96%) diff --git a/spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr b/spec/ameba/rule/performance/chained_call_with_no_bang_spec.cr similarity index 94% rename from spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr rename to spec/ameba/rule/performance/chained_call_with_no_bang_spec.cr index 4259b8661..32504ce59 100644 --- a/spec/ameba/rule/performance/chained_calls_with_no_bang_spec.cr +++ b/spec/ameba/rule/performance/chained_call_with_no_bang_spec.cr @@ -1,9 +1,9 @@ require "../../../spec_helper" module Ameba::Rule::Performance - subject = ChainedCallsWithNoBang.new + subject = ChainedCallWithNoBang.new - describe ChainedCallsWithNoBang do + describe ChainedCallWithNoBang do it "passes if there is no potential performance improvements" do source = Source.new %( (1..3).select { |e| e > 1 }.sort! @@ -35,7 +35,7 @@ module Ameba::Rule::Performance source = Source.new %( [1, 2, 3].select { |e| e > 2 }.reverse ) - rule = ChainedCallsWithNoBang.new + rule = ChainedCallWithNoBang.new rule.call_names = %w(uniq) rule.catch(source).should be_valid end diff --git a/src/ameba/rule/performance/chained_calls_with_no_bang.cr b/src/ameba/rule/performance/chained_call_with_no_bang.cr similarity index 96% rename from src/ameba/rule/performance/chained_calls_with_no_bang.cr rename to src/ameba/rule/performance/chained_call_with_no_bang.cr index 4e0169f6e..d7f827ae5 100644 --- a/src/ameba/rule/performance/chained_calls_with_no_bang.cr +++ b/src/ameba/rule/performance/chained_call_with_no_bang.cr @@ -25,7 +25,7 @@ module Ameba::Rule::Performance # YAML configuration example: # # ``` - # Performance/ChainedCallsWithNoBang + # Performance/ChainedCallWithNoBang # Enabled: true # CallNames: # - uniq @@ -34,7 +34,7 @@ module Ameba::Rule::Performance # - shuffle # - reverse # ``` - class ChainedCallsWithNoBang < Base + class ChainedCallWithNoBang < Base properties do description "Identifies usage of chained calls not utilizing the bang method variants." From ea98554191bfd5d64a8c52a4271d9af98f9b1160 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 26 Jan 2021 07:38:19 +0100 Subject: [PATCH 22/37] Add support for showing end location marker (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add support for showing end location marker * Cleanup Reportable method definitions There’s no need for double splats, since they mess up method resolution, and obscure the actual - single (!) - argument - `status`, so… be gone Also, all of the helpers return the constructed `Issue` like a behaving good methods. * Refactor Util#affected_code * Increase max length of trimmed lines to 120 characters * Refactor Issue to use enum instead of a symbol for #status * Optimize Reportable#valid? * Add spec coverage for newly added Util methods * Refactor DotFormatter a bit Make text format moar in line with Crystal spec runner. * Update README.md --- README.md | 10 +- spec/ameba/formatter/dot_formatter_spec.cr | 10 +- spec/ameba/formatter/util_spec.cr | 61 +++++++++++- spec/ameba/issue_spec.cr | 17 +++- src/ameba/formatter/dot_formatter.cr | 23 ++--- src/ameba/formatter/explain_formatter.cr | 29 +++--- src/ameba/formatter/util.cr | 109 ++++++++++++++------- src/ameba/issue.cr | 25 +++-- src/ameba/reportable.cr | 43 ++++---- 9 files changed, 226 insertions(+), 101 deletions(-) diff --git a/README.md b/README.md index b901259a1..fd5d184ea 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@

Code style linter for Crystal

- (a single-celled animal that catches food and moves about by extending fingerlike projections of protoplasm) + (a single-celled animal that catches food and moves about by extending fingerlike projections of protoplasm)

@@ -35,7 +35,7 @@ ## About Ameba is a static code analysis tool for the Crystal language. -It enforces a consistent [Crystal code style](https://crystal-lang.org/docs/conventions/coding_style.html), +It enforces a consistent [Crystal code style](https://crystal-lang.org/reference/conventions/coding_style.html), also catches code smells and wrong code constructions. See also [Roadmap](https://github.com/crystal-ameba/ameba/wiki). @@ -46,7 +46,7 @@ Run `ameba` binary within your project directory to catch code issues: ```sh $ ameba -Inspecting 107 files. +Inspecting 107 files ...............F.....................F.................................................................... @@ -61,9 +61,7 @@ src/ameba/formatter/base_formatter.cr:12:7 ^ Finished in 542.64 milliseconds - -129 inspected, 2 failures. - +129 inspected, 2 failures ``` ### Run in parallel diff --git a/spec/ameba/formatter/dot_formatter_spec.cr b/spec/ameba/formatter/dot_formatter_spec.cr index 444426228..3ba909838 100644 --- a/spec/ameba/formatter/dot_formatter_spec.cr +++ b/spec/ameba/formatter/dot_formatter_spec.cr @@ -8,7 +8,7 @@ module Ameba::Formatter describe "#started" do it "writes started message" do subject.started [Source.new ""] - output.to_s.should eq "Inspecting 1 file.\n\n" + output.to_s.should eq "Inspecting 1 file\n\n" end end @@ -29,7 +29,7 @@ module Ameba::Formatter describe "#finished" do it "writes a final message" do subject.finished [Source.new ""] - output.to_s.should contain "1 inspected, 0 failures." + output.to_s.should contain "1 inspected, 0 failures" end it "writes the elapsed time" do @@ -45,7 +45,7 @@ module Ameba::Formatter end subject.finished [s] log = output.to_s - log.should contain "1 inspected, 2 failures." + log.should contain "1 inspected, 2 failures" log.should contain "DummyRuleError" log.should contain "NamedRuleError" end @@ -60,7 +60,7 @@ module Ameba::Formatter end subject.finished [s] log = output.to_s - log.should contain "> a = 22" + log.should contain "> \e[97ma = 22" log.should contain " \e[33m^\e[0m" end @@ -99,7 +99,7 @@ module Ameba::Formatter s.add_issue(DummyRule.new, location: {1, 1}, message: "DummyRuleError", status: :disabled) subject.finished [s] - output.to_s.should contain "1 inspected, 0 failures." + output.to_s.should contain "1 inspected, 0 failures" end end end diff --git a/spec/ameba/formatter/util_spec.cr b/spec/ameba/formatter/util_spec.cr index 9a002baa7..9087228f5 100644 --- a/spec/ameba/formatter/util_spec.cr +++ b/spec/ameba/formatter/util_spec.cr @@ -8,6 +8,65 @@ module Ameba::Formatter subject = Subject.new describe Util do + describe "#deansify" do + it "returns given string without ANSI codes" do + str = String.build do |io| + io << "foo".colorize.green.underline + io << '-' + io << "bar".colorize.red.underline + end + subject.deansify("foo-bar").should eq "foo-bar" + subject.deansify(str).should eq "foo-bar" + end + end + + describe "#trim" do + it "trims string longer than :max_length" do + subject.trim(("+" * 300), 1).should eq "+" + subject.trim(("+" * 300), 3).should eq "+++" + subject.trim(("+" * 300), 5).should eq "+ ..." + subject.trim(("+" * 300), 7).should eq "+++ ..." + end + + it "leaves intact string shorter than :max_length" do + subject.trim(("+" * 3), 100).should eq "+++" + end + + it "allows to use custom ellipsis" do + subject.trim(("+" * 300), 3, "…").should eq "++…" + end + end + + describe "#context" do + it "returns correct pre/post context lines" do + source = Source.new <<-EOF + # pre:1 + # pre:2 + # pre:3 + # pre:4 + # pre:5 + a = 1 + # post:1 + # post:2 + # post:3 + # post:4 + # post:5 + EOF + + subject.context(source.lines, lineno: 6, context_lines: 3) + .should eq({<<-PRE.lines, <<-POST.lines + # pre:3 + # pre:4 + # pre:5 + PRE + # post:1 + # post:2 + # post:3 + POST + }) + end + end + describe "#affected_code" do it "returns nil if there is no such a line number" do source = Source.new %( @@ -23,7 +82,7 @@ module Ameba::Formatter ) location = Crystal::Location.new("filename", 1, 1) subject.deansify(subject.affected_code(source, location)) - .should eq "> a = 1\n ^" + .should eq "> a = 1\n ^\n" end it "returns correct line if it is found" do diff --git a/spec/ameba/issue_spec.cr b/spec/ameba/issue_spec.cr index 652f52473..b6488552e 100644 --- a/spec/ameba/issue_spec.cr +++ b/spec/ameba/issue_spec.cr @@ -42,9 +42,22 @@ module Ameba location: nil, end_location: nil, message: "", - status: :enabled + status: :disabled - issue.status.should eq :enabled + issue.status.should eq Issue::Status::Disabled + issue.disabled?.should be_true + issue.enabled?.should be_false + end + + it "sets status to :enabled by default" do + issue = Issue.new rule: DummyRule.new, + location: nil, + end_location: nil, + message: "" + + issue.status.should eq Issue::Status::Enabled + issue.enabled?.should be_true + issue.disabled?.should be_false end end end diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr index d0f43f7a2..121ea1cf3 100644 --- a/src/ameba/formatter/dot_formatter.cr +++ b/src/ameba/formatter/dot_formatter.cr @@ -6,14 +6,15 @@ module Ameba::Formatter class DotFormatter < BaseFormatter include Util - @started_at : Time? + @started_at : Time::Span? @mutex = Thread::Mutex.new # Reports a message when inspection is started. def started(sources) - @started_at = Time.utc # Time.monotonic + @started_at = Time.monotonic - output << started_message(sources.size) + output.puts started_message(sources.size) + output.puts end # Reports a result of the inspection of a corresponding source. @@ -41,29 +42,29 @@ module Ameba::Formatter "#{issue.rule.name}: " \ "#{issue.message}".colorize(:red) - if show_affected_code && (code = affected_code(source, location)) + if show_affected_code && (code = affected_code(source, location, issue.end_location)) output << code.colorize(:default) end - output << '\n' + output.puts end end - output << finished_in_message(@started_at, Time.utc) # Time.monotonic - output << final_message(sources, failed_sources) + output.puts finished_in_message(@started_at, Time.monotonic) + output.puts final_message(sources, failed_sources) end private def started_message(size) if size == 1 - "Inspecting 1 file.\n\n".colorize(:default) + "Inspecting 1 file".colorize(:default) else - "Inspecting #{size} files.\n\n".colorize(:default) + "Inspecting #{size} files".colorize(:default) end end private def finished_in_message(started, finished) if started && finished - "Finished in #{to_human(finished - started)} \n\n".colorize(:default) + "Finished in #{to_human(finished - started)}".colorize(:default) end end @@ -93,7 +94,7 @@ module Ameba::Formatter color = failures == 0 ? :green : :red s = failures != 1 ? "s" : "" - "#{total} inspected, #{failures} failure#{s}.\n".colorize(color) + "#{total} inspected, #{failures} failure#{s}".colorize(color) end end end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index 4724b087d..f8fe87c15 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -20,35 +20,36 @@ module Ameba::Formatter # ExplainFormatter.new output, # {file: path, line: line_number, column: column_number} # ``` - def initialize(@output, loc) - @location = Crystal::Location.new(loc[:file], loc[:line], loc[:column]) + def initialize(@output, location) + @location = Crystal::Location.new(location[:file], location[:line], location[:column]) end # Reports the explainations at the *@location*. def finished(sources) - source = sources.find { |s| s.path == @location.filename } - + source = sources.find(&.path.==(@location.filename)) return unless source - source.issues.each do |issue| - if (location = issue.location) && - location.line_number == @location.line_number && - location.column_number == @location.column_number - explain(source, issue) - end - end + issue = source.issues.find(&.location.==(@location)) + return unless issue + + explain(source, issue) end private def explain(source, issue) rule = issue.rule + location, end_location = + issue.location, issue.end_location + + return unless location + output_title "ISSUE INFO" output_paragraph [ issue.message.colorize(:red).to_s, - @location.to_s.colorize(:cyan).to_s, + location.to_s.colorize(:cyan).to_s, ] - if affected_code = affected_code(source, @location, context_lines: 3) + if affected_code = affected_code(source, location, end_location, context_lines: 3) output_title "AFFECTED CODE" output_paragraph affected_code end @@ -68,7 +69,7 @@ module Ameba::Formatter end private def output_paragraph(paragraph : String) - output_paragraph(paragraph.split('\n')) + output_paragraph(paragraph.lines) end private def output_paragraph(paragraph : Array(String)) diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr index 822489f06..d6c411c86 100644 --- a/src/ameba/formatter/util.cr +++ b/src/ameba/formatter/util.cr @@ -4,70 +4,105 @@ module Ameba::Formatter message.try &.gsub(/\x1b[^m]*m/, "").presence end - def affected_code(source, location, context_lines = 0, max_length = 100, placeholder = " ...", prompt = "> ") + def trim(str, max_length = 120, ellipsis = " ...") + if (str.size - ellipsis.size) > max_length + str = str[0, max_length] + if str.size > ellipsis.size + str = str[0...-ellipsis.size] + ellipsis + end + end + str + end + + def context(lines, lineno, context_lines = 3, remove_empty = true) + pre_context, post_context = %w[], %w[] + + lines.each_with_index do |line, i| + case i + 1 + when lineno - context_lines...lineno + pre_context << line + when lineno + 1..lineno + context_lines + post_context << line + end + end + + if remove_empty + # remove empty lines at the beginning ... + while pre_context.first?.try(&.blank?) + pre_context.shift + end + # ... and the end + while post_context.last?.try(&.blank?) + post_context.pop + end + end + + {pre_context, post_context} + end + + def affected_code(source, location, end_location = nil, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ") lines = source.lines lineno, column = location.line_number, location.column_number return unless affected_line = lines[lineno - 1]?.presence - trim_line = Proc(String, String).new do |line| - if line.size > max_length - line = line[0, max_length - placeholder.size - 1] + placeholder - end - line - end - if column < max_length - affected_line = trim_line.call(affected_line) + affected_line = trim(affected_line, max_length, ellipsis) end show_context = context_lines > 0 + if show_context - pre_context, post_context = %w[], %w[] - - lines.each_with_index do |line, i| - case i + 1 - when lineno - context_lines...lineno - pre_context << trim_line.call(line) - when lineno - # - when lineno + 1..lineno + context_lines - post_context << trim_line.call(line) - end - end + pre_context, post_context = + context(lines, lineno, context_lines) - # remove empty lines at the beginning/end - pre_context.shift? unless pre_context.first?.presence - post_context.pop? unless post_context.last?.presence + position = prompt.size + column + position -= 1 + else + affected_line_size, affected_line = + affected_line.size, affected_line.lstrip + + position = column - (affected_line_size - affected_line.size) + prompt.size + position -= 1 end String.build do |str| if show_context pre_context.try &.each do |line| + line = trim(line, max_length, ellipsis) str << prompt str.puts(line.colorize(:dark_gray)) end + end + + str << prompt + str.puts(affected_line.colorize(:white)) + + str << (" " * position) + str << "^".colorize(:yellow) - str << prompt - str.puts(affected_line.colorize(:white)) + if end_location + end_lineno = end_location.line_number + end_column = end_location.column_number - str << " " * (prompt.size + column - 1) - str.puts("^".colorize(:yellow)) + if end_lineno == lineno && end_column > column + end_position = end_column - column + end_position -= 1 + str << ("-" * end_position).colorize(:dark_gray) + str << "^".colorize(:yellow) + end + end + + str.puts + + if show_context post_context.try &.each do |line| + line = trim(line, max_length, ellipsis) str << prompt str.puts(line.colorize(:dark_gray)) end - else - stripped = affected_line.lstrip - position = column - (affected_line.size - stripped.size) + prompt.size - - str << prompt - str.puts(stripped) - - str << " " * (position - 1) - str << "^".colorize(:yellow) end end end diff --git a/src/ameba/issue.cr b/src/ameba/issue.cr index 03e95165b..b913eac3b 100644 --- a/src/ameba/issue.cr +++ b/src/ameba/issue.cr @@ -1,22 +1,31 @@ module Ameba # Represents an issue reported by Ameba. - record Issue, + struct Issue + enum Status + Enabled + Disabled + end + # A rule that triggers this issue. - rule : Rule::Base, + getter rule : Rule::Base # Location of the issue. - location : Crystal::Location?, + getter location : Crystal::Location? # End location of the issue. - end_location : Crystal::Location?, + getter end_location : Crystal::Location? # Issue message. - message : String, + getter message : String # Issue status. - status : Symbol? do - def disabled? - status == :disabled + getter status : Status + + delegate :enabled?, :disabled?, + to: status + + def initialize(@rule, @location, @end_location, @message, status : Status? = nil) + @status = status || Status::Enabled end def syntax? diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr index e31ef3a74..4632fa915 100644 --- a/src/ameba/reportable.cr +++ b/src/ameba/reportable.cr @@ -5,37 +5,46 @@ module Ameba getter issues = [] of Issue # Adds a new issue to the list of issues. - def add_issue(rule, location : Crystal::Location?, end_location : Crystal::Location?, message, status = nil) - status ||= :disabled if location_disabled?(location, rule) - issues << Issue.new rule, location, end_location, message, status + def add_issue(rule, location, end_location, message, status : Issue::Status? = nil) : Issue + status ||= + Issue::Status::Disabled if location_disabled?(location, rule) + + Issue.new(rule, location, end_location, message, status).tap do |issue| + issues << issue + end end - # Adds a new issue for AST *node*. - def add_issue(rule, node : Crystal::ASTNode, message, **args) - add_issue rule, node.location, node.end_location, message, **args + # Adds a new issue for Crystal AST *node*. + def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil) : Issue + add_issue rule, node.location, node.end_location, message, status end # Adds a new issue for Crystal *token*. - def add_issue(rule, token : Crystal::Token, message, **args) - add_issue rule, token.location, nil, message, **args + def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil) : Issue + add_issue rule, token.location, nil, message, status end # Adds a new issue for *location* defined by line and column numbers. - def add_issue(rule, location : Tuple(Int32, Int32), message, **args) - location = Crystal::Location.new path, *location - add_issue rule, location, nil, message, **args + def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue + location = + Crystal::Location.new(path, *location) + + add_issue rule, location, nil, message, status end # Adds a new issue for *location* and *end_location* defined by line and column numbers. - def add_issue(rule, location : Tuple(Int32, Int32), end_location : Tuple(Int32, Int32), message, **args) - location = Crystal::Location.new path, *location - end_location = Crystal::Location.new path, *end_location - add_issue rule, location, end_location, message, **args + def add_issue(rule, location : {Int32, Int32}, end_location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue + location = + Crystal::Location.new(path, *location) + end_location = + Crystal::Location.new(path, *end_location) + + add_issue rule, location, end_location, message, status end - # Returns true if the list of not disabled issues is empty, false otherwise. + # Returns `true` if the list of not disabled issues is empty, `false` otherwise. def valid? - issues.reject(&.disabled?).empty? + issues.none?(&.enabled?) end end end From 95d340c3ad9aa44b79a7ce3377718f2b25fc95db Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 24 Jan 2021 20:23:32 +0100 Subject: [PATCH 23/37] Add Style/IsAFilter rule --- spec/ameba/rule/style/is_a_filter_spec.cr | 64 ++++++++++++++++++++ src/ameba/rule/style/is_a_filter.cr | 74 +++++++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 spec/ameba/rule/style/is_a_filter_spec.cr create mode 100644 src/ameba/rule/style/is_a_filter.cr diff --git a/spec/ameba/rule/style/is_a_filter_spec.cr b/spec/ameba/rule/style/is_a_filter_spec.cr new file mode 100644 index 000000000..1a49c2d17 --- /dev/null +++ b/spec/ameba/rule/style/is_a_filter_spec.cr @@ -0,0 +1,64 @@ +require "../../../spec_helper" + +module Ameba::Rule::Style + subject = IsAFilter.new + + describe IsAFilter do + it "passes if there is no potential performance improvements" do + source = Source.new %( + [1, 2, nil].select(Int32) + [1, 2, nil].reject(Nil) + ) + subject.catch(source).should be_valid + end + + it "reports if there is .is_a? call within select" do + source = Source.new %( + [1, 2, nil].select(&.is_a?(Int32)) + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is .nil? call within reject" do + source = Source.new %( + [1, 2, nil].reject(&.nil?) + ) + subject.catch(source).should_not be_valid + end + + context "properties" do + it "allows to configure filter_names" do + source = Source.new %( + [1, 2, nil].reject(&.nil?) + ) + rule = IsAFilter.new + rule.filter_names = %w(select) + rule.catch(source).should be_valid + end + end + + context "macro" do + it "reports in macro scope" do + source = Source.new %( + {{ [1, 2, nil].reject(&.nil?) }} + ) + subject.catch(source).should_not be_valid + end + end + + it "reports rule, pos and message" do + source = Source.new path: "source.cr", code: %( + [1, 2, nil].reject(&.nil?) + ) + subject.catch(source).should_not be_valid + source.issues.size.should eq 1 + + issue = source.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:13" + issue.end_location.to_s.should eq "source.cr:1:26" + + issue.message.should eq "Use `reject(Nil)` instead of `reject {...}`" + end + end +end diff --git a/src/ameba/rule/style/is_a_filter.cr b/src/ameba/rule/style/is_a_filter.cr new file mode 100644 index 000000000..fedeedeca --- /dev/null +++ b/src/ameba/rule/style/is_a_filter.cr @@ -0,0 +1,74 @@ +module Ameba::Rule::Style + # This rule is used to identify usage of `is_a?/nil?` calls within filters. + # + # For example, this is considered invalid: + # + # ``` + # matches = %w[Alice Bob].map(&.match(/^A./)) + # + # matches.any?(&.is_a?(Regex::MatchData)) # => true + # matches.one?(&.nil?) # => true + # + # typeof(matches.reject(&.nil?)) # => Array(Regex::MatchData | Nil) + # typeof(matches.select(&.is_a?(Regex::MatchData))) # => Array(Regex::MatchData | Nil) + # ``` + # + # And it should be written as this: + # + # ``` + # matches = %w[Alice Bob].map(&.match(/^A./)) + # + # matches.any?(Regex::MatchData) # => true + # matches.one?(Nil) # => true + # + # typeof(matches.reject(Nil)) # => Array(Regex::MatchData) + # typeof(matches.select(Regex::MatchData)) # => Array(Regex::MatchData) + # ``` + # + # YAML configuration example: + # + # ``` + # Style/IsAFilter: + # Enabled: true + # FilterNames: + # - select + # - reject + # - any? + # - all? + # - none? + # - one? + # ``` + class IsAFilter < Base + properties do + description "Identifies usage of `is_a?/nil?` calls within filters." + filter_names : Array(String) = %w(select reject any? all? none? one?) + end + + MSG = "Use `%s(%s)` instead of `%s {...}`" + + def test(source, node : Crystal::Call) + return unless node.name.in?(filter_names) + return unless (block = node.block) + return unless (body = block.body).is_a?(Crystal::IsA) + return unless (path = body.const).is_a?(Crystal::Path) + return unless body.obj.is_a?(Crystal::Var) + + name = path.names.join("::") + name = "::#{name}" if path.global? && !body.nil_check? + + end_location = node.end_location + if !end_location || end_location.try(&.column_number.zero?) + if end_location = path.end_location + end_location = Crystal::Location.new( + end_location.filename, + end_location.line_number, + end_location.column_number + 1 + ) + end + end + + issue_for node.name_location, end_location, + MSG % {node.name, name, node.name} + end + end +end From d71091a40c1d652cfc1ed22b37b539d0ff1e1065 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 02:31:18 +0100 Subject: [PATCH 24/37] Add Performance/AnyInsteadOfEmpty rule --- .../performance/any_instead_of_empty_spec.cr | 46 +++++++++++++++++++ .../rule/performance/any_instead_of_empty.cr | 44 ++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 spec/ameba/rule/performance/any_instead_of_empty_spec.cr create mode 100644 src/ameba/rule/performance/any_instead_of_empty.cr diff --git a/spec/ameba/rule/performance/any_instead_of_empty_spec.cr b/spec/ameba/rule/performance/any_instead_of_empty_spec.cr new file mode 100644 index 000000000..2a5f82276 --- /dev/null +++ b/spec/ameba/rule/performance/any_instead_of_empty_spec.cr @@ -0,0 +1,46 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = AnyInsteadOfEmpty.new + + describe AnyInsteadOfEmpty do + it "passes if there is no potential performance improvements" do + source = Source.new %( + [1, 2, 3].any?(&.zero?) + [1, 2, 3].any?(String) + [1, 2, 3].any?(1..3) + [1, 2, 3].any? { |e| e > 1 } + ) + subject.catch(source).should be_valid + end + + it "reports if there is any? call without a block nor argument" do + source = Source.new %( + [1, 2, 3].any? + ) + subject.catch(source).should_not be_valid + end + + context "macro" do + it "reports in macro scope" do + source = Source.new %( + {{ [1, 2, 3].any? }} + ) + subject.catch(source).should_not be_valid + end + end + + it "reports rule, pos and message" do + source = Source.new path: "source.cr", code: %( + [1, 2, 3].any? + ) + subject.catch(source).should_not be_valid + issue = source.issues.first + + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:11" + issue.end_location.to_s.should eq "source.cr:1:15" + issue.message.should eq "Use `!{...}.empty?` instead of `{...}.any?`" + end + end +end diff --git a/src/ameba/rule/performance/any_instead_of_empty.cr b/src/ameba/rule/performance/any_instead_of_empty.cr new file mode 100644 index 000000000..6dd7a2bad --- /dev/null +++ b/src/ameba/rule/performance/any_instead_of_empty.cr @@ -0,0 +1,44 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of arg-less `Enumerable#any?` calls. + # + # Using `Enumerable#any?` instead of `Enumerable#empty?` might lead to an + # unexpected results (like `[nil, false].any? # => false`). In some cases + # it also might be less efficient, since it iterates until the block will + # return a _truthy_ value, instead of just checking if there's at least + # one value present. + # + # For example, this is considered invalid: + # + # ``` + # [1, 2, 3].any? + # ``` + # + # And it should be written as this: + # + # ``` + # ![1, 2, 3].empty? + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/AnyInsteadOfEmpty: + # Enabled: true + # ``` + class AnyInsteadOfEmpty < Base + properties do + description "Identifies usage of arg-less `any?` calls." + end + + ANY_NAME = "any?" + MSG = "Use `!{...}.empty?` instead of `{...}.any?`" + + def test(source, node : Crystal::Call) + return unless node.name == ANY_NAME + return unless node.block.nil? && node.args.empty? + return unless node.obj + + issue_for node.name_location, node.name_end_location, MSG + end + end +end From 7b3c814914052ed023832cdb464757607b7891cb Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 02:37:11 +0100 Subject: [PATCH 25/37] Fix newly found issues --- spec/ameba/ast/scope_spec.cr | 2 +- spec/ameba/ast/variabling/variable_spec.cr | 4 ++-- src/ameba/ast/variabling/variable.cr | 2 +- src/ameba/ast/visitors/flow_expression_visitor.cr | 2 +- src/ameba/cli/cmd.cr | 2 +- src/ameba/rule/lint/unneeded_disable_directive.cr | 2 +- src/ameba/rule/performance/first_last_after_filter.cr | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr index 72951b14d..00f3061cb 100644 --- a/spec/ameba/ast/scope_spec.cr +++ b/spec/ameba/ast/scope_spec.cr @@ -51,7 +51,7 @@ module Ameba::AST it "adds a new variable to the scope" do scope = Scope.new as_node("") scope.add_variable(Crystal::Var.new "foo") - scope.variables.any?.should be_true + scope.variables.empty?.should be_false end end diff --git a/spec/ameba/ast/variabling/variable_spec.cr b/spec/ameba/ast/variabling/variable_spec.cr index fc39ef0d1..66b1a1882 100644 --- a/spec/ameba/ast/variabling/variable_spec.cr +++ b/spec/ameba/ast/variabling/variable_spec.cr @@ -48,7 +48,7 @@ module Ameba::AST it "assigns the variable (creates a new assignment)" do variable = Variable.new(var_node, scope) variable.assign(assign_node, scope) - variable.assignments.any?.should be_true + variable.assignments.empty?.should be_false end it "can create multiple assignments" do @@ -64,7 +64,7 @@ module Ameba::AST variable = Variable.new(var_node, scope) variable.assign(as_node("foo=1"), scope) variable.reference(var_node, scope) - variable.references.any?.should be_true + variable.references.empty?.should be_false end it "adds a reference to the scope" do diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index ea741e820..b3dab3056 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -58,7 +58,7 @@ module Ameba::AST # variable.referenced? # => true # ``` def referenced? - references.any? + !references.empty? end # Creates a reference to this variable in some scope. diff --git a/src/ameba/ast/visitors/flow_expression_visitor.cr b/src/ameba/ast/visitors/flow_expression_visitor.cr index 745927050..69ec1e25f 100644 --- a/src/ameba/ast/visitors/flow_expression_visitor.cr +++ b/src/ameba/ast/visitors/flow_expression_visitor.cr @@ -60,7 +60,7 @@ module Ameba::AST end private def in_loop? - @loop_stack.any? + !@loop_stack.empty? end end end diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 3f80899eb..821f2845c 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -61,7 +61,7 @@ module Ameba::Cli if f.size == 1 && f.first =~ /.+:\d+:\d+/ configure_explain_opts(f.first, opts) else - opts.globs = f if f.any? + opts.globs = f unless f.empty? end end diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr index d6d8fc2c2..4a55f4463 100644 --- a/src/ameba/rule/lint/unneeded_disable_directive.cr +++ b/src/ameba/rule/lint/unneeded_disable_directive.cr @@ -36,7 +36,7 @@ module Ameba::Rule::Lint next unless token.type == :COMMENT next unless directive = source.parse_inline_directive(token.value.to_s) next unless names = unneeded_disables(source, directive, token.location) - next unless names.any? + next if names.empty? issue_for token, MSG % names.join(", ") end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index 4333bd6d5..f33680276 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -45,7 +45,7 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name.in?(CALL_NAMES) && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block - return if !node.block.nil? || node.args.any? + return unless node.block.nil? && node.args.empty? return unless obj.name.in?(filter_names) message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE From ecad80a96b069b4354b8b5ede02011bad4edc859 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 3 Feb 2021 09:49:00 +0200 Subject: [PATCH 26/37] NewRule: SpecFocus closes #172 --- spec/ameba/rule/lint/spec_focus_spec.cr | 127 ++++++++++++++++++++++++ src/ameba/rule/lint/spec_focus.cr | 70 +++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 spec/ameba/rule/lint/spec_focus_spec.cr create mode 100644 src/ameba/rule/lint/spec_focus.cr diff --git a/spec/ameba/rule/lint/spec_focus_spec.cr b/spec/ameba/rule/lint/spec_focus_spec.cr new file mode 100644 index 000000000..328d6f066 --- /dev/null +++ b/spec/ameba/rule/lint/spec_focus_spec.cr @@ -0,0 +1,127 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + describe SpecFocus do + subject = SpecFocus.new + + it "does not report if spec is not focused" do + s = Source.new %( + context "context" {} + describe "describe" {} + it "it" {} + pending "pending" {} + ), path: "source_spec.cr" + + subject.catch(s).should be_valid + end + + it "reports if there is a focused context" do + s = Source.new %( + context "context", focus: true do + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + end + + it "reports if there is a focused describe block" do + s = Source.new %( + describe "describe", focus: true do + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + end + + it "reports if there is a focused it block" do + s = Source.new %( + it "it", focus: true do + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + end + + it "reports if there is a focused pending block" do + s = Source.new %( + pending "pending", focus: true do + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + end + + it "reports if there is a spec item with `focus: false`" do + s = Source.new %( + it "it", focus: false do + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + end + + it "does not report if there is non spec block with :focus" do + s = Source.new %( + some_method "foo", focus: true do + end + ), path: "source_spec.cr" + + subject.catch(s).should be_valid + end + + it "does not report if there is a tagged item with :focus" do + s = Source.new %( + it "foo", tags: "focus" do + end + ), path: "source_spec.cr" + + subject.catch(s).should be_valid + end + + it "does not report if there are focused spec items without blocks" do + s = Source.new %( + describe "foo", focus: true + context "foo", focus: true + it "foo", focus: true + pending "foo", focus: true + ), path: "source_spec.cr" + + subject.catch(s).should be_valid + end + + it "does not report if there are focused items out of spec file" do + s = Source.new %( + describe "foo", focus: true {} + context "foo", focus: true {} + it "foo", focus: true {} + pending "foo", focus: true {} + ) + + subject.catch(s).should be_valid + end + + it "reports rule, pos and message" do + s = Source.new %( + it "foo", focus: true do + it "bar", focus: true {} + end + ), path: "source_spec.cr" + + subject.catch(s).should_not be_valid + + s.issues.size.should eq(2) + + first, second = s.issues + + first.rule.should_not be_nil + first.location.to_s.should eq "source_spec.cr:1:11" + first.end_location.to_s.should eq "" + first.message.should eq "Focused spec item detected" + + second.rule.should_not be_nil + second.location.to_s.should eq "source_spec.cr:2:13" + second.end_location.to_s.should eq "" + second.message.should eq "Focused spec item detected" + end + end +end diff --git a/src/ameba/rule/lint/spec_focus.cr b/src/ameba/rule/lint/spec_focus.cr new file mode 100644 index 000000000..6407fa11e --- /dev/null +++ b/src/ameba/rule/lint/spec_focus.cr @@ -0,0 +1,70 @@ +module Ameba::Rule::Lint + # Checks if specs are focused. + # + # In specs `focus: true` is mainly used to focus on a spec + # item locally during development. However, if such change + # is committed, it silently runs only focused spec on all + # other enviroment, which is undesired. + # + # This is considered bad: + # + # ``` + # describe MyClass, focus: true do + # end + # + # describe ".new", focus: true do + # end + # + # context "my context", focus: true do + # end + # + # it "works", focus: true do + # end + # ``` + # + # And it should be written as the following: + # + # ``` + # describe MyClass do + # end + # + # describe ".new" do + # end + # + # context "my context" do + # end + # + # it "works" do + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/SpecFocus: + # Enabled: true + # ``` + class SpecFocus < Base + properties do + description "Reports focused spec items" + end + + MSG = "Focused spec item detected" + SPEC_ITEM_NAMES = %w(describe context it pending) + + def test(source) + return unless source.path.ends_with?("_spec.cr") + + AST::NodeVisitor.new self, source + end + + def test(source, node : Crystal::Call) + return unless node.name.in?(SPEC_ITEM_NAMES) + return unless node.block + + arg = node.named_args.try &.find(&.name.== "focus") + + issue_for arg, MSG if arg + end + end +end From f8c22a6e77b839eda99cfe6df1ce5f2803d27e74 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 3 Feb 2021 17:14:03 +0200 Subject: [PATCH 27/37] Utilize Source#spec? --- spec/ameba/source_spec.cr | 12 ++++++++++++ src/ameba/rule/lint/spec_focus.cr | 2 +- src/ameba/source.cr | 7 ++++++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/spec/ameba/source_spec.cr b/spec/ameba/source_spec.cr index cd8af8759..fd4781fa4 100644 --- a/spec/ameba/source_spec.cr +++ b/spec/ameba/source_spec.cr @@ -23,6 +23,18 @@ module Ameba end end + describe "#spec?" do + it "returns true if the source is a spec file" do + s = Source.new "", "./source_spec.cr" + s.spec?.should be_true + end + + it "returns false if the source is not a spec file" do + s = Source.new "", "./source.cr" + s.spec?.should be_false + end + end + describe "#matches_path?" do it "returns true if source's path is matched" do s = Source.new "", "source.cr" diff --git a/src/ameba/rule/lint/spec_focus.cr b/src/ameba/rule/lint/spec_focus.cr index 6407fa11e..d1810e318 100644 --- a/src/ameba/rule/lint/spec_focus.cr +++ b/src/ameba/rule/lint/spec_focus.cr @@ -53,7 +53,7 @@ module Ameba::Rule::Lint SPEC_ITEM_NAMES = %w(describe context it pending) def test(source) - return unless source.path.ends_with?("_spec.cr") + return unless source.spec? AST::NodeVisitor.new self, source end diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 2e3f2af4f..68dd2a1c7 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -53,7 +53,12 @@ module Ameba File.expand_path(path) end - # Returns true if *filepath* matches the source's path, false if it does not. + # Returns `true` if the source is a spec file, `false` otherwise. + def spec? + path.ends_with?("_spec.cr") + end + + # Returns `true` if *filepath* matches the source's path, `false` if it does not. def matches_path?(filepath) path == filepath || path == File.expand_path(filepath) end From 0739fad67029def7d55e9f79c8ddcff828478abd Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 24 Jan 2021 20:26:11 +0100 Subject: [PATCH 28/37] Add Style/VerboseBlock rule --- spec/ameba/rule/style/verbose_block_spec.cr | 131 +++++++++++++++++ src/ameba/rule/style/verbose_block.cr | 150 ++++++++++++++++++++ 2 files changed, 281 insertions(+) create mode 100644 spec/ameba/rule/style/verbose_block_spec.cr create mode 100644 src/ameba/rule/style/verbose_block.cr diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr new file mode 100644 index 000000000..bca8cf031 --- /dev/null +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -0,0 +1,131 @@ +require "../../../spec_helper" + +module Ameba::Rule::Style + subject = VerboseBlock.new + + describe VerboseBlock do + it "passes if there is no potential performance improvements" do + source = Source.new %( + (1..3).any?(&.odd?) + (1..3).join('.', &.to_s) + (1..3).map { |i| typeof(i) } + (1..3).map { |i| i || 0 } + ) + subject.catch(source).should be_valid + end + + it "reports if there is a call with a collapsible block" do + source = Source.new %( + (1..3).any? { |i| i.odd? } + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is a call with an argument + collapsible block" do + source = Source.new %( + (1..3).join('.') { |i| i.to_s } + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is a call with a collapsible block (with chained call)" do + source = Source.new %( + (1..3).map { |i| i.to_s.split.reverse.join.strip } + ) + subject.catch(source).should_not be_valid + end + + context "properties" do + it "#exclude_calls_with_block" do + source = Source.new %( + (1..3).in_groups_of(1) { |i| i.map(&.to_s) } + ) + rule = VerboseBlock.new + rule + .tap(&.exclude_calls_with_block = true) + .catch(source).should be_valid + rule + .tap(&.exclude_calls_with_block = false) + .catch(source).should_not be_valid + end + + it "#exclude_multiple_line_blocks" do + source = Source.new %( + (1..3).any? do |i| + i.odd? + end + ) + rule = VerboseBlock.new + rule + .tap(&.exclude_multiple_line_blocks = true) + .catch(source).should be_valid + rule + .tap(&.exclude_multiple_line_blocks = false) + .catch(source).should_not be_valid + end + + it "#exclude_operators" do + source = Source.new %( + (1..3).sum { |i| i * 2 } + ) + rule = VerboseBlock.new + rule + .tap(&.exclude_operators = true) + .catch(source).should be_valid + rule + .tap(&.exclude_operators = false) + .catch(source).should_not be_valid + end + + it "#exclude_setters" do + source = Source.new %( + Char::Reader.new("abc").tap { |reader| reader.pos = 0 } + ) + rule = VerboseBlock.new + rule + .tap(&.exclude_setters = true) + .catch(source).should be_valid + rule + .tap(&.exclude_setters = false) + .catch(source).should_not be_valid + end + + it "#max_length" do + source = Source.new %( + (1..3).tap { |i| i.to_s.split.reverse.join.strip.blank? } + ) + rule = VerboseBlock.new + rule + .tap(&.max_length = 30) + .catch(source).should be_valid + rule + .tap(&.max_length = nil) + .catch(source).should_not be_valid + end + end + + context "macro" do + it "reports in macro scope" do + source = Source.new %( + {{ (1..3).any? { |i| i.odd? } }} + ) + subject.catch(source).should_not be_valid + end + end + + it "reports rule, pos and message" do + source = Source.new path: "source.cr", code: %( + (1..3).any? { |i| i.odd? } + ) + subject.catch(source).should_not be_valid + source.issues.size.should eq 1 + + issue = source.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:8" + issue.end_location.to_s.should eq "source.cr:1:26" + + issue.message.should eq "Use short block notation instead: `any?(&.odd?)`" + end + end +end diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr new file mode 100644 index 000000000..f36728f26 --- /dev/null +++ b/src/ameba/rule/style/verbose_block.cr @@ -0,0 +1,150 @@ +module Ameba::Rule::Style + # This rule is used to identify usage of single expression blocks with + # argument as a receiver, that can be collapsed into a short form. + # + # For example, this is considered invalid: + # + # ``` + # (1..3).any? { |i| i.odd? } + # ``` + # + # And it should be written as this: + # + # ``` + # (1..3).any?(&.odd?) + # ``` + # + # YAML configuration example: + # + # ``` + # Style/VerboseBlock: + # Enabled: true + # ExcludeMultipleLineBlocks: true + # ExcludeCallsWithBlocks: false + # ExcludeOperators: false + # ExcludeSetters: false + # MaxLength: 50 # use ~ to disable + # ``` + class VerboseBlock < Base + properties do + description "Identifies usage of collapsible single expression blocks." + + exclude_calls_with_block true + exclude_multiple_line_blocks false + exclude_operators false + exclude_setters false + + max_length : Int32? = 50 + end + + MSG = "Use short block notation instead: `%s`" + CALL_PATTERN = "%s(%s&.%s)" + + protected def same_location_lines?(a, b) + return unless a_location = a.name_location + return unless b_location = b.location + + a_location.line_number == b_location.line_number + end + + private OPERATOR_CHARS = + {'[', ']', '!', '=', '>', '<', '~', '+', '-', '*', '/', '%', '^', '|', '&'} + + protected def operator?(name) + name.each_char do |char| + return false unless char.in?(OPERATOR_CHARS) + end + !name.empty? + end + + protected def setter?(name) + !name.empty? && name[0].letter? && name.ends_with?('=') + end + + protected def valid_length?(code) + if max_length = self.max_length + return code.size <= max_length + end + true + end + + protected def node_to_s(node : Crystal::Call) + case name = node.name + when "[]" + name = "[#{node.args.join ", "}]" + when "[]?" + name = "[#{node.args.join ", "}]?" + when "[]=" + unless node.args.empty? + name = "[#{node.args[..-2].join ", "}]=(#{node.args.last})" + end + else + name += "(#{node.args.join ", "})" unless node.args.empty? + name += " {...}" if node.block + end + name + end + + protected def call_code(call, body) + args = call.args.join ", " unless call.args.empty? + args += ", " if args + + call_chain = %w[].tap do |arr| + obj = body.obj + while obj.is_a?(Crystal::Call) + arr << node_to_s(obj) + obj = obj.obj + end + arr.reverse! + arr << node_to_s(body) + end + + name = + call_chain.join('.') + + CALL_PATTERN % {call.name, args, name} + end + + # ameba:disable Metrics/CyclomaticComplexity + def test(source, node : Crystal::Call) + # we are interested only in calls with block taking a single argument + # + # ``` + # (1..3).any? { |i| i.to_i64.odd? } + # ^--- ^ ^------------ + # block arg body + # ``` + return unless (block = node.block) && block.args.size == 1 + + # 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 challs, so the final receiver object + # ends up being a variable - `i` + obj = body.obj + while obj.is_a?(Crystal::Call) + obj = obj.obj + end + return unless obj.is_a?(Crystal::Var) + + # only calls with a first argument used as a receiver are the valid game + return unless (arg = block.args.first) == obj + + # we skip auto-generated blocks - `(1..3).any?(&.odd?)` + return if arg.name.starts_with?("__arg") + + return if exclude_calls_with_block && body.block + return if exclude_multiple_line_blocks && !same_location_lines?(node, body) + return if exclude_operators && operator?(body.name) + return if exclude_setters && setter?(body.name) + + call_code = + call_code(node, body) + + return unless valid_length?(call_code) + + issue_for node.name_location, node.end_location, + MSG % call_code + end + end +end From a53d44617d681906c69928c1cc9a383177d1cf1e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 25 Jan 2021 02:34:33 +0100 Subject: [PATCH 29/37] Fix newly found issues --- spec/ameba/config_spec.cr | 4 ++-- spec/ameba/severity_spec.cr | 4 +--- src/ameba/ast/scope.cr | 2 +- src/ameba/glob_utils.cr | 2 +- src/ameba/runner.cr | 2 +- 5 files changed, 6 insertions(+), 8 deletions(-) diff --git a/spec/ameba/config_spec.cr b/spec/ameba/config_spec.cr index c92768737..d7547a2a9 100644 --- a/spec/ameba/config_spec.cr +++ b/spec/ameba/config_spec.cr @@ -120,7 +120,7 @@ module Ameba it "returns list of sources" do config.sources.size.should be > 0 config.sources.first.should be_a Source - config.sources.any? { |s| s.fullpath == __FILE__ }.should be_true + config.sources.any?(&.fullpath.==(__FILE__)).should be_true end it "returns a list of sources mathing globs" do @@ -130,7 +130,7 @@ module Ameba it "returns a lisf of sources excluding 'Excluded'" do config.excluded = %w(**/config_spec.cr) - config.sources.any? { |s| s.fullpath == __FILE__ }.should be_false + config.sources.any?(&.fullpath.==(__FILE__)).should be_false end end diff --git a/spec/ameba/severity_spec.cr b/spec/ameba/severity_spec.cr index 94f3b7304..cf63c1334 100644 --- a/spec/ameba/severity_spec.cr +++ b/spec/ameba/severity_spec.cr @@ -4,9 +4,7 @@ module Ameba describe Severity do describe "#symbol" do it "returns the symbol for each severity in the enum" do - Severity.values.each do |severity| - severity.symbol.should_not be_nil - end + Severity.values.each(&.symbol.should_not(be_nil)) end it "returns symbol for Error" do diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 68bed1563..bacf42585 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -78,7 +78,7 @@ module Ameba::AST # scope.find_variable("foo") # ``` def find_variable(name : String) - variables.find { |v| v.name == name } || outer_scope.try &.find_variable(name) + variables.find(&.name.==(name)) || outer_scope.try &.find_variable(name) end # Creates a new assignment for the variable. diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr index a4a035495..71cd0cb5f 100644 --- a/src/ameba/glob_utils.cr +++ b/src/ameba/glob_utils.cr @@ -11,7 +11,7 @@ module Ameba rejected = rejected_globs(globs) selected = globs - rejected - expand(selected) - expand(rejected.map! { |p| p[1..-1] }) + expand(selected) - expand(rejected.map!(&.[1..-1])) end # Expands globs. Globs can point to files or even directories. diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 9e6d29657..56b06e644 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -126,7 +126,7 @@ module Ameba @sources.all? do |source| source.issues .reject(&.disabled?) - .none? { |issue| issue.rule.severity <= @severity } + .none?(&.rule.severity.<=(@severity)) end end From 4b7f3ba6ee5ba425bef6987080b4bb3327019d9e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sat, 30 Jan 2021 18:22:10 +0100 Subject: [PATCH 30/37] Add MaxLineLength option to Style/VerboseBlock rule --- spec/ameba/rule/style/verbose_block_spec.cr | 15 +++++++++++++++ src/ameba/rule/style/verbose_block.cr | 13 +++++++++++++ 2 files changed, 28 insertions(+) diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr index bca8cf031..60e5f676b 100644 --- a/spec/ameba/rule/style/verbose_block_spec.cr +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -90,6 +90,21 @@ module Ameba::Rule::Style .catch(source).should_not be_valid end + it "#max_line_length" do + source = Source.new %( + (1..3).tap &.tap &.tap &.tap &.tap &.tap &.tap do |i| + i.to_s.reverse.strip.blank? + end + ) + rule = VerboseBlock.new + rule + .tap(&.max_line_length = 60) + .catch(source).should be_valid + rule + .tap(&.max_line_length = nil) + .catch(source).should_not be_valid + end + it "#max_length" do source = Source.new %( (1..3).tap { |i| i.to_s.split.reverse.join.strip.blank? } diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index f36728f26..5a09f372b 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -23,6 +23,7 @@ module Ameba::Rule::Style # ExcludeCallsWithBlocks: false # ExcludeOperators: false # ExcludeSetters: false + # MaxLineLength: ~ # MaxLength: 50 # use ~ to disable # ``` class VerboseBlock < Base @@ -34,6 +35,7 @@ module Ameba::Rule::Style exclude_operators false exclude_setters false + max_line_length : Int32? = nil # 100 max_length : Int32? = 50 end @@ -68,6 +70,16 @@ module Ameba::Rule::Style true end + protected def valid_line_length?(node, code) + if max_line_length = self.max_line_length + if location = node.name_location + final_line_length = location.column_number + code.size + return final_line_length <= max_line_length + end + end + true + end + protected def node_to_s(node : Crystal::Call) case name = node.name when "[]" @@ -141,6 +153,7 @@ module Ameba::Rule::Style call_code = call_code(node, body) + return unless valid_line_length?(node, call_code) return unless valid_length?(call_code) issue_for node.name_location, node.end_location, From 5faf8da81cac3b2e1d0112fa4488bf1a1fa1721a Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 1 Feb 2021 01:51:21 +0100 Subject: [PATCH 31/37] Split VerboseBlock#test method into a smaller pieces --- src/ameba/rule/style/verbose_block.cr | 32 +++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index 5a09f372b..552bdb777 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -117,7 +117,22 @@ module Ameba::Rule::Style CALL_PATTERN % {call.name, args, name} end - # ameba:disable Metrics/CyclomaticComplexity + protected def issue_for_valid(source, call : Crystal::Call, body : Crystal::Call) + return if exclude_calls_with_block && body.block + return if exclude_multiple_line_blocks && !same_location_lines?(call, body) + return if exclude_operators && operator?(body.name) + return if exclude_setters && setter?(body.name) + + call_code = + call_code(call, body) + + return unless valid_line_length?(call, call_code) + return unless valid_length?(call_code) + + issue_for call.name_location, call.end_location, + MSG % call_code + end + def test(source, node : Crystal::Call) # we are interested only in calls with block taking a single argument # @@ -145,19 +160,8 @@ module Ameba::Rule::Style # we skip auto-generated blocks - `(1..3).any?(&.odd?)` return if arg.name.starts_with?("__arg") - return if exclude_calls_with_block && body.block - return if exclude_multiple_line_blocks && !same_location_lines?(node, body) - return if exclude_operators && operator?(body.name) - return if exclude_setters && setter?(body.name) - - call_code = - call_code(node, body) - - return unless valid_line_length?(node, call_code) - return unless valid_length?(call_code) - - issue_for node.name_location, node.end_location, - MSG % call_code + # add issue if the given nodes pass all of the checks + issue_for_valid source, node, body end end end From eed094b928f0f2a944c96664b8019642c4f4a6d0 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 2 Feb 2021 13:52:03 +0100 Subject: [PATCH 32/37] Fix couple of edge-cases in VerboseBlock rule --- spec/ameba/rule/style/verbose_block_spec.cr | 12 ++++++++ src/ameba/rule/style/verbose_block.cr | 32 +++++++++++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr index 60e5f676b..3122eb972 100644 --- a/spec/ameba/rule/style/verbose_block_spec.cr +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -14,6 +14,18 @@ module Ameba::Rule::Style subject.catch(source).should be_valid end + it "passes if the block argument is used within the body" do + source = Source.new %( + (1..3).map { |i| i * i } + (1..3).map { |j| j * j.to_i64 } + (1..3).map { |k| k.to_i64 * k } + (1..3).map { |l| l.to_i64 * l.to_i64 } + (1..3).map { |m| m.to_s[start: m.to_i64, count: 3]? } + (1..3).map { |n| n.to_s.split.map { |z| n.to_i * z.to_i }.join } + ) + subject.catch(source).should be_valid + end + it "reports if there is a call with a collapsible block" do source = Source.new %( (1..3).any? { |i| i.odd? } diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index 552bdb777..42784f4d3 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -80,6 +80,27 @@ module Ameba::Rule::Style true end + protected def reference_count(node, obj : Crystal::Var) + i = 0 + case node + when Crystal::Call + i += reference_count(node.obj, obj) + i += reference_count(node.block, obj) + + node.args.each do |arg| + i += reference_count(arg, obj) + end + node.named_args.try &.each do |arg| + i += reference_count(arg.value, obj) + end + when Crystal::Block + i += reference_count(node.body, obj) + when Crystal::Var + i += 1 if node == obj + end + i + end + protected def node_to_s(node : Crystal::Call) case name = node.name when "[]" @@ -143,6 +164,11 @@ module Ameba::Rule::Style # ``` return unless (block = node.block) && block.args.size == 1 + arg = block.args.first + + # we skip auto-generated blocks - `(1..3).any?(&.odd?)` + return if arg.name.starts_with?("__arg") + # we filter out the blocks that are of call type - `i.to_i64.odd?` return unless (body = block.body).is_a?(Crystal::Call) @@ -155,10 +181,10 @@ module Ameba::Rule::Style return unless obj.is_a?(Crystal::Var) # only calls with a first argument used as a receiver are the valid game - return unless (arg = block.args.first) == obj + return unless obj == arg - # we skip auto-generated blocks - `(1..3).any?(&.odd?)` - return if arg.name.starts_with?("__arg") + # we bail out if the block node include the block argument + return if reference_count(body, arg) > 1 # add issue if the given nodes pass all of the checks issue_for_valid source, node, body From 16743a756cf329f7d6610d8e7d40838c00948dba Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 3 Feb 2021 02:00:49 +0100 Subject: [PATCH 33/37] Add ExcludePrefixOperators option to Style/VerboseBlock rule --- spec/ameba/rule/style/verbose_block_spec.cr | 14 ++++++++++++++ src/ameba/rule/style/verbose_block.cr | 11 ++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr index 3122eb972..b53aa6927 100644 --- a/spec/ameba/rule/style/verbose_block_spec.cr +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -76,6 +76,20 @@ module Ameba::Rule::Style .catch(source).should_not be_valid end + it "#exclude_prefix_operators" do + source = Source.new %( + (1..3).sum { |i| +i } + (1..3).sum { |i| -i } + ) + rule = VerboseBlock.new + rule + .tap(&.exclude_prefix_operators = true) + .catch(source).should be_valid + rule + .tap(&.exclude_prefix_operators = false) + .catch(source).should_not be_valid + end + it "#exclude_operators" do source = Source.new %( (1..3).sum { |i| i * 2 } diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index 42784f4d3..a3ec6142d 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -21,6 +21,7 @@ module Ameba::Rule::Style # Enabled: true # ExcludeMultipleLineBlocks: true # ExcludeCallsWithBlocks: false + # ExcludePrefixOperators: true # ExcludeOperators: false # ExcludeSetters: false # MaxLineLength: ~ @@ -32,6 +33,7 @@ module Ameba::Rule::Style exclude_calls_with_block true exclude_multiple_line_blocks false + exclude_prefix_operators true exclude_operators false exclude_setters false @@ -49,9 +51,14 @@ module Ameba::Rule::Style a_location.line_number == b_location.line_number end - private OPERATOR_CHARS = + private PREFIX_OPERATORS = {"+", "-"} + private OPERATOR_CHARS = {'[', ']', '!', '=', '>', '<', '~', '+', '-', '*', '/', '%', '^', '|', '&'} + protected def prefix_operator?(node) + node.name.in?(PREFIX_OPERATORS) && node.args.empty? + end + protected def operator?(name) name.each_char do |char| return false unless char.in?(OPERATOR_CHARS) @@ -138,9 +145,11 @@ module Ameba::Rule::Style CALL_PATTERN % {call.name, args, name} end + # ameba:disable Metrics/CyclomaticComplexity protected def issue_for_valid(source, call : Crystal::Call, body : Crystal::Call) return if exclude_calls_with_block && body.block return if exclude_multiple_line_blocks && !same_location_lines?(call, body) + return if exclude_prefix_operators && prefix_operator?(body) return if exclude_operators && operator?(body.name) return if exclude_setters && setter?(body.name) From a9d1b17debf34eb716089bc3cc0d9146408df70a Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 3 Feb 2021 13:39:35 +0100 Subject: [PATCH 34/37] Support named arguments in VerboseBlock#node_to_s --- spec/ameba/rule/style/verbose_block_spec.cr | 38 ++++++++++++++++ src/ameba/rule/style/verbose_block.cr | 49 +++++++++++++++------ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr index b53aa6927..3a6ff8fe1 100644 --- a/spec/ameba/rule/style/verbose_block_spec.cr +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -154,6 +154,44 @@ module Ameba::Rule::Style end end + it "reports call args and named_args" do + short_block_variants = { + %|map(&.to_s.[start: 0.to_i64, count: 3]?)|, + %|map(&.to_s.[0.to_i64, count: 3]?)|, + %|map(&.to_s.[0.to_i64, 3]?)|, + %|map(&.to_s.[start: 0.to_i64, count: 3]=("foo"))|, + %|map(&.to_s.[0.to_i64, count: 3]=("foo"))|, + %|map(&.to_s.[0.to_i64, 3]=("foo"))|, + %|map(&.to_s.camelcase(lower: true))|, + %|map(&.to_s.camelcase)|, + %|map(&.to_s.gsub('_', '-'))|, + %|map(&.in?(*{1, 2, 3}, **{foo: :bar}))|, + %|map(&.in?(1, *foo, 3, **bar))|, + %|join(separator: '.', &.to_s)|, + } + + source = Source.new path: "source.cr", code: %( + (1..3).map { |i| i.to_s[start: 0.to_i64, count: 3]? } + (1..3).map { |i| i.to_s[0.to_i64, count: 3]? } + (1..3).map { |i| i.to_s[0.to_i64, 3]? } + (1..3).map { |i| i.to_s[start: 0.to_i64, count: 3] = "foo" } + (1..3).map { |i| i.to_s[0.to_i64, count: 3] = "foo" } + (1..3).map { |i| i.to_s[0.to_i64, 3] = "foo" } + (1..3).map { |i| i.to_s.camelcase(lower: true) } + (1..3).map { |i| i.to_s.camelcase } + (1..3).map { |i| i.to_s.gsub('_', '-') } + (1..3).map { |i| i.in?(*{1, 2, 3}, **{foo: :bar}) } + (1..3).map { |i| i.in?(1, *foo, 3, **bar) } + (1..3).join(separator: '.') { |i| i.to_s } + ) + subject.catch(source).should_not be_valid + source.issues.size.should eq(short_block_variants.size) + + source.issues.each_with_index do |issue, i| + issue.message.should eq(VerboseBlock::MSG % short_block_variants[i]) + end + end + it "reports rule, pos and message" do source = Source.new path: "source.cr", code: %( (1..3).any? { |i| i.odd? } diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index a3ec6142d..12bb0e0ba 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -108,25 +108,48 @@ module Ameba::Rule::Style i end + protected def args_to_s(io : IO, node : Crystal::Call, skip_last_arg = false) + node.args.dup.tap do |args| + args.pop? if skip_last_arg + args.join io, ", " + node.named_args.try do |named_args| + io << ", " unless args.empty? || named_args.empty? + named_args.join io, ", " do |arg, inner_io| + inner_io << arg.name << ": " << arg.value + end + end + end + end + protected def node_to_s(node : Crystal::Call) - case name = node.name - when "[]" - name = "[#{node.args.join ", "}]" - when "[]?" - name = "[#{node.args.join ", "}]?" - when "[]=" - unless node.args.empty? - name = "[#{node.args[..-2].join ", "}]=(#{node.args.last})" + String.build do |str| + case name = node.name + when "[]" + str << '[' + args_to_s(str, node) + str << ']' + when "[]?" + str << '[' + args_to_s(str, node) + str << "]?" + when "[]=" + str << '[' + args_to_s(str, node, skip_last_arg: true) + str << "]=(" << node.args.last? << ')' + else + str << name + if !node.args.empty? || (node.named_args && !node.named_args.try(&.empty?)) + str << '(' + args_to_s(str, node) + str << ')' + end + str << " {...}" if node.block end - else - name += "(#{node.args.join ", "})" unless node.args.empty? - name += " {...}" if node.block end - name end protected def call_code(call, body) - args = call.args.join ", " unless call.args.empty? + args = String.build { |io| args_to_s(io, call) }.presence args += ", " if args call_chain = %w[].tap do |arr| From 694c41650c92fbbb298f4f34ecaee7e7709b98e7 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 4 Feb 2021 22:24:29 +0100 Subject: [PATCH 35/37] Remove redundant check and add a few more test cases --- spec/ameba/rule/style/verbose_block_spec.cr | 4 ++++ src/ameba/rule/style/verbose_block.cr | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/ameba/rule/style/verbose_block_spec.cr b/spec/ameba/rule/style/verbose_block_spec.cr index 3a6ff8fe1..3689cecb9 100644 --- a/spec/ameba/rule/style/verbose_block_spec.cr +++ b/spec/ameba/rule/style/verbose_block_spec.cr @@ -8,8 +8,12 @@ module Ameba::Rule::Style source = Source.new %( (1..3).any?(&.odd?) (1..3).join('.', &.to_s) + (1..3).each_with_index { |i, idx| i * idx } (1..3).map { |i| typeof(i) } (1..3).map { |i| i || 0 } + (1..3).map { |i| :foo } + (1..3).map { |i| :foo.to_s.split.join('.') } + (1..3).map { :foo } ) subject.catch(source).should be_valid end diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index 12bb0e0ba..d50490915 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -210,7 +210,6 @@ module Ameba::Rule::Style while obj.is_a?(Crystal::Call) obj = obj.obj end - return unless obj.is_a?(Crystal::Var) # only calls with a first argument used as a receiver are the valid game return unless obj == arg From bf64c8242b893412dc0f9f0c3ad9a63e5c2b230b Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 8 Feb 2021 21:03:34 +0100 Subject: [PATCH 36/37] Bump to v0.14.0 --- shard.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shard.yml b/shard.yml index e40c86dcd..e64b2a966 100644 --- a/shard.yml +++ b/shard.yml @@ -1,5 +1,5 @@ name: ameba -version: 0.13.4 +version: 0.14.0 authors: - Vitalii Elenhaupt From 3a1ee7fa218ecf3a833029360133a05ec5fd564c Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Thu, 11 Feb 2021 11:01:30 +0200 Subject: [PATCH 37/37] Correct yml examples --- src/ameba/rule/performance/chained_call_with_no_bang.cr | 2 +- src/ameba/rule/performance/compact_after_map.cr | 2 +- src/ameba/rule/performance/flatten_after_map.cr | 2 +- src/ameba/rule/performance/map_instead_of_block.cr | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ameba/rule/performance/chained_call_with_no_bang.cr b/src/ameba/rule/performance/chained_call_with_no_bang.cr index d7f827ae5..22a57c780 100644 --- a/src/ameba/rule/performance/chained_call_with_no_bang.cr +++ b/src/ameba/rule/performance/chained_call_with_no_bang.cr @@ -25,7 +25,7 @@ module Ameba::Rule::Performance # YAML configuration example: # # ``` - # Performance/ChainedCallWithNoBang + # Performance/ChainedCallWithNoBang: # Enabled: true # CallNames: # - uniq diff --git a/src/ameba/rule/performance/compact_after_map.cr b/src/ameba/rule/performance/compact_after_map.cr index 77f542cf2..63184498d 100644 --- a/src/ameba/rule/performance/compact_after_map.cr +++ b/src/ameba/rule/performance/compact_after_map.cr @@ -16,7 +16,7 @@ module Ameba::Rule::Performance # YAML configuration example: # # ``` - # Performance/CompactAfterMap + # Performance/CompactAfterMap: # Enabled: true # ``` class CompactAfterMap < Base diff --git a/src/ameba/rule/performance/flatten_after_map.cr b/src/ameba/rule/performance/flatten_after_map.cr index 631c656d0..18ee2f8ab 100644 --- a/src/ameba/rule/performance/flatten_after_map.cr +++ b/src/ameba/rule/performance/flatten_after_map.cr @@ -16,7 +16,7 @@ module Ameba::Rule::Performance # YAML configuration example: # # ``` - # Performance/FlattenAfterMap + # Performance/FlattenAfterMap: # Enabled: true # ``` class FlattenAfterMap < Base diff --git a/src/ameba/rule/performance/map_instead_of_block.cr b/src/ameba/rule/performance/map_instead_of_block.cr index 5e8036173..377130fc6 100644 --- a/src/ameba/rule/performance/map_instead_of_block.cr +++ b/src/ameba/rule/performance/map_instead_of_block.cr @@ -19,7 +19,7 @@ module Ameba::Rule::Performance # YAML configuration example: # # ``` - # Performance/MapInsteadOfBlock + # Performance/MapInsteadOfBlock: # Enabled: true # ``` class MapInsteadOfBlock < Base