From 23c61e04c073cfe5cec1d08aa0aecd768df178de Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 15 Jun 2023 03:14:47 +0200 Subject: [PATCH 1/6] Add `Lint/DocumentationAdmonition` rule --- .../lint/documentation_admonition_spec.cr | 113 ++++++++++++++++++ .../rule/lint/documentation_admonition.cr | 96 +++++++++++++++ 2 files changed, 209 insertions(+) create mode 100644 spec/ameba/rule/lint/documentation_admonition_spec.cr create mode 100644 src/ameba/rule/lint/documentation_admonition.cr diff --git a/spec/ameba/rule/lint/documentation_admonition_spec.cr b/spec/ameba/rule/lint/documentation_admonition_spec.cr new file mode 100644 index 000000000..e07d0613f --- /dev/null +++ b/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 diff --git a/src/ameba/rule/lint/documentation_admonition.cr b/src/ameba/rule/lint/documentation_admonition.cr new file mode 100644 index 000000000..726c96165 --- /dev/null +++ b/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*(?#{Regex.union(admonitions)})(?:\((?.+?)\))?(?:\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 From 1b342e825769b6a82c4e572db94b56459ad8b6a7 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 6 Nov 2023 08:53:22 +0100 Subject: [PATCH 2/6] Make `TODOFormatter`'s configuration file path configurable Fixes the case where formatter specs were deleting project's `.ameba.yml` file --- spec/ameba/formatter/todo_formatter_spec.cr | 8 +++++--- src/ameba/formatter/todo_formatter.cr | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/ameba/formatter/todo_formatter_spec.cr b/spec/ameba/formatter/todo_formatter_spec.cr index 99ad39492..cc7dd32c2 100644 --- a/spec/ameba/formatter/todo_formatter_spec.cr +++ b/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 diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index 8bcc3d5c4..3e6efee37 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/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}" From bdbb79f1fa63768cf81f3b0860b2ec023347ae80 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 6 Nov 2023 08:54:12 +0100 Subject: [PATCH 3/6] Fix nonexistent method name used in error message --- src/ameba/spec/expect_issue.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr index 3cf1933ab..dc5cfee22 100644 --- a/src/ameba/spec/expect_issue.cr +++ b/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) From d5ac394d197137dd7d48478cea015b70e1ff732b Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 6 Nov 2023 14:55:48 +0100 Subject: [PATCH 4/6] Switch only `FIXME` comment to `TODO` --- src/ameba/rule/performance/minmax_after_map.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/rule/performance/minmax_after_map.cr b/src/ameba/rule/performance/minmax_after_map.cr index 1ed54158e..248c34210 100644 --- a/src/ameba/rule/performance/minmax_after_map.cr +++ b/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 }}), From c2b5e9449c3f034ec7f8bf885f3770409dd50fd5 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 6 Nov 2023 14:56:12 +0100 Subject: [PATCH 5/6] Do not report `TODO` admonitions --- .ameba.yml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .ameba.yml diff --git a/.ameba.yml b/.ameba.yml new file mode 100644 index 000000000..083323678 --- /dev/null +++ b/.ameba.yml @@ -0,0 +1,3 @@ +Lint/DocumentationAdmonition: + Timezone: UTC + Admonitions: [FIXME, BUG] From 1fccbfc8b8633100647d793c8fb6f0a6c6be3224 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 6 Nov 2023 15:02:54 +0100 Subject: [PATCH 6/6] Set timezone to `UTC` in CI --- .github/workflows/ci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97844cb72..751399341 100644 --- a/.github/workflows/ci.yml +++ b/.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: