From 7c8b4860b1cf4393d30392966b4676cfb319edee Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 16:40:41 -0400 Subject: [PATCH 01/15] Add `context` attribute to link `TODO`s with GitHub issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR introduces an optional `context` attribute for TODO comments that allows linking them to GitHub issues using the `issue()` function. This provides better traceability between code `TODO`s and their corresponding GitHub issues. ## Key Features - **New `context` attribute**: Can be added to any `TODO` with an event trigger - **`issue()` function**: Takes 3 parameters (org, repo, issue_number) - **Rich context information**: When a `TODO` is triggered, the linked issue's title, state, and assignee are included in the notification - **Works with all events**: Compatible with `date`, `gem_release`, `gem_bump`, and `ruby_version` events (not with `issue_close`/`pull_request_close` as they already reference issues/PRs) ## Example Usage ```ruby # TODO(on: date('2025-02-01'), to: 'team@example.com', context: issue('shopify', 'smart_todo', '108')) # Implement the caching strategy discussed in the issue ``` ## Implementation Details - Context validation follows the same pattern as existing event validations - The smart_todo_cop validates context syntax and arguments - Context information is fetched only when the `TODO` event is triggered - If the issue cannot be fetched, the `TODO` notification is still sent without context Closes #108 πŸ€– Generated with Claude Code --- README.md | 8 + lib/smart_todo/cli.rb | 12 +- lib/smart_todo/events.rb | 26 ++++ lib/smart_todo/todo.rb | 14 ++ lib/smart_todo_cop.rb | 12 ++ test/smart_todo/events/issue_context_test.rb | 147 +++++++++++++++++++ test/smart_todo/integration_test.rb | 81 ++++++++++ test/smart_todo/smart_todo_cop_test.rb | 61 ++++++++ 8 files changed, 360 insertions(+), 1 deletion(-) create mode 100644 test/smart_todo/events/issue_context_test.rb diff --git a/README.md b/README.md index 48e257e..5d2eded 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,14 @@ When the TODO's event is met (i.e. a certain date is reached), the TODO's assign end ``` +You can also add context to your TODOs by linking them to GitHub issues: +```ruby + # TODO(on: date('2025-01-01'), to: 'team@example.com', context: issue('shopify', 'smart_todo', '108')) + # Implement the caching strategy discussed in the issue + def process_order + end +``` + Documentation ---------------- Please check out the GitHub [wiki](https://github.com/Shopify/smart_todo/wiki) for documentation and example on how to setup SmartTodo in your project. diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index 0d73923..0d6cbb7 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -106,7 +106,17 @@ def process_todos(todos) end @errors.concat(todo.errors) - dispatches << [event_message, todo] if event_met + + if event_met + # Append context information if present + if todo.context + org, repo, issue_number = todo.context.arguments + context_message = events.issue_context(org, repo, issue_number) + event_message = "#{event_message}\n\n#{context_message}" if context_message + end + + dispatches << [event_message, todo] + end end dispatches diff --git a/lib/smart_todo/events.rb b/lib/smart_todo/events.rb index fb21fb5..7ea9c1f 100644 --- a/lib/smart_todo/events.rb +++ b/lib/smart_todo/events.rb @@ -157,6 +157,32 @@ def pull_request_close(organization, repo, pr_number) end end + # Retrieve context information for an issue + # This is used when a TODO has a context: issue() attribute + # + # @param organization [String] the GitHub organization name + # @param repo [String] the GitHub repo name + # @param issue_number [String, Integer] + # @return [String, nil] + def issue_context(organization, repo, issue_number) + headers = github_headers(organization, repo) + response = github_client.get("/repos/#{organization}/#{repo}/issues/#{issue_number}", headers) + + if response.code_type < Net::HTTPClientError + return nil + else + issue = JSON.parse(response.body) + state = issue["state"] + title = issue["title"] + assignee = issue["assignee"] ? "@#{issue["assignee"]["login"]}" : "unassigned" + + "πŸ“Œ Context: Issue ##{issue_number} - \"#{title}\" [#{state}] (#{assignee}) - " \ + "https://github.com/#{organization}/#{repo}/issues/#{issue_number}" + end + rescue + nil + end + # Check if the installed ruby version meets requirements. # # @param requirements [Array] a list of version specifiers diff --git a/lib/smart_todo/todo.rb b/lib/smart_todo/todo.rb index 51bdcb2..8571328 100644 --- a/lib/smart_todo/todo.rb +++ b/lib/smart_todo/todo.rb @@ -4,6 +4,7 @@ module SmartTodo class Todo attr_reader :filepath, :comment, :indent attr_reader :events, :assignees, :errors + attr_accessor :context def initialize(source, filepath = "-e") @filepath = filepath @@ -12,6 +13,7 @@ def initialize(source, filepath = "-e") @events = [] @assignees = [] + @context = nil @errors = [] parse(source[(indent + 1)..]) @@ -66,6 +68,18 @@ def visit_keyword_hash_node(node) end when :to metadata.assignees << visit(element.value) + when :context + value = visit(element.value) + + if value.is_a?(CallNode) && value.method_name == :issue + if value.arguments.length == 3 && value.arguments.all? { |arg| arg.is_a?(String) } + metadata.context = value + else + metadata.errors << "Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number)" + end + else + metadata.errors << "Incorrect `:context` format: only issue() function is supported" + end end end end diff --git a/lib/smart_todo_cop.rb b/lib/smart_todo_cop.rb index c5bfbd7..82a49b9 100644 --- a/lib/smart_todo_cop.rb +++ b/lib/smart_todo_cop.rb @@ -38,6 +38,8 @@ def on_new_investigation add_offense(comment, message: "Invalid event assignee. This method only accepts strings. #{HELP}") elsif (invalid_events = validate_events(metadata.events)).any? add_offense(comment, message: "#{invalid_events.join(". ")}. #{HELP}") + elsif metadata.context && (context_error = validate_context(metadata.context)) + add_offense(comment, message: "#{context_error}. #{HELP}") end end end @@ -103,6 +105,16 @@ def validate_pull_request_close_args(args) validate_fixed_arity_args(args, 3, "pull_request_close", ["organization", "repo", "pr_number"]) end + # @param context [SmartTodo::Todo::CallNode] + # @return [String, nil] Returns error message if context is invalid, nil if valid + def validate_context(context) + if context.method_name != :issue + "Invalid context: only issue() function is supported" + else + validate_fixed_arity_args(context.arguments, 3, "context issue", ["organization", "repo", "issue_number"]) + end + end + # @param args [Array] # @return [String, nil] Returns error message if arguments are invalid, nil if valid def validate_gem_release_args(args) diff --git a/test/smart_todo/events/issue_context_test.rb b/test/smart_todo/events/issue_context_test.rb new file mode 100644 index 0000000..ad49bd4 --- /dev/null +++ b/test/smart_todo/events/issue_context_test.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require "test_helper" + +module SmartTodo + class Events + class IssueContextTest < Minitest::Test + def test_when_issue_exists_and_is_open + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Add support for caching", + number: 123, + assignee: { login: "johndoe" }, + )) + + expected = "πŸ“Œ Context: Issue #123 - \"Add support for caching\" [open] (@johndoe) - " \ + "https://github.com/rails/rails/issues/123" + + assert_equal(expected, issue_context("rails", "rails", "123")) + end + + def test_when_issue_exists_and_is_closed + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "closed", + title: "Fix memory leak", + number: 456, + assignee: { login: "janedoe" }, + )) + + expected = "πŸ“Œ Context: Issue #456 - \"Fix memory leak\" [closed] (@janedoe) - " \ + "https://github.com/shopify/smart_todo/issues/456" + + assert_equal(expected, issue_context("shopify", "smart_todo", "456")) + end + + def test_when_issue_has_no_assignee + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Improve documentation", + number: 789, + assignee: nil, + )) + + expected = "πŸ“Œ Context: Issue #789 - \"Improve documentation\" [open] (unassigned) - " \ + "https://github.com/org/repo/issues/789" + + assert_equal(expected, issue_context("org", "repo", "789")) + end + + def test_when_issue_does_not_exist + stub_request(:get, /api.github.com/) + .to_return(status: 404) + + assert_nil(issue_context("rails", "rails", "999")) + end + + def test_when_token_env_is_not_present + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Test issue", + number: 1, + assignee: nil, + )) + + result = issue_context("rails", "rails", "1") + assert(result.include?("πŸ“Œ Context: Issue #1")) + + assert_requested(:get, /api.github.com/) do |request| + assert(!request.headers.key?("Authorization")) + end + end + + def test_when_token_env_is_present + ENV[GITHUB_TOKEN] = "abc123" + + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Test issue", + number: 2, + assignee: nil, + )) + + result = issue_context("rails", "rails", "2") + assert(result.include?("πŸ“Œ Context: Issue #2")) + + assert_requested(:get, /api.github.com/) do |request| + assert_equal("token abc123", request.headers["Authorization"]) + end + ensure + ENV.delete(GITHUB_TOKEN) + end + + def test_when_organization_specific_token_is_present + ENV["#{GITHUB_TOKEN}__RAILS"] = "rails_token" + + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Test issue", + number: 3, + assignee: nil, + )) + + result = issue_context("rails", "rails", "3") + assert(result.include?("πŸ“Œ Context: Issue #3")) + + assert_requested(:get, /api.github.com/) do |request| + assert_equal("token rails_token", request.headers["Authorization"]) + end + ensure + ENV.delete("#{GITHUB_TOKEN}__RAILS") + end + + def test_when_repo_specific_token_is_present + ENV["#{GITHUB_TOKEN}__RAILS__RAILS"] = "rails_rails_token" + + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Test issue", + number: 4, + assignee: nil, + )) + + result = issue_context("rails", "rails", "4") + assert(result.include?("πŸ“Œ Context: Issue #4")) + + assert_requested(:get, /api.github.com/) do |request| + assert_equal("token rails_rails_token", request.headers["Authorization"]) + end + ensure + ENV.delete("#{GITHUB_TOKEN}__RAILS__RAILS") + end + + private + + def issue_context(*args) + Events.new.issue_context(*args) + end + end + end +end \ No newline at end of file diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index 0667a52..2346973 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -104,6 +104,87 @@ def hello ) end + def test_includes_context_when_date_is_met_with_issue_context + ruby_code = <<~EOM + # TODO(on: date('2015-03-01'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '123')) + # Implement the caching strategy discussed in the issue + def hello + end + EOM + + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "open", + title: "Add caching support", + number: 123, + assignee: { login: "developer" }, + )) + + generate_ruby_file(ruby_code) do |file| + run_cli(file) + end + + assert_slack_message_sent( + "Hello :wave:,", + "We are past the *2015-03-01* due date", + "πŸ“Œ Context: Issue #123", + "Add caching support", + "Implement the caching strategy discussed in the issue", + ) + end + + def test_includes_context_when_gem_release_is_met_with_issue_context + ruby_code = <<~EOM + # TODO(on: gem_release('rails', '> 5.1'), to: 'john@example.com', context: issue('rails', 'rails', '456')) + # Upgrade to new Rails version as per issue + def hello + end + EOM + + stub_request(:get, /rubygems.org/) + .to_return(body: JSON.dump([{ number: "5.1.1" }])) + + stub_request(:get, /api.github.com/) + .to_return(body: JSON.dump( + state: "closed", + title: "Rails upgrade needed", + number: 456, + assignee: nil, + )) + + generate_ruby_file(ruby_code) do |file| + run_cli(file) + end + + assert_slack_message_sent( + "Hello :wave:,", + "The gem *rails* was released to version *5.1.1*", + "πŸ“Œ Context: Issue #456", + "Rails upgrade needed", + "Upgrade to new Rails version as per issue", + ) + end + + def test_context_does_not_affect_todos_without_events_triggered + ruby_code = <<~EOM + # TODO(on: date('2099-12-31'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '789')) + # This TODO is not ready yet + def hello + end + EOM + + # Should not make any GitHub API calls since the event is not triggered + generate_ruby_file(ruby_code) do |file| + stub_slack_request + CLI.new.run([file.path, "--slack_token", "123", "--fallback_channel", '#general"', "--dispatcher", "slack"]) + end + + # No slack message should be sent + assert_not_requested(:post, /chat.postMessage/) + # No GitHub API call should be made for context + assert_not_requested(:get, /api.github.com/) + end + private def assert_slack_message_sent(*messages) diff --git a/test/smart_todo/smart_todo_cop_test.rb b/test/smart_todo/smart_todo_cop_test.rb index c87b963..ccf3012 100644 --- a/test/smart_todo/smart_todo_cop_test.rb +++ b/test/smart_todo/smart_todo_cop_test.rb @@ -487,6 +487,67 @@ def hello RUBY end + def test_does_not_add_offense_when_todo_has_valid_context_with_issue + expect_no_offense(<<~RUBY) + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '123')) + def hello + end + RUBY + end + + def test_does_not_add_offense_when_todo_has_context_with_different_events + expect_no_offense(<<~RUBY) + # TODO(on: gem_release('rails', '> 6.0'), to: 'john@example.com', context: issue('rails', 'rails', '456')) + def hello + end + + # TODO(on: gem_bump('rails', '> 7.0'), to: 'john@example.com', context: issue('rails', 'rails', '789')) + def hello + end + + # TODO(on: ruby_version('> 3.0'), to: 'john@example.com', context: issue('ruby', 'ruby', '123')) + def hello + end + RUBY + end + + def test_add_offense_when_todo_has_context_with_wrong_number_of_arguments + expect_offense(<<~RUBY) + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue('shopify', 'smart_todo')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number). For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + def hello + end + RUBY + end + + def test_add_offense_when_todo_has_context_with_non_string_arguments + expect_offense(<<~RUBY) + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue(123, 456, 789)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number). For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + def hello + end + RUBY + end + + def test_add_offense_when_todo_has_context_with_invalid_function + expect_offense(<<~RUBY) + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: pull_request('shopify', 'smart_todo', '123')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: only issue() function is supported. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + def hello + end + RUBY + end + + def test_context_does_not_require_assignee_when_event_does + # Even though context is present, the TODO still requires an assignee for the event + expect_offense(<<~RUBY) + # TODO(on: date('2019-08-04'), context: issue('shopify', 'smart_todo', '123')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{expected_message} + def hello + end + RUBY + end + private def expected_message From 39ccf2b93174b96c3b58143c3c2385104a1af20d Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 17:05:33 -0400 Subject: [PATCH 02/15] Add missing validation and documentation to ensure `context` isn't considered valid with `issue_close` and `pull_request_close` --- README.md | 13 ++++++++++++- lib/smart_todo/cli.rb | 12 ++++++++++-- lib/smart_todo_cop.rb | 11 +++++++++-- test/smart_todo/integration_test.rb | 25 +++++++++++++++++++++++++ test/smart_todo/smart_todo_cop_test.rb | 18 ++++++++++++++++++ 5 files changed, 74 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5d2eded..92c3687 100644 --- a/README.md +++ b/README.md @@ -40,14 +40,25 @@ When the TODO's event is met (i.e. a certain date is reached), the TODO's assign end ``` -You can also add context to your TODOs by linking them to GitHub issues: +You can also add context to your TODOs by linking them to GitHub issues. The `context` attribute +works with `date`, `gem_release`, `gem_bump`, and `ruby_version` events: + ```ruby # TODO(on: date('2025-01-01'), to: 'team@example.com', context: issue('shopify', 'smart_todo', '108')) # Implement the caching strategy discussed in the issue def process_order end + + # TODO(on: gem_release('rails', '> 7.2'), to: 'dev@example.com', context: issue('rails', 'rails', '456')) + # Upgrade to new Rails version as discussed in the issue + def legacy_method + end ``` +When the TODO is triggered, the linked issue's title, state, and assignee will be included in the notification. + +Note: The `context` attribute is not supported with `issue_close` or `pull_request_close` events as they already reference specific issues/PRs. + Documentation ---------------- Please check out the GitHub [wiki](https://github.com/Shopify/smart_todo/wiki) for documentation and example on how to setup SmartTodo in your project. diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index 0d6cbb7..ae18d98 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -108,8 +108,9 @@ def process_todos(todos) @errors.concat(todo.errors) if event_met - # Append context information if present - if todo.context + # Append context information if present (but not for issue_close or pull_request_close) + # These events already reference specific issues/PRs + if todo.context && should_apply_context?(event_met) org, repo, issue_number = todo.context.arguments context_message = events.issue_context(org, repo, issue_number) event_message = "#{event_message}\n\n#{context_message}" if context_message @@ -122,6 +123,13 @@ def process_todos(todos) dispatches end + private + + def should_apply_context?(event) + # Context should not be applied to events that already reference issues/PRs + ![:issue_close, :pull_request_close].include?(event.method_name) + end + def process_dispatches(dispatches) queue = Queue.new dispatches.each { |dispatch| queue << dispatch } diff --git a/lib/smart_todo_cop.rb b/lib/smart_todo_cop.rb index 82a49b9..e9ad249 100644 --- a/lib/smart_todo_cop.rb +++ b/lib/smart_todo_cop.rb @@ -38,7 +38,7 @@ def on_new_investigation add_offense(comment, message: "Invalid event assignee. This method only accepts strings. #{HELP}") elsif (invalid_events = validate_events(metadata.events)).any? add_offense(comment, message: "#{invalid_events.join(". ")}. #{HELP}") - elsif metadata.context && (context_error = validate_context(metadata.context)) + elsif metadata.context && (context_error = validate_context(metadata.context, metadata.events)) add_offense(comment, message: "#{context_error}. #{HELP}") end end @@ -106,8 +106,15 @@ def validate_pull_request_close_args(args) end # @param context [SmartTodo::Todo::CallNode] + # @param events [Array] # @return [String, nil] Returns error message if context is invalid, nil if valid - def validate_context(context) + def validate_context(context, events) + # Check if any event is issue_close or pull_request_close + restricted_events = events.select { |e| [:issue_close, :pull_request_close].include?(e.method_name) } + if restricted_events.any? + return "Invalid context: context attribute cannot be used with #{restricted_events.first.method_name} event" + end + if context.method_name != :issue "Invalid context: only issue() function is supported" else diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index 2346973..c0056f8 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -185,6 +185,31 @@ def hello assert_not_requested(:get, /api.github.com/) end + def test_context_ignored_with_issue_close_event + ruby_code = <<~EOM + # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '999')) + # This context should be ignored + def hello + end + EOM + + stub_request(:get, /api.github.com.*issues\/100/) + .to_return(body: JSON.dump(state: "closed")) + + generate_ruby_file(ruby_code) do |file| + run_cli(file) + end + + assert_slack_message_sent( + "Hello :wave:,", + "The issue https://github.com/shopify/smart_todo/issues/100 is now closed", + "This context should be ignored", + ) + + # Should NOT make a call for issue 999 (the context) + assert_not_requested(:get, /api.github.com.*issues\/999/) + end + private def assert_slack_message_sent(*messages) diff --git a/test/smart_todo/smart_todo_cop_test.rb b/test/smart_todo/smart_todo_cop_test.rb index ccf3012..39f275d 100644 --- a/test/smart_todo/smart_todo_cop_test.rb +++ b/test/smart_todo/smart_todo_cop_test.rb @@ -548,6 +548,24 @@ def hello RUBY end + def test_add_offense_when_todo_has_context_with_issue_close_event + expect_offense(<<~RUBY) + # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'dev@example.com', context: issue('shopify', 'smart_todo', '123')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with issue_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + def hello + end + RUBY + end + + def test_add_offense_when_todo_has_context_with_pull_request_close_event + expect_offense(<<~RUBY) + # TODO(on: pull_request_close('shopify', 'smart_todo', '200'), to: 'dev@example.com', context: issue('shopify', 'smart_todo', '123')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with pull_request_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + def hello + end + RUBY + end + private def expected_message From 8abc253945dbfaabefe085185f76693e7221455c Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 17:10:39 -0400 Subject: [PATCH 03/15] Fix broken tests --- lib/smart_todo/cli.rb | 20 ++++++++++---------- lib/smart_todo/events.rb | 2 +- lib/smart_todo/todo.rb | 3 ++- test/smart_todo/integration_test.rb | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index ae18d98..22995b4 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -107,17 +107,17 @@ def process_todos(todos) @errors.concat(todo.errors) - if event_met - # Append context information if present (but not for issue_close or pull_request_close) - # These events already reference specific issues/PRs - if todo.context && should_apply_context?(event_met) - org, repo, issue_number = todo.context.arguments - context_message = events.issue_context(org, repo, issue_number) - event_message = "#{event_message}\n\n#{context_message}" if context_message - end - - dispatches << [event_message, todo] + next unless event_met + + # Append context information if present (but not for issue_close or pull_request_close) + # These events already reference specific issues/PRs + if todo.context && should_apply_context?(event_met) + org, repo, issue_number = todo.context.arguments + context_message = events.issue_context(org, repo, issue_number) + event_message = "#{event_message}\n\n#{context_message}" if context_message end + + dispatches << [event_message, todo] end dispatches diff --git a/lib/smart_todo/events.rb b/lib/smart_todo/events.rb index 7ea9c1f..0241b4a 100644 --- a/lib/smart_todo/events.rb +++ b/lib/smart_todo/events.rb @@ -169,7 +169,7 @@ def issue_context(organization, repo, issue_number) response = github_client.get("/repos/#{organization}/#{repo}/issues/#{issue_number}", headers) if response.code_type < Net::HTTPClientError - return nil + nil else issue = JSON.parse(response.body) state = issue["state"] diff --git a/lib/smart_todo/todo.rb b/lib/smart_todo/todo.rb index 8571328..d6559ee 100644 --- a/lib/smart_todo/todo.rb +++ b/lib/smart_todo/todo.rb @@ -75,7 +75,8 @@ def visit_keyword_hash_node(node) if value.arguments.length == 3 && value.arguments.all? { |arg| arg.is_a?(String) } metadata.context = value else - metadata.errors << "Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number)" + metadata.errors << "Incorrect `:context` format: issue() requires exactly 3 string arguments " \ + "(org, repo, issue_number)" end else metadata.errors << "Incorrect `:context` format: only issue() function is supported" diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index c0056f8..682061b 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -193,7 +193,7 @@ def hello end EOM - stub_request(:get, /api.github.com.*issues\/100/) + stub_request(:get, %r{api.github.com.*issues/100}) .to_return(body: JSON.dump(state: "closed")) generate_ruby_file(ruby_code) do |file| @@ -207,7 +207,7 @@ def hello ) # Should NOT make a call for issue 999 (the context) - assert_not_requested(:get, /api.github.com.*issues\/999/) + assert_not_requested(:get, %r{api.github.com.*issues/999}) end private From c9e8887c460503b969d9b68c2c58f1e4c6cb9ce3 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 17:11:59 -0400 Subject: [PATCH 04/15] Added missing newline --- test/smart_todo/events/issue_context_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/smart_todo/events/issue_context_test.rb b/test/smart_todo/events/issue_context_test.rb index ad49bd4..d7e755c 100644 --- a/test/smart_todo/events/issue_context_test.rb +++ b/test/smart_todo/events/issue_context_test.rb @@ -144,4 +144,4 @@ def issue_context(*args) end end end -end \ No newline at end of file +end From e66af5a2896612a0346a1caf56039112c3bd36a8 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 18:18:31 -0400 Subject: [PATCH 05/15] Got rid of clanker-provided comments I should have removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rafael MendonΓ§a FranΓ§a --- test/smart_todo/integration_test.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index 682061b..877c57f 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -173,15 +173,12 @@ def hello end EOM - # Should not make any GitHub API calls since the event is not triggered generate_ruby_file(ruby_code) do |file| stub_slack_request CLI.new.run([file.path, "--slack_token", "123", "--fallback_channel", '#general"', "--dispatcher", "slack"]) end - # No slack message should be sent assert_not_requested(:post, /chat.postMessage/) - # No GitHub API call should be made for context assert_not_requested(:get, /api.github.com/) end @@ -206,7 +203,6 @@ def hello "This context should be ignored", ) - # Should NOT make a call for issue 999 (the context) assert_not_requested(:get, %r{api.github.com.*issues/999}) end From 532eceee856f39d8c37e3d2cfbf07e7afa805321 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Thu, 23 Oct 2025 21:59:43 -0400 Subject: [PATCH 06/15] PR feedback: re-organize the code and remove some comments Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2457422360 and https://github.com/Shopify/smart_todo/pull/107#discussion_r2457425785 --- lib/smart_todo/cli.rb | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index 22995b4..e224f06 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -109,13 +109,7 @@ def process_todos(todos) next unless event_met - # Append context information if present (but not for issue_close or pull_request_close) - # These events already reference specific issues/PRs - if todo.context && should_apply_context?(event_met) - org, repo, issue_number = todo.context.arguments - context_message = events.issue_context(org, repo, issue_number) - event_message = "#{event_message}\n\n#{context_message}" if context_message - end + event_message = append_context_if_applicable(event_message, todo, event_met, events) dispatches << [event_message, todo] end @@ -125,9 +119,27 @@ def process_todos(todos) private - def should_apply_context?(event) - # Context should not be applied to events that already reference issues/PRs - ![:issue_close, :pull_request_close].include?(event.method_name) + # @param event_message [String] the original event message + # @param todo [Todo] the todo object that may contain context + # @param event [Event] the event that was met + # @param events [Events] the events instance for fetching issue context + # @return [String] the event message, potentially with context appended + def append_context_if_applicable(event_message, todo, event, events) + return event_message unless should_apply_context?(todo, event) + + org, repo, issue_number = todo.context.arguments + context_message = events.issue_context(org, repo, issue_number) + + context_message ? "#{event_message}\n\n#{context_message}" : event_message + end + + # @param todo [Todo] the todo object to check for context + # @param event [Event] the event to check + # @return [Boolean] true if context should be applied, false otherwise + def should_apply_context?(todo, event) + # Context should not be applied to events that already reference specific issues/PRs + # (like issue_close or pull_request_close) since they already have that context. + todo.context && ![:issue_close, :pull_request_close].include?(event.method_name) end def process_dispatches(dispatches) From 2090a294099b9775941a7f437e743b1f7243b7cb Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Fri, 24 Oct 2025 08:22:17 -0400 Subject: [PATCH 07/15] PR feedback: organize in a closer approach to other blocks above At the same time, I refactored to avoid code duplication and added tests Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2457444516 --- lib/smart_todo/cli.rb | 2 +- lib/smart_todo/todo.rb | 16 +++++++ lib/smart_todo_cop.rb | 30 +++++++----- test/smart_todo/cli_test.rb | 47 +++++++++++++++++++ test/smart_todo/smart_todo_cop_test.rb | 64 ++++++++++++++++++++++++++ test/smart_todo/todo_test.rb | 36 +++++++++++++++ 6 files changed, 183 insertions(+), 12 deletions(-) create mode 100644 test/smart_todo/todo_test.rb diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index e224f06..30a03a1 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -139,7 +139,7 @@ def append_context_if_applicable(event_message, todo, event, events) def should_apply_context?(todo, event) # Context should not be applied to events that already reference specific issues/PRs # (like issue_close or pull_request_close) since they already have that context. - todo.context && ![:issue_close, :pull_request_close].include?(event.method_name) + todo.context && Todo.event_can_use_context?(event.method_name) end def process_dispatches(dispatches) diff --git a/lib/smart_todo/todo.rb b/lib/smart_todo/todo.rb index d6559ee..5dac9d5 100644 --- a/lib/smart_todo/todo.rb +++ b/lib/smart_todo/todo.rb @@ -6,6 +6,22 @@ class Todo attr_reader :events, :assignees, :errors attr_accessor :context + # Events that already contain issue/PR context and therefore + # should not have additional context applied + EVENTS_WITH_IMPLICIT_CONTEXT = [:issue_close, :pull_request_close].freeze + + class << self + # Check if an event is eligible to have context information applied. + # Events like issue_close and pull_request_close already reference + # specific issues/PRs and shouldn't have additional context. + # + # @param event_name [Symbol] the name of the event method + # @return [Boolean] true if the event can use context, false otherwise + def event_can_use_context?(event_name) + !EVENTS_WITH_IMPLICIT_CONTEXT.include?(event_name.to_sym) + end + end + def initialize(source, filepath = "-e") @filepath = filepath @comment = +"" diff --git a/lib/smart_todo_cop.rb b/lib/smart_todo_cop.rb index e9ad249..b217b13 100644 --- a/lib/smart_todo_cop.rb +++ b/lib/smart_todo_cop.rb @@ -38,8 +38,8 @@ def on_new_investigation add_offense(comment, message: "Invalid event assignee. This method only accepts strings. #{HELP}") elsif (invalid_events = validate_events(metadata.events)).any? add_offense(comment, message: "#{invalid_events.join(". ")}. #{HELP}") - elsif metadata.context && (context_error = validate_context(metadata.context, metadata.events)) - add_offense(comment, message: "#{context_error}. #{HELP}") + elsif (context_errors = validate_context(metadata)).any? + add_offense(comment, message: "#{context_errors.join(". ")}. #{HELP}") end end end @@ -105,20 +105,28 @@ def validate_pull_request_close_args(args) validate_fixed_arity_args(args, 3, "pull_request_close", ["organization", "repo", "pr_number"]) end - # @param context [SmartTodo::Todo::CallNode] - # @param events [Array] - # @return [String, nil] Returns error message if context is invalid, nil if valid - def validate_context(context, events) - # Check if any event is issue_close or pull_request_close - restricted_events = events.select { |e| [:issue_close, :pull_request_close].include?(e.method_name) } + # @param metadata [SmartTodo::Parser::Visitor] The metadata containing context and events + # @return [Array] Returns array of error messages, empty if valid + def validate_context(metadata) + return [] unless metadata.context + + context = metadata.context + events = metadata.events + + restricted_events = events.reject { |e| ::SmartTodo::Todo.event_can_use_context?(e.method_name) } if restricted_events.any? - return "Invalid context: context attribute cannot be used with #{restricted_events.first.method_name} event" + event_name = restricted_events.first.method_name + return ["Invalid context: context attribute cannot be used with #{event_name} event"] end if context.method_name != :issue - "Invalid context: only issue() function is supported" + ["Invalid context: only issue() function is supported"] + elsif (error = validate_fixed_arity_args( + context.arguments, 3, "context issue", ["organization", "repo", "issue_number"] + )) + [error] else - validate_fixed_arity_args(context.arguments, 3, "context issue", ["organization", "repo", "issue_number"]) + [] end end diff --git a/test/smart_todo/cli_test.rb b/test/smart_todo/cli_test.rb index 92eddee..3888393 100644 --- a/test/smart_todo/cli_test.rb +++ b/test/smart_todo/cli_test.rb @@ -150,5 +150,52 @@ def hello end end end + + def test_should_apply_context_returns_false_without_context + cli = CLI.new + todo = Todo.new("# TODO(on: date('2015-03-01'), to: 'john@example.com')") + event = Todo::CallNode.new(:date, ["2015-03-01"], nil) + + refute(cli.send(:should_apply_context?, todo, event)) + end + + def test_should_apply_context_returns_false_for_issue_close_event + cli = CLI.new + todo = Todo.new("# TODO(on: issue_close('org', 'repo', '123'), to: 'john@example.com')") + todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + event = Todo::CallNode.new(:issue_close, ["org", "repo", "123"], nil) + + refute(cli.send(:should_apply_context?, todo, event)) + end + + def test_should_apply_context_returns_false_for_pull_request_close_event + cli = CLI.new + todo = Todo.new("# TODO(on: pull_request_close('org', 'repo', '123'), to: 'john@example.com')") + todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + event = Todo::CallNode.new(:pull_request_close, ["org", "repo", "123"], nil) + + refute(cli.send(:should_apply_context?, todo, event)) + end + + def test_should_apply_context_returns_true_for_regular_event_with_context + cli = CLI.new + todo = Todo.new("# TODO(on: date('2015-03-01'), to: 'john@example.com')") + todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + event = Todo::CallNode.new(:date, ["2015-03-01"], nil) + + assert(cli.send(:should_apply_context?, todo, event)) + end + + def test_append_context_if_applicable_returns_original_message_when_context_not_applicable + cli = CLI.new + todo = Todo.new("# TODO(on: issue_close('org', 'repo', '123'), to: 'john@example.com')") + todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + event = Todo::CallNode.new(:issue_close, ["org", "repo", "123"], nil) + events = Events.new + original_message = "Original event message" + + result = cli.send(:append_context_if_applicable, original_message, todo, event, events) + assert_equal(original_message, result) + end end end diff --git a/test/smart_todo/smart_todo_cop_test.rb b/test/smart_todo/smart_todo_cop_test.rb index 39f275d..4dd9860 100644 --- a/test/smart_todo/smart_todo_cop_test.rb +++ b/test/smart_todo/smart_todo_cop_test.rb @@ -593,5 +593,69 @@ def investigate(source, ruby_version = 2.5, file = "(file)") def cop @cop ||= RuboCop::Cop::SmartTodo::SmartTodoCop.new end + + def test_validate_context_returns_empty_array_when_no_context + metadata = SmartTodo::Parser::Visitor.new + metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) + + assert_equal([], cop.send(:validate_context, metadata)) + end + + def test_validate_context_returns_error_for_issue_close_event_with_context + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:issue_close, ["org", "repo", "456"], nil) + + result = cop.send(:validate_context, metadata) + assert_equal(1, result.length) + assert_match(/context attribute cannot be used with issue_close event/, result.first) + end + + def test_validate_context_returns_error_for_pull_request_close_event_with_context + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:pull_request_close, ["org", "repo", "456"], nil) + + result = cop.send(:validate_context, metadata) + assert_equal(1, result.length) + assert_match(/context attribute cannot be used with pull_request_close event/, result.first) + end + + def test_validate_context_returns_error_for_non_issue_context + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:custom_function, ["arg1", "arg2"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) + + result = cop.send(:validate_context, metadata) + assert_equal(1, result.length) + assert_match(/only issue\(\) function is supported/, result.first) + end + + def test_validate_context_returns_error_for_wrong_number_of_arguments + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) + + result = cop.send(:validate_context, metadata) + assert_equal(1, result.length) + assert_match(/wrong number of arguments/, result.first) + end + + def test_validate_context_returns_empty_array_for_valid_context + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) + + assert_equal([], cop.send(:validate_context, metadata)) + end + + def test_validate_context_returns_empty_array_for_multiple_valid_events + metadata = SmartTodo::Parser::Visitor.new + metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) + metadata.events << SmartTodo::Todo::CallNode.new(:gem_release, ["rails", "> 5.0"], nil) + + assert_equal([], cop.send(:validate_context, metadata)) + end end end diff --git a/test/smart_todo/todo_test.rb b/test/smart_todo/todo_test.rb new file mode 100644 index 0000000..756457d --- /dev/null +++ b/test/smart_todo/todo_test.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require "test_helper" + +module SmartTodo + class TodoTest < Minitest::Test + def test_event_can_use_context_returns_true_for_regular_events + assert(Todo.event_can_use_context?(:date)) + assert(Todo.event_can_use_context?(:gem_release)) + assert(Todo.event_can_use_context?(:gem_bump)) + assert(Todo.event_can_use_context?(:custom_event)) + end + + def test_event_can_use_context_returns_false_for_issue_close + refute(Todo.event_can_use_context?(:issue_close)) + end + + def test_event_can_use_context_returns_false_for_pull_request_close + refute(Todo.event_can_use_context?(:pull_request_close)) + end + + def test_event_can_use_context_handles_string_input + assert(Todo.event_can_use_context?("date")) + refute(Todo.event_can_use_context?("issue_close")) + refute(Todo.event_can_use_context?("pull_request_close")) + end + + def test_events_with_implicit_context_constant_is_frozen + assert(Todo::EVENTS_WITH_IMPLICIT_CONTEXT.frozen?) + end + + def test_events_with_implicit_context_contains_expected_events + assert_equal([:issue_close, :pull_request_close], Todo::EVENTS_WITH_IMPLICIT_CONTEXT) + end + end +end \ No newline at end of file From 5d196bd985dc2fac01d146a70280a02007e1ae66 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Fri, 24 Oct 2025 08:37:16 -0400 Subject: [PATCH 08/15] Remove comment that belongs somewhere else --- lib/smart_todo/cli.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index 30a03a1..15719a0 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -137,8 +137,6 @@ def append_context_if_applicable(event_message, todo, event, events) # @param event [Event] the event to check # @return [Boolean] true if context should be applied, false otherwise def should_apply_context?(todo, event) - # Context should not be applied to events that already reference specific issues/PRs - # (like issue_close or pull_request_close) since they already have that context. todo.context && Todo.event_can_use_context?(event.method_name) end From 7a0a3cf3a3fe76a81ccb93e801228ac5165d84be Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Fri, 24 Oct 2025 08:40:14 -0400 Subject: [PATCH 09/15] Add missing newline --- test/smart_todo/todo_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/smart_todo/todo_test.rb b/test/smart_todo/todo_test.rb index 756457d..78ca725 100644 --- a/test/smart_todo/todo_test.rb +++ b/test/smart_todo/todo_test.rb @@ -33,4 +33,4 @@ def test_events_with_implicit_context_contains_expected_events assert_equal([:issue_close, :pull_request_close], Todo::EVENTS_WITH_IMPLICIT_CONTEXT) end end -end \ No newline at end of file +end From d030ac6bfc5b629eb489a21916f40942ce2e3f6a Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Tue, 28 Oct 2025 08:34:13 -0400 Subject: [PATCH 10/15] PR feedback: use a string format for `context` instead of using a more complex construct that makes it harder for humans to process Refs: https://github.com/Shopify/smart_todo/pull/107#issuecomment-3443696612 and https://github.com/Shopify/smart_todo/pull/107#issuecomment-3444195662 --- README.md | 4 +- lib/smart_todo/todo.rb | 25 +++++++++---- lib/smart_todo_cop.rb | 11 +----- test/smart_todo/integration_test.rb | 8 ++-- test/smart_todo/smart_todo_cop_test.rb | 52 ++++++++------------------ 5 files changed, 40 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 92c3687..d3824ab 100644 --- a/README.md +++ b/README.md @@ -44,12 +44,12 @@ You can also add context to your TODOs by linking them to GitHub issues. The `co works with `date`, `gem_release`, `gem_bump`, and `ruby_version` events: ```ruby - # TODO(on: date('2025-01-01'), to: 'team@example.com', context: issue('shopify', 'smart_todo', '108')) + # TODO(on: date('2025-01-01'), to: 'team@example.com', context: "shopify/smart_todo#108") # Implement the caching strategy discussed in the issue def process_order end - # TODO(on: gem_release('rails', '> 7.2'), to: 'dev@example.com', context: issue('rails', 'rails', '456')) + # TODO(on: gem_release('rails', '> 7.2'), to: 'dev@example.com', context: "rails/rails#456") # Upgrade to new Rails version as discussed in the issue def legacy_method end diff --git a/lib/smart_todo/todo.rb b/lib/smart_todo/todo.rb index 5dac9d5..5b54627 100644 --- a/lib/smart_todo/todo.rb +++ b/lib/smart_todo/todo.rb @@ -87,15 +87,24 @@ def visit_keyword_hash_node(node) when :context value = visit(element.value) - if value.is_a?(CallNode) && value.method_name == :issue - if value.arguments.length == 3 && value.arguments.all? { |arg| arg.is_a?(String) } - metadata.context = value - else - metadata.errors << "Incorrect `:context` format: issue() requires exactly 3 string arguments " \ - "(org, repo, issue_number)" - end + unless value.is_a?(String) + metadata.errors << "Incorrect `:context` format: expected string value" + next + end + + unless value =~ %r{^([^/]+)/([^#]+)#(\d+)$} + metadata.errors << "Incorrect `:context` format: expected \"org/repo#issue_number\"" + next + end + + org = ::Regexp.last_match(1) + repo = ::Regexp.last_match(2) + issue_number = ::Regexp.last_match(3) + + if org.empty? || repo.empty? + metadata.errors << "Incorrect `:context` format: org and repo cannot be empty" else - metadata.errors << "Incorrect `:context` format: only issue() function is supported" + metadata.context = CallNode.new(:issue, [org, repo, issue_number], element.value.location) end end end diff --git a/lib/smart_todo_cop.rb b/lib/smart_todo_cop.rb index b217b13..42553a3 100644 --- a/lib/smart_todo_cop.rb +++ b/lib/smart_todo_cop.rb @@ -110,7 +110,6 @@ def validate_pull_request_close_args(args) def validate_context(metadata) return [] unless metadata.context - context = metadata.context events = metadata.events restricted_events = events.reject { |e| ::SmartTodo::Todo.event_can_use_context?(e.method_name) } @@ -119,15 +118,7 @@ def validate_context(metadata) return ["Invalid context: context attribute cannot be used with #{event_name} event"] end - if context.method_name != :issue - ["Invalid context: only issue() function is supported"] - elsif (error = validate_fixed_arity_args( - context.arguments, 3, "context issue", ["organization", "repo", "issue_number"] - )) - [error] - else - [] - end + [] end # @param args [Array] diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index 877c57f..54e57b8 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -106,7 +106,7 @@ def hello def test_includes_context_when_date_is_met_with_issue_context ruby_code = <<~EOM - # TODO(on: date('2015-03-01'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '123')) + # TODO(on: date('2015-03-01'), to: 'john@example.com', context: "shopify/smart_todo#123") # Implement the caching strategy discussed in the issue def hello end @@ -135,7 +135,7 @@ def hello def test_includes_context_when_gem_release_is_met_with_issue_context ruby_code = <<~EOM - # TODO(on: gem_release('rails', '> 5.1'), to: 'john@example.com', context: issue('rails', 'rails', '456')) + # TODO(on: gem_release('rails', '> 5.1'), to: 'john@example.com', context: "rails/rails#456") # Upgrade to new Rails version as per issue def hello end @@ -167,7 +167,7 @@ def hello def test_context_does_not_affect_todos_without_events_triggered ruby_code = <<~EOM - # TODO(on: date('2099-12-31'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '789')) + # TODO(on: date('2099-12-31'), to: 'john@example.com', context: "shopify/smart_todo#789") # This TODO is not ready yet def hello end @@ -184,7 +184,7 @@ def hello def test_context_ignored_with_issue_close_event ruby_code = <<~EOM - # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '999')) + # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'john@example.com', context: "shopify/smart_todo#999") # This context should be ignored def hello end diff --git a/test/smart_todo/smart_todo_cop_test.rb b/test/smart_todo/smart_todo_cop_test.rb index 4dd9860..a88e01e 100644 --- a/test/smart_todo/smart_todo_cop_test.rb +++ b/test/smart_todo/smart_todo_cop_test.rb @@ -489,7 +489,7 @@ def hello def test_does_not_add_offense_when_todo_has_valid_context_with_issue expect_no_offense(<<~RUBY) - # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue('shopify', 'smart_todo', '123')) + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: "shopify/smart_todo#123") def hello end RUBY @@ -497,15 +497,15 @@ def hello def test_does_not_add_offense_when_todo_has_context_with_different_events expect_no_offense(<<~RUBY) - # TODO(on: gem_release('rails', '> 6.0'), to: 'john@example.com', context: issue('rails', 'rails', '456')) + # TODO(on: gem_release('rails', '> 6.0'), to: 'john@example.com', context: "rails/rails#456") def hello end - # TODO(on: gem_bump('rails', '> 7.0'), to: 'john@example.com', context: issue('rails', 'rails', '789')) + # TODO(on: gem_bump('rails', '> 7.0'), to: 'john@example.com', context: "rails/rails#789") def hello end - # TODO(on: ruby_version('> 3.0'), to: 'john@example.com', context: issue('ruby', 'ruby', '123')) + # TODO(on: ruby_version('> 3.0'), to: 'john@example.com', context: "ruby/ruby#123") def hello end RUBY @@ -513,8 +513,8 @@ def hello def test_add_offense_when_todo_has_context_with_wrong_number_of_arguments expect_offense(<<~RUBY) - # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue('shopify', 'smart_todo')) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number). For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: "shopify/smart_todo") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: expected "org/repo#issue_number". For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax def hello end RUBY @@ -522,8 +522,8 @@ def hello def test_add_offense_when_todo_has_context_with_non_string_arguments expect_offense(<<~RUBY) - # TODO(on: date('2019-08-04'), to: 'john@example.com', context: issue(123, 456, 789)) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: issue() requires exactly 3 string arguments (org, repo, issue_number). For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: 123) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: expected string value. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax def hello end RUBY @@ -531,8 +531,8 @@ def hello def test_add_offense_when_todo_has_context_with_invalid_function expect_offense(<<~RUBY) - # TODO(on: date('2019-08-04'), to: 'john@example.com', context: pull_request('shopify', 'smart_todo', '123')) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: only issue() function is supported. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + # TODO(on: date('2019-08-04'), to: 'john@example.com', context: "shopify-smart_todo-123") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid TODO format: Incorrect `:context` format: expected "org/repo#issue_number". For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax def hello end RUBY @@ -541,8 +541,8 @@ def hello def test_context_does_not_require_assignee_when_event_does # Even though context is present, the TODO still requires an assignee for the event expect_offense(<<~RUBY) - # TODO(on: date('2019-08-04'), context: issue('shopify', 'smart_todo', '123')) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{expected_message} + # TODO(on: date('2019-08-04'), context: "shopify/smart_todo#123") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{expected_message} def hello end RUBY @@ -550,8 +550,8 @@ def hello def test_add_offense_when_todo_has_context_with_issue_close_event expect_offense(<<~RUBY) - # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'dev@example.com', context: issue('shopify', 'smart_todo', '123')) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with issue_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'dev@example.com', context: "shopify/smart_todo#123") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with issue_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax def hello end RUBY @@ -559,8 +559,8 @@ def hello def test_add_offense_when_todo_has_context_with_pull_request_close_event expect_offense(<<~RUBY) - # TODO(on: pull_request_close('shopify', 'smart_todo', '200'), to: 'dev@example.com', context: issue('shopify', 'smart_todo', '123')) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with pull_request_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax + # TODO(on: pull_request_close('shopify', 'smart_todo', '200'), to: 'dev@example.com', context: "shopify/smart_todo#123") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with pull_request_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax def hello end RUBY @@ -621,26 +621,6 @@ def test_validate_context_returns_error_for_pull_request_close_event_with_contex assert_match(/context attribute cannot be used with pull_request_close event/, result.first) end - def test_validate_context_returns_error_for_non_issue_context - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:custom_function, ["arg1", "arg2"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) - - result = cop.send(:validate_context, metadata) - assert_equal(1, result.length) - assert_match(/only issue\(\) function is supported/, result.first) - end - - def test_validate_context_returns_error_for_wrong_number_of_arguments - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) - - result = cop.send(:validate_context, metadata) - assert_equal(1, result.length) - assert_match(/wrong number of arguments/, result.first) - end - def test_validate_context_returns_empty_array_for_valid_context metadata = SmartTodo::Parser::Visitor.new metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) From 6bd985a220973e7111d8f178b0d525c585a7365c Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Tue, 28 Oct 2025 08:38:07 -0400 Subject: [PATCH 11/15] Make rubocop happy --- test/smart_todo/todo_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/smart_todo/todo_test.rb b/test/smart_todo/todo_test.rb index 756457d..78ca725 100644 --- a/test/smart_todo/todo_test.rb +++ b/test/smart_todo/todo_test.rb @@ -33,4 +33,4 @@ def test_events_with_implicit_context_contains_expected_events assert_equal([:issue_close, :pull_request_close], Todo::EVENTS_WITH_IMPLICIT_CONTEXT) end end -end \ No newline at end of file +end From 57477e8487bc1aeaae0313bd85ec1978839d267e Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Tue, 28 Oct 2025 15:09:57 -0400 Subject: [PATCH 12/15] PR feedback: remove restriction on `context`; it can be added to any `smart_todo` Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2469489029 --- README.md | 9 ++-- lib/smart_todo/cli.rb | 2 +- lib/smart_todo/todo.rb | 16 ------- lib/smart_todo_cop.rb | 18 -------- test/smart_todo/cli_test.rb | 33 +++++--------- test/smart_todo/integration_test.rb | 17 ++++--- test/smart_todo/smart_todo_cop_test.rb | 62 -------------------------- test/smart_todo/todo_test.rb | 36 --------------- 8 files changed, 30 insertions(+), 163 deletions(-) delete mode 100644 test/smart_todo/todo_test.rb diff --git a/README.md b/README.md index d3824ab..b190b19 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ When the TODO's event is met (i.e. a certain date is reached), the TODO's assign ``` You can also add context to your TODOs by linking them to GitHub issues. The `context` attribute -works with `date`, `gem_release`, `gem_bump`, and `ruby_version` events: +works with all events: ```ruby # TODO(on: date('2025-01-01'), to: 'team@example.com', context: "shopify/smart_todo#108") @@ -53,12 +53,15 @@ works with `date`, `gem_release`, `gem_bump`, and `ruby_version` events: # Upgrade to new Rails version as discussed in the issue def legacy_method end + + # TODO(on: issue_close('shopify', 'smart_todo', '123'), to: 'team@example.com', context: "shopify/other-repo#456") + # Update once the referenced issue is closed, see related context for details + def feature_flag + end ``` When the TODO is triggered, the linked issue's title, state, and assignee will be included in the notification. -Note: The `context` attribute is not supported with `issue_close` or `pull_request_close` events as they already reference specific issues/PRs. - Documentation ---------------- Please check out the GitHub [wiki](https://github.com/Shopify/smart_todo/wiki) for documentation and example on how to setup SmartTodo in your project. diff --git a/lib/smart_todo/cli.rb b/lib/smart_todo/cli.rb index 15719a0..b04bffb 100644 --- a/lib/smart_todo/cli.rb +++ b/lib/smart_todo/cli.rb @@ -137,7 +137,7 @@ def append_context_if_applicable(event_message, todo, event, events) # @param event [Event] the event to check # @return [Boolean] true if context should be applied, false otherwise def should_apply_context?(todo, event) - todo.context && Todo.event_can_use_context?(event.method_name) + !!todo.context end def process_dispatches(dispatches) diff --git a/lib/smart_todo/todo.rb b/lib/smart_todo/todo.rb index 5b54627..1bc0c26 100644 --- a/lib/smart_todo/todo.rb +++ b/lib/smart_todo/todo.rb @@ -6,22 +6,6 @@ class Todo attr_reader :events, :assignees, :errors attr_accessor :context - # Events that already contain issue/PR context and therefore - # should not have additional context applied - EVENTS_WITH_IMPLICIT_CONTEXT = [:issue_close, :pull_request_close].freeze - - class << self - # Check if an event is eligible to have context information applied. - # Events like issue_close and pull_request_close already reference - # specific issues/PRs and shouldn't have additional context. - # - # @param event_name [Symbol] the name of the event method - # @return [Boolean] true if the event can use context, false otherwise - def event_can_use_context?(event_name) - !EVENTS_WITH_IMPLICIT_CONTEXT.include?(event_name.to_sym) - end - end - def initialize(source, filepath = "-e") @filepath = filepath @comment = +"" diff --git a/lib/smart_todo_cop.rb b/lib/smart_todo_cop.rb index 42553a3..c5bfbd7 100644 --- a/lib/smart_todo_cop.rb +++ b/lib/smart_todo_cop.rb @@ -38,8 +38,6 @@ def on_new_investigation add_offense(comment, message: "Invalid event assignee. This method only accepts strings. #{HELP}") elsif (invalid_events = validate_events(metadata.events)).any? add_offense(comment, message: "#{invalid_events.join(". ")}. #{HELP}") - elsif (context_errors = validate_context(metadata)).any? - add_offense(comment, message: "#{context_errors.join(". ")}. #{HELP}") end end end @@ -105,22 +103,6 @@ def validate_pull_request_close_args(args) validate_fixed_arity_args(args, 3, "pull_request_close", ["organization", "repo", "pr_number"]) end - # @param metadata [SmartTodo::Parser::Visitor] The metadata containing context and events - # @return [Array] Returns array of error messages, empty if valid - def validate_context(metadata) - return [] unless metadata.context - - events = metadata.events - - restricted_events = events.reject { |e| ::SmartTodo::Todo.event_can_use_context?(e.method_name) } - if restricted_events.any? - event_name = restricted_events.first.method_name - return ["Invalid context: context attribute cannot be used with #{event_name} event"] - end - - [] - end - # @param args [Array] # @return [String, nil] Returns error message if arguments are invalid, nil if valid def validate_gem_release_args(args) diff --git a/test/smart_todo/cli_test.rb b/test/smart_todo/cli_test.rb index 3888393..c7a6216 100644 --- a/test/smart_todo/cli_test.rb +++ b/test/smart_todo/cli_test.rb @@ -159,43 +159,32 @@ def test_should_apply_context_returns_false_without_context refute(cli.send(:should_apply_context?, todo, event)) end - def test_should_apply_context_returns_false_for_issue_close_event + def test_should_apply_context_returns_true_for_issue_close_event_with_context cli = CLI.new - todo = Todo.new("# TODO(on: issue_close('org', 'repo', '123'), to: 'john@example.com')") - todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + todo = Todo.new( + "# TODO(on: issue_close('org', 'repo', '123'), to: 'john@example.com', context: \"org/repo#456\")", + ) event = Todo::CallNode.new(:issue_close, ["org", "repo", "123"], nil) - refute(cli.send(:should_apply_context?, todo, event)) + assert(cli.send(:should_apply_context?, todo, event)) end - def test_should_apply_context_returns_false_for_pull_request_close_event + def test_should_apply_context_returns_true_for_pull_request_close_event_with_context cli = CLI.new - todo = Todo.new("# TODO(on: pull_request_close('org', 'repo', '123'), to: 'john@example.com')") - todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + todo = Todo.new( + "# TODO(on: pull_request_close('org', 'repo', '123'), to: 'john@example.com', context: \"org/repo#456\")", + ) event = Todo::CallNode.new(:pull_request_close, ["org", "repo", "123"], nil) - refute(cli.send(:should_apply_context?, todo, event)) + assert(cli.send(:should_apply_context?, todo, event)) end def test_should_apply_context_returns_true_for_regular_event_with_context cli = CLI.new - todo = Todo.new("# TODO(on: date('2015-03-01'), to: 'john@example.com')") - todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) + todo = Todo.new("# TODO(on: date('2015-03-01'), to: 'john@example.com', context: \"org/repo#456\")") event = Todo::CallNode.new(:date, ["2015-03-01"], nil) assert(cli.send(:should_apply_context?, todo, event)) end - - def test_append_context_if_applicable_returns_original_message_when_context_not_applicable - cli = CLI.new - todo = Todo.new("# TODO(on: issue_close('org', 'repo', '123'), to: 'john@example.com')") - todo.context = Todo::CallNode.new(:issue, ["org", "repo", "456"], nil) - event = Todo::CallNode.new(:issue_close, ["org", "repo", "123"], nil) - events = Events.new - original_message = "Original event message" - - result = cli.send(:append_context_if_applicable, original_message, todo, event, events) - assert_equal(original_message, result) - end end end diff --git a/test/smart_todo/integration_test.rb b/test/smart_todo/integration_test.rb index 54e57b8..c780f08 100644 --- a/test/smart_todo/integration_test.rb +++ b/test/smart_todo/integration_test.rb @@ -182,10 +182,10 @@ def hello assert_not_requested(:get, /api.github.com/) end - def test_context_ignored_with_issue_close_event + def test_context_included_with_issue_close_event ruby_code = <<~EOM # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'john@example.com', context: "shopify/smart_todo#999") - # This context should be ignored + # This context should be included def hello end EOM @@ -193,6 +193,13 @@ def hello stub_request(:get, %r{api.github.com.*issues/100}) .to_return(body: JSON.dump(state: "closed")) + stub_request(:get, %r{api.github.com.*issues/999}) + .to_return(body: JSON.dump( + title: "Context Issue", + state: "open", + assignee: { login: "developer" }, + )) + generate_ruby_file(ruby_code) do |file| run_cli(file) end @@ -200,10 +207,10 @@ def hello assert_slack_message_sent( "Hello :wave:,", "The issue https://github.com/shopify/smart_todo/issues/100 is now closed", - "This context should be ignored", + "This context should be included", + "Context:", + "Context Issue", ) - - assert_not_requested(:get, %r{api.github.com.*issues/999}) end private diff --git a/test/smart_todo/smart_todo_cop_test.rb b/test/smart_todo/smart_todo_cop_test.rb index a88e01e..3a334e0 100644 --- a/test/smart_todo/smart_todo_cop_test.rb +++ b/test/smart_todo/smart_todo_cop_test.rb @@ -548,24 +548,6 @@ def hello RUBY end - def test_add_offense_when_todo_has_context_with_issue_close_event - expect_offense(<<~RUBY) - # TODO(on: issue_close('shopify', 'smart_todo', '100'), to: 'dev@example.com', context: "shopify/smart_todo#123") - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with issue_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax - def hello - end - RUBY - end - - def test_add_offense_when_todo_has_context_with_pull_request_close_event - expect_offense(<<~RUBY) - # TODO(on: pull_request_close('shopify', 'smart_todo', '200'), to: 'dev@example.com', context: "shopify/smart_todo#123") - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SmartTodo/SmartTodoCop: Invalid context: context attribute cannot be used with pull_request_close event. For more info please look at https://github.com/Shopify/smart_todo/wiki/Syntax - def hello - end - RUBY - end - private def expected_message @@ -593,49 +575,5 @@ def investigate(source, ruby_version = 2.5, file = "(file)") def cop @cop ||= RuboCop::Cop::SmartTodo::SmartTodoCop.new end - - def test_validate_context_returns_empty_array_when_no_context - metadata = SmartTodo::Parser::Visitor.new - metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) - - assert_equal([], cop.send(:validate_context, metadata)) - end - - def test_validate_context_returns_error_for_issue_close_event_with_context - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:issue_close, ["org", "repo", "456"], nil) - - result = cop.send(:validate_context, metadata) - assert_equal(1, result.length) - assert_match(/context attribute cannot be used with issue_close event/, result.first) - end - - def test_validate_context_returns_error_for_pull_request_close_event_with_context - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:pull_request_close, ["org", "repo", "456"], nil) - - result = cop.send(:validate_context, metadata) - assert_equal(1, result.length) - assert_match(/context attribute cannot be used with pull_request_close event/, result.first) - end - - def test_validate_context_returns_empty_array_for_valid_context - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) - - assert_equal([], cop.send(:validate_context, metadata)) - end - - def test_validate_context_returns_empty_array_for_multiple_valid_events - metadata = SmartTodo::Parser::Visitor.new - metadata.context = SmartTodo::Todo::CallNode.new(:issue, ["org", "repo", "123"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:date, ["2015-03-01"], nil) - metadata.events << SmartTodo::Todo::CallNode.new(:gem_release, ["rails", "> 5.0"], nil) - - assert_equal([], cop.send(:validate_context, metadata)) - end end end diff --git a/test/smart_todo/todo_test.rb b/test/smart_todo/todo_test.rb deleted file mode 100644 index 78ca725..0000000 --- a/test/smart_todo/todo_test.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -module SmartTodo - class TodoTest < Minitest::Test - def test_event_can_use_context_returns_true_for_regular_events - assert(Todo.event_can_use_context?(:date)) - assert(Todo.event_can_use_context?(:gem_release)) - assert(Todo.event_can_use_context?(:gem_bump)) - assert(Todo.event_can_use_context?(:custom_event)) - end - - def test_event_can_use_context_returns_false_for_issue_close - refute(Todo.event_can_use_context?(:issue_close)) - end - - def test_event_can_use_context_returns_false_for_pull_request_close - refute(Todo.event_can_use_context?(:pull_request_close)) - end - - def test_event_can_use_context_handles_string_input - assert(Todo.event_can_use_context?("date")) - refute(Todo.event_can_use_context?("issue_close")) - refute(Todo.event_can_use_context?("pull_request_close")) - end - - def test_events_with_implicit_context_constant_is_frozen - assert(Todo::EVENTS_WITH_IMPLICIT_CONTEXT.frozen?) - end - - def test_events_with_implicit_context_contains_expected_events - assert_equal([:issue_close, :pull_request_close], Todo::EVENTS_WITH_IMPLICIT_CONTEXT) - end - end -end From 77ec13827a24d3fb4081e2daee59019980ef305b Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Tue, 28 Oct 2025 18:14:57 -0400 Subject: [PATCH 13/15] PR feedback: use `to_return_json` instead of `to_return` + `JSON.dump()` Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2469492681 --- test/smart_todo/events/issue_context_test.rb | 28 ++++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/smart_todo/events/issue_context_test.rb b/test/smart_todo/events/issue_context_test.rb index d7e755c..70f3c29 100644 --- a/test/smart_todo/events/issue_context_test.rb +++ b/test/smart_todo/events/issue_context_test.rb @@ -7,12 +7,12 @@ class Events class IssueContextTest < Minitest::Test def test_when_issue_exists_and_is_open stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Add support for caching", number: 123, assignee: { login: "johndoe" }, - )) + }) expected = "πŸ“Œ Context: Issue #123 - \"Add support for caching\" [open] (@johndoe) - " \ "https://github.com/rails/rails/issues/123" @@ -22,12 +22,12 @@ def test_when_issue_exists_and_is_open def test_when_issue_exists_and_is_closed stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "closed", title: "Fix memory leak", number: 456, assignee: { login: "janedoe" }, - )) + }) expected = "πŸ“Œ Context: Issue #456 - \"Fix memory leak\" [closed] (@janedoe) - " \ "https://github.com/shopify/smart_todo/issues/456" @@ -37,12 +37,12 @@ def test_when_issue_exists_and_is_closed def test_when_issue_has_no_assignee stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Improve documentation", number: 789, assignee: nil, - )) + }) expected = "πŸ“Œ Context: Issue #789 - \"Improve documentation\" [open] (unassigned) - " \ "https://github.com/org/repo/issues/789" @@ -59,12 +59,12 @@ def test_when_issue_does_not_exist def test_when_token_env_is_not_present stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Test issue", number: 1, assignee: nil, - )) + }) result = issue_context("rails", "rails", "1") assert(result.include?("πŸ“Œ Context: Issue #1")) @@ -78,12 +78,12 @@ def test_when_token_env_is_present ENV[GITHUB_TOKEN] = "abc123" stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Test issue", number: 2, assignee: nil, - )) + }) result = issue_context("rails", "rails", "2") assert(result.include?("πŸ“Œ Context: Issue #2")) @@ -99,12 +99,12 @@ def test_when_organization_specific_token_is_present ENV["#{GITHUB_TOKEN}__RAILS"] = "rails_token" stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Test issue", number: 3, assignee: nil, - )) + }) result = issue_context("rails", "rails", "3") assert(result.include?("πŸ“Œ Context: Issue #3")) @@ -120,12 +120,12 @@ def test_when_repo_specific_token_is_present ENV["#{GITHUB_TOKEN}__RAILS__RAILS"] = "rails_rails_token" stub_request(:get, /api.github.com/) - .to_return(body: JSON.dump( + .to_return_json(body: { state: "open", title: "Test issue", number: 4, assignee: nil, - )) + }) result = issue_context("rails", "rails", "4") assert(result.include?("πŸ“Œ Context: Issue #4")) From ed3d9f843989c61f451d76ac3551b9b607c8edb8 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Tue, 28 Oct 2025 18:35:17 -0400 Subject: [PATCH 14/15] PR feedback: rescuing for some errors and let the other ones go through Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2469524110 --- lib/smart_todo/events.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/smart_todo/events.rb b/lib/smart_todo/events.rb index 0241b4a..1ee1d4e 100644 --- a/lib/smart_todo/events.rb +++ b/lib/smart_todo/events.rb @@ -179,7 +179,7 @@ def issue_context(organization, repo, issue_number) "πŸ“Œ Context: Issue ##{issue_number} - \"#{title}\" [#{state}] (#{assignee}) - " \ "https://github.com/#{organization}/#{repo}/issues/#{issue_number}" end - rescue + rescue Net::HTTPError, JSON::ParserError nil end From c8f0dca9be271346f673de0cd390d870471bbd89 Mon Sep 17 00:00:00 2001 From: Charles Ouimet Date: Wed, 29 Oct 2025 14:58:11 -0400 Subject: [PATCH 15/15] PR feedback: examples will be moved to the wiki Ref: https://github.com/Shopify/smart_todo/pull/107#discussion_r2472368783 --- README.md | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/README.md b/README.md index b190b19..48e257e 100644 --- a/README.md +++ b/README.md @@ -40,28 +40,6 @@ When the TODO's event is met (i.e. a certain date is reached), the TODO's assign end ``` -You can also add context to your TODOs by linking them to GitHub issues. The `context` attribute -works with all events: - -```ruby - # TODO(on: date('2025-01-01'), to: 'team@example.com', context: "shopify/smart_todo#108") - # Implement the caching strategy discussed in the issue - def process_order - end - - # TODO(on: gem_release('rails', '> 7.2'), to: 'dev@example.com', context: "rails/rails#456") - # Upgrade to new Rails version as discussed in the issue - def legacy_method - end - - # TODO(on: issue_close('shopify', 'smart_todo', '123'), to: 'team@example.com', context: "shopify/other-repo#456") - # Update once the referenced issue is closed, see related context for details - def feature_flag - end -``` - -When the TODO is triggered, the linked issue's title, state, and assignee will be included in the notification. - Documentation ---------------- Please check out the GitHub [wiki](https://github.com/Shopify/smart_todo/wiki) for documentation and example on how to setup SmartTodo in your project.