Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/smart_todo.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module SmartTodo
autoload :Todo, "smart_todo/todo"
autoload :CommentParser, "smart_todo/comment_parser"
autoload :HttpClientBuilder, "smart_todo/http_client_builder"
autoload :DeepLink, "smart_todo/deep_link"

module Dispatchers
autoload :Base, "smart_todo/dispatchers/base"
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_todo/comment_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def parse_comments(comments, filepath)

if source.match?(TAG_PATTERN)
todos << current_todo if current_todo
current_todo = Todo.new(source, filepath)
current_todo = Todo.new(source, filepath, line_number: comment.location.start_line)
elsif current_todo && (indent = source[/^#(\s*)/, 1].length) && (indent - current_todo.indent == 2)
current_todo << "#{source[(indent + 1)..]}\n"
else
Expand Down
95 changes: 95 additions & 0 deletions lib/smart_todo/deep_link.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# frozen_string_literal: true

require "pathname"

module SmartTodo
module DeepLink
Link = Struct.new(:url, :display, keyword_init: true)

class << self
# Filepaths that indicate inline/stdin evaluation and shouldn't be linked
UNLINKABLE_PATHS = ["-e", "-"].freeze

# Generate a deep link for a TODO if possible.
# Returns structured data if a CI environment is detected, nil otherwise.
#
# @param todo [SmartTodo::Todo] the todo containing filepath and line numbers
# @return [Link, nil] Link with url and display, or nil if no link can be generated
def for_todo(todo)
return if UNLINKABLE_PATHS.include?(todo.filepath)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required ? I don't think we run smart todo with inline code on CI (it wouldn't make a lot of sense)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! While you're right that the CLI iterates over actual files, the library's API explicitly defaults to -e to support programmatic use cases:

  • CommentParser.parse(source, filepath = "-e")
  • Todo.new(source, filepath = "-e")

Without the check, we'd generate broken URLs like https://github.com/org/repo/blob/sha/-e#L1. The check ensures graceful fallback to `-e:1` (code-formatted path) instead of a broken link.


from_github_actions(todo) || from_buildkite(todo)
end

private

def from_github_actions(todo)
return unless ENV["GITHUB_ACTIONS"]

prefix = ENV.fetch("SMART_TODO_REPO_PATH") do
Pathname.new(Dir.pwd).relative_path_from(ENV["GITHUB_WORKSPACE"]).to_s.delete_prefix(".")
end
relative_path = join_path(prefix, todo.filepath.delete_prefix("./"))
repo = "#{ENV["GITHUB_SERVER_URL"]}/#{ENV["GITHUB_REPOSITORY"]}"

url = if (fragment = line_fragment(todo))
"#{repo}/blob/#{ENV["GITHUB_SHA"]}/#{relative_path}##{fragment}"
else
"#{repo}/blob/#{ENV["GITHUB_SHA"]}/#{relative_path}"
end

display = if todo.line_reference
"#{relative_path}:#{todo.line_reference}"
else
relative_path
end

Link.new(url: url, display: display)
end

def from_buildkite(todo)
return unless ENV["BUILDKITE"]

prefix = ENV.fetch("SMART_TODO_REPO_PATH") do
Pathname.new(Dir.pwd).relative_path_from(ENV["BUILDKITE_BUILD_CHECKOUT_PATH"]).to_s.delete_prefix(".")
end
relative_path = join_path(prefix, todo.filepath.delete_prefix("./"))
repo = ENV["BUILDKITE_REPO"].delete_suffix(".git")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to grab env from the CI provider. Do you think the --repo option we pass to the CLI is now any useful ? If its still useful, should it have precedence ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These features are actually complementary rather than overlapping:

Feature Purpose Output
--repo Human-readable repo name "in repository \smart_todo`"` at end of message
DeepLink Clickable URL <url|file.rb:42> for file reference

Both can appear in the same message. --repo remains useful for:

  1. Local runs — No CI env vars means DeepLink returns nil, but --repo still provides context
  2. Unsupported CI providers — Jenkins, GitLab CI, CircleCI, etc. won't have the expected env vars
  3. Custom naming — In monorepos, someone might want a different name than the GitHub repo

No precedence logic needed since they operate on different parts of the message.


url = if (fragment = line_fragment(todo))
"#{repo}/blob/#{ENV["BUILDKITE_COMMIT"]}/#{relative_path}##{fragment}"
else
"#{repo}/blob/#{ENV["BUILDKITE_COMMIT"]}/#{relative_path}"
end

display = if todo.line_reference
"#{relative_path}:#{todo.line_reference}"
else
relative_path
end

Link.new(url: url, display: display)
end

def join_path(prefix, path)
if prefix.empty?
path
else
File.join(prefix, path)
end
end

# GitHub-style line fragment for URL (e.g., "L5" or "L5-L7")
# Returns nil if line_number is not available
def line_fragment(todo)
return unless todo.line_number

if todo.end_line_number != todo.line_number
"L#{todo.line_number}-L#{todo.end_line_number}"
else
"L#{todo.line_number}"
end
end
end
end
end
13 changes: 12 additions & 1 deletion lib/smart_todo/dispatchers/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def slack_message(user, assignee)
<<~EOM
#{header}

You have an assigned TODO in the `#{@file}` file#{repo}.
You have an assigned TODO in #{slack_file_reference(@todo_node)}#{repo}.
#{@event_message}

Here is the associated comment on your TODO:
Expand Down Expand Up @@ -100,6 +100,17 @@ def repo
" in repository `#{repo}`"
end
end

# Format file reference for Slack
# Uses deep link if available, otherwise falls back to code-formatted path
def slack_file_reference(todo)
link = DeepLink.for_todo(todo)
if link
"<#{link.url}|#{link.display}>"
else
"`#{todo.file_reference}`"
end
end
end
end
end
38 changes: 36 additions & 2 deletions lib/smart_todo/todo.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,46 @@

module SmartTodo
class Todo
attr_reader :filepath, :comment, :indent
attr_reader :filepath, :comment, :indent, :line_number
attr_reader :events, :assignees, :errors
attr_accessor :context

def initialize(source, filepath = "-e")
def end_line_number
return unless line_number

line_number + comment.count("\n")
end

def line_reference
return unless line_number

if end_line_number != line_number
"#{line_number}-#{end_line_number}"
else
line_number.to_s
end
end

def file_reference
if line_reference
"#{filepath}:#{line_reference}"
else
filepath
end
end

def initialize(source, filepath = "-e", line_number: nil)
@filepath = filepath
@line_number = line_number

if line_number.nil?
warn(
"Calling `SmartTodo::Todo.new` without `line_number:` is deprecated " \
"and will become required in a future version.",
category: :deprecated,
uplevel: 1,
)
end
@comment = +""
@indent = source[/^#(\s+)/, 1].length

Expand Down
8 changes: 4 additions & 4 deletions lib/smart_todo_cop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def on_new_investigation
next
end

metadata = metadata(comment.text)
metadata = metadata(comment)

if metadata.errors.any?
add_offense(comment, message: "Invalid TODO format: #{metadata.errors.join(", ")}. #{HELP}")
Expand All @@ -44,10 +44,10 @@ def on_new_investigation

private

# @param comment [String]
# @return [SmartTodo::Parser::Visitor]
# @param comment [RuboCop::AST::Comment]
# @return [SmartTodo::Todo]
def metadata(comment)
::SmartTodo::Todo.new(comment)
::SmartTodo::Todo.new(comment.text, line_number: comment.loc.line)
end

# @param metadata [SmartTodo::Parser::Visitor]
Expand Down
9 changes: 7 additions & 2 deletions test/smart_todo/cli_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def hello

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')")
todo = Todo.new("# TODO(on: date('2015-03-01'), to: 'john@example.com')", line_number: 1)
event = Todo::CallNode.new(:date, ["2015-03-01"], nil)

refute(cli.send(:should_apply_context?, todo, event))
Expand All @@ -163,6 +163,7 @@ 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', context: \"org/repo#456\")",
line_number: 1,
)
event = Todo::CallNode.new(:issue_close, ["org", "repo", "123"], nil)

Expand All @@ -173,6 +174,7 @@ def test_should_apply_context_returns_true_for_pull_request_close_event_with_con
cli = CLI.new
todo = Todo.new(
"# TODO(on: pull_request_close('org', 'repo', '123'), to: 'john@example.com', context: \"org/repo#456\")",
line_number: 1,
)
event = Todo::CallNode.new(:pull_request_close, ["org", "repo", "123"], nil)

Expand All @@ -181,7 +183,10 @@ def test_should_apply_context_returns_true_for_pull_request_close_event_with_con

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', context: \"org/repo#456\")")
todo = Todo.new(
"# TODO(on: date('2015-03-01'), to: 'john@example.com', context: \"org/repo#456\")",
line_number: 1,
)
event = Todo::CallNode.new(:date, ["2015-03-01"], nil)

assert(cli.send(:should_apply_context?, todo, event))
Expand Down
87 changes: 87 additions & 0 deletions test/smart_todo/comment_parser_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,5 +187,92 @@ def hello
assert_equal(:date, todo[0].events[0].method_name)
assert_equal(["john@example.com"], todo[0].assignees)
end

def test_parse_todo_captures_line_number
ruby_code = <<~RUBY
# TODO(on: date('2019-08-04'), to: 'john@example.com')
# Remove this code once done
def hello
end
RUBY

todo = CommentParser.parse(ruby_code)
assert_equal(1, todo[0].line_number)
end

def test_parse_multiple_todos_captures_correct_line_numbers
ruby_code = <<~RUBY
# TODO(on: date('2019-08-04'), to: 'john@example.com')
# First todo
def hello
end

# TODO(on: date('2019-08-04'), to: 'jane@example.com')
# Second todo
def bar
end
RUBY

todos = CommentParser.parse(ruby_code)
assert_equal(2, todos.size)
assert_equal(1, todos[0].line_number)
assert_equal(6, todos[1].line_number)
end

def test_parse_todo_with_preceding_code_captures_correct_line_number
ruby_code = <<~RUBY
def hello
end

# TODO(on: date('2019-08-04'), to: 'john@example.com')
# Remove this code once done
def world
end
RUBY

todo = CommentParser.parse(ruby_code)
assert_equal(1, todo.size)
assert_equal(4, todo[0].line_number)
end

def test_parse_todo_single_line_has_same_start_and_end_line
ruby_code = <<~RUBY
# TODO(on: date('2019-08-04'), to: 'john@example.com')
def hello
end
RUBY

todo = CommentParser.parse(ruby_code)
assert_equal(1, todo[0].line_number)
assert_equal(1, todo[0].end_line_number)
end

def test_parse_todo_multi_line_captures_end_line_number
ruby_code = <<~RUBY
# TODO(on: date('2019-08-04'), to: 'john@example.com')
# Remove this code once done
# This is important
# Please don't disappoint me
def hello
end
RUBY

todo = CommentParser.parse(ruby_code)
assert_equal(1, todo[0].line_number)
assert_equal(4, todo[0].end_line_number)
end

def test_parse_todo_with_single_continuation_line
ruby_code = <<~RUBY
# TODO(on: date('2019-08-04'), to: 'john@example.com')
# Just one extra line
def hello
end
RUBY

todo = CommentParser.parse(ruby_code)
assert_equal(1, todo[0].line_number)
assert_equal(2, todo[0].end_line_number)
end
end
end
Loading