Merge pull request #380 from crystal-ameba/add-documentation-admonition-rule

Add `Lint/DocumentationAdmonition` rule

.ameba.yml

@@ -0,0 +1,3 @@
+Lint/DocumentationAdmonition:
+  Timezone: UTC
+  Admonitions: [FIXME, BUG]

.github/workflows/ci.yml

@@ -18,6 +18,9 @@ jobs:
     runs-on: ${{ matrix.os }}
 
     steps:
+      - name: Set timezone to UTC
+        uses: szenius/set-timezone@v1.2
+
       - name: Install Crystal
         uses: crystal-lang/install-crystal@v1
         with:

spec/ameba/formatter/todo_formatter_spec.cr

@@ -1,10 +1,12 @@
 require "../../spec_helper"
 require "file_utils"
 
+CONFIG_PATH = Path[Dir.tempdir] / Ameba::Config::FILENAME
+
 module Ameba
   private def with_formatter(&)
     io = IO::Memory.new
-    formatter = Formatter::TODOFormatter.new(io)
+    formatter = Formatter::TODOFormatter.new(io, CONFIG_PATH)
 
     yield formatter, io
   end
@@ -20,7 +22,7 @@ module Ameba
 
   describe Formatter::TODOFormatter do
     ::Spec.after_each do
-      FileUtils.rm_rf(Ameba::Config::DEFAULT_PATH)
+      FileUtils.rm_rf(CONFIG_PATH)
     end
 
     context "problems not found" do
@@ -45,7 +47,7 @@ module Ameba
           s = Source.new "a = 1", "source.cr"
           s.add_issue DummyRule.new, {1, 2}, "message"
           formatter.finished([s])
-          io.to_s.should contain "Created #{Config::DEFAULT_PATH}"
+          io.to_s.should contain "Created #{CONFIG_PATH}"
         end
       end
 

spec/ameba/rule/lint/documentation_admonition_spec.cr

@@ -0,0 +1,113 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  subject = DocumentationAdmonition.new
+
+  describe DocumentationAdmonition do
+    it "passes for comments with admonition mid-word/sentence" do
+      subject.admonitions.each do |admonition|
+        expect_no_issues subject, <<-CRYSTAL
+          # Mentioning #{admonition} mid-sentence
+          # x#{admonition}x
+          # x#{admonition}
+          # #{admonition}x
+          CRYSTAL
+      end
+    end
+
+    it "fails for comments with admonition" do
+      subject.admonitions.each do |admonition|
+        expect_issue subject, <<-CRYSTAL
+          # #{admonition}: Single-line comment
+          # ^{} error: Found a #{admonition} admonition in a comment
+          CRYSTAL
+
+        expect_issue subject, <<-CRYSTAL
+          # Text before ...
+          # #{admonition}(some context): Part of multi-line comment
+          # ^{} error: Found a #{admonition} admonition in a comment
+          # Text after ...
+          CRYSTAL
+
+        expect_issue subject, <<-CRYSTAL
+          # #{admonition}
+          # ^{} error: Found a #{admonition} admonition in a comment
+          if rand > 0.5
+          end
+          CRYSTAL
+      end
+    end
+
+    context "with date" do
+      it "passes for admonitions with future date" do
+        subject.admonitions.each do |admonition|
+          future_date = (Time.utc + 21.days).to_s(format: "%F")
+          expect_no_issues subject, <<-CRYSTAL
+            # #{admonition}(#{future_date}): sth in the future
+            CRYSTAL
+        end
+      end
+
+      it "fails for admonitions with past date" do
+        subject.admonitions.each do |admonition|
+          past_date = (Time.utc - 21.days).to_s(format: "%F")
+          expect_issue subject, <<-CRYSTAL
+            # #{admonition}(#{past_date}): sth in the past
+            # ^{} error: Found a #{admonition} admonition in a comment (21 days past)
+            CRYSTAL
+        end
+      end
+
+      it "fails for admonitions with yesterday's date" do
+        subject.admonitions.each do |admonition|
+          yesterday_date = (Time.utc - 1.day).to_s(format: "%F")
+          expect_issue subject, <<-CRYSTAL
+            # #{admonition}(#{yesterday_date}): sth in the past
+            # ^{} error: Found a #{admonition} admonition in a comment (1 day past)
+            CRYSTAL
+        end
+      end
+
+      it "fails for admonitions with today's date" do
+        subject.admonitions.each do |admonition|
+          today_date = Time.utc.to_s(format: "%F")
+          expect_issue subject, <<-CRYSTAL
+            # #{admonition}(#{today_date}): sth in the past
+            # ^{} error: Found a #{admonition} admonition in a comment (today is the day!)
+            CRYSTAL
+        end
+      end
+
+      it "fails for admonitions with invalid date" do
+        subject.admonitions.each do |admonition|
+          expect_issue subject, <<-CRYSTAL
+            # #{admonition}(0000-00-00): sth wrong
+            # ^{} error: #{admonition} admonition error: Invalid time: "0000-00-00"
+            CRYSTAL
+        end
+      end
+    end
+
+    context "properties" do
+      describe "#admonitions" do
+        it "lets setting custom admonitions" do
+          rule = DocumentationAdmonition.new
+          rule.admonitions = %w[FOO BAR]
+
+          rule.admonitions.each do |admonition|
+            expect_issue rule, <<-CRYSTAL
+              # #{admonition}
+              # ^{} error: Found a #{admonition} admonition in a comment
+              CRYSTAL
+          end
+
+          subject.admonitions.each do |admonition|
+            expect_no_issues rule, <<-CRYSTAL
+              # #{admonition}
+              CRYSTAL
+          end
+        end
+      end
+    end
+  end
+end

