Skip to content

feat(emojis): Distinguish approved reviews with inline comments - #58

Open
chouetz wants to merge 1 commit into
masterfrom
approved-with-comments
Open

feat(emojis): Distinguish approved reviews with inline comments#58
chouetz wants to merge 1 commit into
masterfrom
approved-with-comments

Conversation

@chouetz

@chouetz chouetz commented Mar 18, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Adds a new emoji (:approved_with_comments:) that is used when a review approval contains inline code comments, distinguishing it from a plain approval (:approved:) that has only a general comment or no comment at all.

Motivation

When a reviewer approves a PR but leaves inline comments, the PR author should have a clear visual signal that there are code-level remarks to address. Using the standard :approved: emoji doesn't convey this — the author might miss the inline feedback entirely.

Describe how you validated your changes

  • Added a parametrized test case (approval-with-inline-comments) covering the new behavior
  • All 15 existing + new tests pass

Additional Notes

  • The new emoji is configurable via the SLAPR_EMOJI_APPROVED_WITH_COMMENTS environment variable (defaults to approved_with_comments)
  • Inline comments are detected by fetching PR review comments once and matching them to reviews via raw_data["pull_request_review_id"] (PyGithub 1.45 doesn't expose this as a property)
  • No extra API calls per review — all review comments are fetched in a single call and grouped by review ID

🤖 Generated with Claude Code

Use :approved_with_comments: emoji when an approval review contains
inline code comments, and standard :approved: when it only has a
general comment. This gives PR authors clearer signal that there are
inline comments to address even though the review is an approval.
@chouetz
chouetz marked this pull request as ready for review March 18, 2026 17:41
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.

2 participants