Add support for showing end location marker (#200)

* 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

@@ -4,7 +4,7 @@
   <p align="center">Code style linter for Crystal<p>
   <p align="center">
     <sup>
-      <i> (a single-celled animal that catches food and moves about by extending fingerlike projections of protoplasm) </i>
+      <i>(a single-celled animal that catches food and moves about by extending fingerlike projections of protoplasm)</i>
     </sup>
   </p>
   <p align="center">
@@ -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

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

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

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

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.puts finished_in_message(@started_at, Time.monotonic)
-      output << final_message(sources, failed_sources)
+      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

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)
+    def initialize(@output, location)
-      @location = Crystal::Location.new(loc[:file], loc[:line], loc[:column])
+      @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|
+      issue = source.issues.find(&.location.==(@location))
-        if (location = issue.location) &&
+      return unless issue
-           location.line_number == @location.line_number &&
+
-           location.column_number == @location.column_number
+      explain(source, issue)
-          explain(source, issue)
-        end
-      end
     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))

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|
+        position = prompt.size + column
-          case i + 1
+        position -= 1
-          when lineno - context_lines...lineno
+      else
-            pre_context << trim_line.call(line)
+        affected_line_size, affected_line =
-          when lineno
+          affected_line.size, affected_line.lstrip
-            #
-          when lineno + 1..lineno + context_lines
-            post_context << trim_line.call(line)
-          end
-        end
 
-        # remove empty lines at the beginning/end
+        position = column - (affected_line_size - affected_line.size) + prompt.size
-        pre_context.shift? unless pre_context.first?.presence
+        position -= 1
-        post_context.pop? unless post_context.last?.presence
       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 << prompt
-          str.puts(affected_line.colorize(:white))
+        str.puts(affected_line.colorize(:white))
 
-          str << " " * (prompt.size + column - 1)
+        str << (" " * position)
-          str.puts("^".colorize(:yellow))
+        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

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
+    getter status : Status
-    def disabled?
+
-      status == :disabled
+    delegate :enabled?, :disabled?,
+      to: status
+
+    def initialize(@rule, @location, @end_location, @message, status : Status? = nil)
+      @status = status || Status::Enabled
     end
 
     def syntax?

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)
+    def add_issue(rule, location, end_location, message, status : Issue::Status? = nil) : Issue
-      status ||= :disabled if location_disabled?(location, rule)
+      status ||=
-      issues << Issue.new rule, location, end_location, message, 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*.
+    # Adds a new issue for Crystal AST *node*.
-    def add_issue(rule, node : Crystal::ASTNode, message, **args)
+    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil) : Issue
-      add_issue rule, node.location, node.end_location, message, **args
+      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)
+    def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil) : Issue
-      add_issue rule, token.location, nil, message, **args
+      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)
+    def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue
-      location = Crystal::Location.new path, *location
+      location =
-      add_issue rule, location, nil, message, **args
+        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)
+    def add_issue(rule, location : {Int32, Int32}, end_location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue
-      location = Crystal::Location.new path, *location
+      location =
-      end_location = Crystal::Location.new path, *end_location
+        Crystal::Location.new(path, *location)
-      add_issue rule, location, end_location, message, **args
+      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