Skip to content

Conversation

@couimet
Copy link
Contributor

@couimet couimet commented Oct 23, 2025

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 TODOs 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

# TODO(on: date('2025-02-01'), to: '[email protected]', 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

@couimet couimet requested review from a team and rafaelfranca October 23, 2025 18:27
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: '[email protected]', 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
@couimet couimet changed the title Add issue_pin event to link TODOs with GitHub issues Add context attribute to link TODOs with GitHub issues Oct 23, 2025
…nsidered valid with `issue_close` and `pull_request_close`
@couimet couimet force-pushed the add-issue-pin-feature branch from 236b2b0 to 39ccf2b Compare October 23, 2025 21:06
Comment on lines 113 to 116
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
Copy link
Member

Choose a reason for hiding this comment

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

Extract to a method called invalid_events_for_context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I built this utility as SmartTodo::Todo.event_can_use_context? in 2090a29 so it can be used from different modules without copy-pasta.

Let me know what you think

Copy link
Member

@Edouard-chin Edouard-chin left a comment

Choose a reason for hiding this comment

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

I think this is a great addition, but I have the feeling that this may try to over-engineer a solution. It also makes a todo definition very verbose and it becomes hard to read when looking at the TODO definition.

I wonder if a more simpler approach à la GitHub would be easier.

# TODO(on...)
#   Part of the bigger cleanup
#   Ref Shopify/super_repo#34

When SmartTodo detects the Ref ... part, it would transfom it to a link to be sent along the slack message

@couimet couimet self-assigned this Oct 24, 2025
@couimet
Copy link
Contributor Author

couimet commented Oct 24, 2025

I think this is a great addition, but I have the feeling that this may try to over-engineer a solution. It also makes a todo definition very verbose and it becomes hard to read when looking at the TODO definition.

I wonder if a more simpler approach à la GitHub would be easier.

# TODO(on...)
#   Part of the bigger cleanup
#   Ref Shopify/super_repo#34

When SmartTodo detects the Ref ... part, it would transfom it to a link to be sent along the slack message

Thanks for the feedback !

Using Ref <org>/<repo>/<id> format could indeed have been used, but I didn't come across it much in shop-server (the main repo I work on).

Given context is optional, people will be free to either use it or don't if they prefer to use a full link (or short Ref <org>/<repo>/<id> version) in a free-fom way in the comments.

I believe it's better to build context as structured info so it can easily be handled in Rubocop/code and additional features can be built on top of it if desired.

What do you think ?

@Edouard-chin
Copy link
Member

Edouard-chin commented Oct 24, 2025

I understand that building a structured context makes it easier for us to parse. But it comes with the downside of making it harder for humans to write and read.

Writing a smart todo is already not the most user friendly (especially since its just a comment without any syntax highlighting), and I'm worried that adding a new programming syntax info to the TODO will clutter it even more.

Ultimately a TODO should be written to be read by humans not machines which is why I suggested the standard Ref repo/org#123. I honestly wouldn't bother reading the TODO metadata if I were to stumbled upon this:

# TODO(on: date('2025-03-15'), to: '#proj-slack-channel-session-something', context: issue('shopify', tapioca, 27721))

I wouldn't want to dismiss the great work you already put in this PR and since Rafael is also ok with this approach I'll defer to his approval. At the very minimum if we go with the structured context, please consider context: "org/repo#123 instead of context: issue('org', 'repo', 123) (which also opens the door for other context and more complexity)

@rafaelfranca
Copy link
Member

I actually really like context: "org/repo#123" good idea. Can we see how supporting only that would look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for context attribute that refers to an issue

3 participants