From ea98554191bfd5d64a8c52a4271d9af98f9b1160 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 26 Jan 2021 07:38:19 +0100 Subject: [PATCH] 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 | 107 ++++++++++++++------- src/ameba/issue.cr | 25 +++-- src/ameba/reportable.cr | 43 +++++---- 9 files changed, 225 insertions(+), 100 deletions(-) diff --git a/README.md b/README.md index b901259a..fd5d184e 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 44442622..3ba90983 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 9a002baa..9087228f 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 652f5247..b6488552 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 d0f43f7a..121ea1cf 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 4724b087..f8fe87c1 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 822489f0..d6c411c8 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[] + pre_context, post_context = + context(lines, lineno, context_lines) - 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 + position = prompt.size + column + position -= 1 + else + affected_line_size, affected_line = + affected_line.size, affected_line.lstrip - # remove empty lines at the beginning/end - pre_context.shift? unless pre_context.first?.presence - post_context.pop? unless post_context.last?.presence + 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 << prompt + str.puts(affected_line.colorize(:white)) - str << " " * (prompt.size + column - 1) - str.puts("^".colorize(:yellow)) + str << (" " * position) + str << "^".colorize(:yellow) + if end_location + end_lineno = end_location.line_number + end_column = end_location.column_number + + 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 03e95165..b913eac3 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 e31ef3a7..4632fa91 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