src/ameba/formatter/todo_formatter.cr

@@ -3,7 +3,7 @@ module Ameba::Formatter
   # Basically, it takes all issues reported and disables corresponding rules
   # or excludes failed sources from these rules.
   class TODOFormatter < DotFormatter
-    def initialize(@output = STDOUT)
+    def initialize(@output = STDOUT, @config_path : Path = Config::DEFAULT_PATH)
     end
 
     def finished(sources)
@@ -26,7 +26,7 @@ module Ameba::Formatter
     end
 
     private def generate_todo_config(issues)
-      file = File.new(Config::DEFAULT_PATH, mode: "w")
+      file = File.new(@config_path, mode: "w")
       file << header
       rule_issues_map(issues).each do |rule, rule_issues|
         file << "\n# Problems found: #{rule_issues.size}"

src/ameba/rule/lint/documentation_admonition.cr

@@ -0,0 +1,96 @@
+module Ameba::Rule::Lint
+  # A rule that reports documentation admonitions.
+  #
+  # Optionally, these can fail at an appropriate time.
+  #
+  # ```
+  # def get_user(id)
+  #   # TODO(2024-04-24) Fix this hack when the database migration is complete
+  #   if id < 1_000_000
+  #     v1_api_call(id)
+  #   else
+  #     v2_api_call(id)
+  #   end
+  # end
+  # ```
+  #
+  # `TODO` comments are used to remind yourself of source code related things.
+  #
+  # The premise here is that `TODO` should be dealt with in the near future
+  # and are therefore reported by Ameba.
+  #
+  # `FIXME` comments are used to indicate places where source code needs fixing.
+  #
+  # The premise here is that `FIXME` should indeed be fixed as soon as possible
+  # and are therefore reported by Ameba.
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/DocumentationAdmonition:
+  #   Enabled: true
+  #   Admonitions: [TODO, FIXME, BUG]
+  #   Timezone: UTC
+  # ```
+  class DocumentationAdmonition < Base
+    properties do
+      description "Reports documentation admonitions"
+      admonitions %w[TODO FIXME BUG]
+      timezone "UTC"
+    end
+
+    MSG      = "Found a %s admonition in a comment"
+    MSG_LATE = "Found a %s admonition in a comment (%s)"
+    MSG_ERR  = "%s admonition error: %s"
+
+    @[YAML::Field(ignore: true)]
+    private getter location : Time::Location {
+      Time::Location.load(self.timezone)
+    }
+
+    def test(source)
+      Tokenizer.new(source).run do |token|
+        next unless token.type.comment?
+        next unless doc = token.value.to_s
+
+        pattern =
+          /^#\s*(?<admonition>#{Regex.union(admonitions)})(?:\((?<context>.+?)\))?(?:\W+|$)/m
+
+        matches = doc.scan(pattern)
+        matches.each do |match|
+          admonition = match["admonition"]
+          begin
+            case expr = match["context"]?.presence
+            when /\A\d{4}-\d{2}-\d{2}\Z/ # date
+              # ameba:disable Lint/NotNil
+              date = Time.parse(expr.not_nil!, "%F", location)
+              issue_for_date source, token, admonition, date
+            when /\A\d{4}-\d{2}-\d{2} \d{2}:\d{2}(:\d{2})?\Z/ # date + time (no tz)
+              # ameba:disable Lint/NotNil
+              date = Time.parse(expr.not_nil!, "%F #{$1?.presence ? "%T" : "%R"}", location)
+              issue_for_date source, token, admonition, date
+            else
+              issue_for token, MSG % admonition
+            end
+          rescue ex
+            issue_for token, MSG_ERR % {admonition, "#{ex}: #{expr.inspect}"}
+          end
+        end
+      end
+    end
+
+    private def issue_for_date(source, node, admonition, date)
+      diff = Time.utc - date.to_utc
+
+      return if diff.negative?
+
+      past = case diff
+             when 0.seconds..1.day then "today is the day!"
+             when 1.day..2.days    then "1 day past"
+             else                       "#{diff.total_days.to_i} days past"
+             end
+
+      issue_for node, MSG_LATE % {admonition, past}
+    end
+  end
+end

src/ameba/rule/performance/minmax_after_map.cr

@@ -51,7 +51,7 @@ module Ameba::Rule::Performance
       issue_for name_location, end_location, message do |corrector|
         next unless node_name_location = node.name_location
 
-        # FIXME: switching the order of the below calls breaks the corrector
+        # TODO: switching the order of the below calls breaks the corrector
         corrector.replace(
           name_location,
           name_location.adjust(column_number: {{ "map".size - 1 }}),

src/ameba/spec/expect_issue.cr

@@ -109,7 +109,7 @@ module Ameba::Spec::ExpectIssue
     code = lines.join('\n')
 
     if code == annotated_code
-      raise "Use `report_no_issues` to assert that no issues are found"
+      raise "Use `expect_no_issues` to assert that no issues are found"
     end
 
     source, actual_annotations = actual_annotations(rules, code, path, lines)