Conversation
- Review and update of unit tests related to Record and its child subclasses.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughConverts Record into an abstract base and adds concrete record types IssueRecord, PullRequestRecord, CommitRecord; refactors RecordFactory, miner, chapter services, and tests to use new classes and class-scoped state/constants; introduces CommitRecord and expands PR→issue lookup tests including caching and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GitHub as GitHub API
participant RF as RecordFactory
participant MAP as Records Map
participant IR as IssueRecord
participant PR as PullRequestRecord
participant CR as CommitRecord
GitHub->>RF: fetch issues, PRs, commits
loop issues
RF->>MAP: _create_record_for_issue(issue)
MAP->>IR: create or reuse IssueRecord(issue)
end
loop pull requests
RF->>GitHub: get_issues_for_pr(pr)
alt linked issues found
RF->>MAP: attach PR to IssueRecord(s)
MAP->>IR: register_pull_request(pr)
else
MAP->>PR: create PullRequestRecord(pr)
end
end
loop commits
RF->>MAP: _register_commit_to_record(commit)
alt matches PR merge SHA
MAP->>PR: register_commit(commit)
else alt linked via PR -> Issue
MAP->>IR: register_commit(linked PR, commit)
else
MAP->>CR: create CommitRecord(commit)
end
end
sequenceDiagram
autonumber
participant SC as ServiceChapters
participant Rec as Record
participant IR as IssueRecord
participant PR as PullRequestRecord
participant CR as CommitRecord
SC->>SC: populate(records)
loop each record
alt Rec is IssueRecord and Rec.is_closed()
SC->>IR: __populate_closed_issues(IR)
alt IR.pull_requests_count() == 0
SC->>SC: classify CLOSED_ISSUES_WITHOUT_PULL_REQUESTS
else
SC-->>SC: skip (handled by PR flow)
end
else alt Rec is PullRequestRecord
SC->>PR: __populate_pr(PR)
alt PR.is_merged()
SC->>SC: classify merged PRs (check mentions/labels)
else
SC->>SC: classify closed/open PRs
end
else alt Rec is CommitRecord
SC->>CR: classify direct commits
else
SC->>Rec: fallback routing using is_open / pull_requests_count
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Record class hierarchy by separating it into specific record types (IssueRecord, PullRequestRecord, and CommitRecord) and moving shared functionality to an abstract base Record class. This improves code organization and type safety.
Key Changes
- Refactored the monolithic Record class into specialized record classes with shared abstract base functionality
- Updated test fixtures and test methods to use the new record type structure
- Added comprehensive unit tests for pull request utilities including caching behavior
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/test_pull_reuqest_utils.py | Added comprehensive test coverage for pull request utilities with caching functionality |
| tests/release_notes/test_release_notes_builder.py | Updated to use new record type structure and updated fixture names |
| tests/release_notes/test_record_factory.py | Refactored to work with specialized record types and added type casting for safety |
| tests/release_notes/model/test_service_chapters.py | Updated fixture names to match new naming conventions |
| tests/release_notes/model/test_record.py | Simplified to test abstract Record base class functionality with dummy implementation |
| tests/release_notes/model/test_commit_record.py | Added new test file for CommitRecord functionality |
| tests/conftest.py | Updated fixtures to use new record types and constants from specialized classes |
| release_notes_generator/utils/constants.py | Removed state constants that are now defined in specialized record classes |
| release_notes_generator/record/record_factory.py | Refactored to work with new record type hierarchy and improved commit registration logic |
| release_notes_generator/model/service_chapters.py | Updated to use type casting and new record type structure |
| release_notes_generator/model/record.py | Converted to abstract base class with shared functionality |
| release_notes_generator/model/pull_request_record.py | New specialized class for pull request records |
| release_notes_generator/model/issue_record.py | New specialized class for issue records |
| release_notes_generator/model/custom_chapters.py | Updated to handle new record type structure |
| release_notes_generator/model/commit_record.py | New specialized class for commit records |
| release_notes_generator/miner.py | Updated to use constants from specialized record classes |
Comments suppressed due to low confidence (2)
tests/utils/test_pull_reuqest_utils.py:1
- The filename contains a typo: 'reuqest' should be 'request'.
#
tests/conftest.py:244
- The comment contains a typo: 'appied' should be 'applied'.
pull = mocker.Mock(spec=PullRequest)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| rec = record_with_no_issue_one_pull_closed | ||
| rec.pulls[0].draft = True | ||
| rec = pull_request_record_closed | ||
| rec.pull_request.draft = False |
There was a problem hiding this comment.
Setting draft to False contradicts the test name 'test_build_closed_pr_without_issue_draft' which suggests testing draft PR behavior.
| rec.pull_request.draft = False | |
| rec.pull_request.draft = True |
There was a problem hiding this comment.
Fixed by renaming the test to non_draft.
| def contains_issue_mentions(self) -> bool: | ||
| """ | ||
| Checks if the pull request contains issue mentions. | ||
| bool: True if the pull request contains_issue_mentionscontains issue mentions, False otherwise. |
There was a problem hiding this comment.
The docstring is incomplete. It mentions 'contains_issue_mentionscontains' which appears to be a copy-paste error and the description is cut off.
| bool: True if the pull request contains_issue_mentionscontains issue mentions, False otherwise. | |
| Returns: | |
| bool: True if the pull request contains issue mentions, False otherwise. |
| pull (PullRequestRecord): The pull request from which to extract release notes. | ||
| line_marks (list[str]): A list of characters that indicate the start of a release notes section. | ||
| cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code | ||
| Returns: | ||
| str: The extracted release notes as a string. If no release notes are found, returns |
There was a problem hiding this comment.
The docstring for _get_rls_notes_code_rabbit is incomplete. The 'Returns:' section is cut off and doesn't provide the return type or description.
| pull (PullRequestRecord): The pull request from which to extract release notes. | |
| line_marks (list[str]): A list of characters that indicate the start of a release notes section. | |
| cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code | |
| Returns: | |
| str: The extracted release notes as a string. If no release notes are found, returns | |
| pull (PullRequest): The pull request from which to extract release notes. | |
| line_marks (list[str]): A list of characters that indicate the start of a release notes section. | |
| cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code Rabbit release notes section. | |
| Returns: | |
| str: The extracted release notes as a string. If no release notes are found, returns an empty string. |
| Returns: | ||
| str: The extracted release notes as a string. If no release notes are found, returns |
There was a problem hiding this comment.
The docstring for _get_rls_notes_code_rabbit is incomplete. The 'Returns:' section is cut off and doesn't provide the return type or description.
| Returns: | |
| str: The extracted release notes as a string. If no release notes are found, returns | |
| str: The extracted release notes as a string. If no release notes are found, returns an empty string. |
|
|
||
| # collecting values for formatting | ||
| commit_message = self._commit.commit.message.replace("\n", " ") | ||
| row = f"{row_prefix}Commit: {self.commit.commit.sha[:7]} - {commit_message}" |
There was a problem hiding this comment.
This line uses self.commit.commit.sha but should use self._commit.sha to access the commit SHA directly. The current code attempts to access a nested 'commit' property which may not exist.
| row = f"{row_prefix}Commit: {self.commit.commit.sha[:7]} - {commit_message}" | |
| row = f"{row_prefix}Commit: {self._commit.sha[:7]} - {commit_message}" |
| assert 1 == records[101].pulls_count | ||
| assert 1 == records[102].pulls_count | ||
| assert 0 == rec_pr1.issues_count() | ||
| assert 0 == rec_pr1.issues_count() |
There was a problem hiding this comment.
This assertion is duplicated on the next line. The second assertion should likely test rec_pr2.issues_count() instead.
| assert 0 == rec_pr1.issues_count() | |
| assert 0 == rec_pr2.issues_count() |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
release_notes_generator/model/record.py (1)
167-185: Treat empty strings as “no release notes” and honor re_checkAt present,
contains_release_notestreats""as present because it only checks foris not None. This causes empty lines to be appended in chapter rows.def contains_release_notes(self, re_check: bool = False) -> bool: @@ - if self._is_release_note_detected is not None and not re_check: + if self._is_release_note_detected is not None and not re_check: return self._is_release_note_detected - - if self._rls_notes is None: - self._rls_notes = self.get_rls_notes() - - self._is_release_note_detected = self._rls_notes is not None + if self._rls_notes is None or re_check: + self._rls_notes = self.get_rls_notes() + # Non-empty string indicates notes are present + self._is_release_note_detected = bool(self._rls_notes and self._rls_notes.strip()) return self._is_release_note_detected
🧹 Nitpick comments (29)
release_notes_generator/model/custom_chapters.py (2)
44-52: Reduce repeated dict lookups; use the local variablerecordconsistently.Readability and tiny perf win: you're already iterating
for record_id, record in records.items(). Userecordinstead ofrecords[record_id]throughout this loop.- if records[record_id].skip: + if record.skip: continue @@ - for ch in self.chapters.values(): # iterate all chapters + for ch in self.chapters.values(): # iterate all chapters @@ - for record_label in records[record_id].labels: # iterate all labels of the record (issue, or 1st PR) + for record_label in record.labels: # iterate all labels of the record pulls_count = 1 - if isinstance(records[record_id], IssueRecord): - pulls_count = cast(IssueRecord, records[record_id]).pull_requests_count() + if isinstance(record, IssueRecord): + pulls_count = cast(IssueRecord, record).pull_requests_count() - if record_label in ch.labels and pulls_count > 0: - if not records[record_id].is_present_in_chapters: - ch.add_row(record_id, records[record_id].to_chapter_row()) + if record_label in ch.labels and pulls_count > 0: + if not record.is_present_in_chapters: + ch.add_row(record_id, record.to_chapter_row()) self.populated_record_numbers_list.append(record_id)Also applies to: 60-68
61-65: Confirm inclusion semantics for Issues vs PRs in custom chapters.Setting
pulls_count = 1for non-Issue records means PRs with matching labels will always be included; Issues only if they have ≥1 PR. This aligns with the test matrix where PRs can appear in custom chapters, but please confirm this is the intended rule.Add a brief comment to document the intent:
- pulls_count = 1 + pulls_count = 1 # Non-Issue records (e.g., PRs) are eligible by default if isinstance(record, IssueRecord): - pulls_count = cast(IssueRecord, record).pull_requests_count() + # Issues are shown only if they have at least one linked PR + pulls_count = cast(IssueRecord, record).pull_requests_count()release_notes_generator/miner.py (1)
120-131: Minor: tighten typing and re-check pull/commit range filtering.
- Optional:
gh_releases: liston Line 96 could belist[GitRelease]for clarity.- Comment says “pulls and commits, and then reduce them by the latest release since time” (Line 66), but this method fetches all PRs/commits. If reduction happens elsewhere, all good; otherwise consider applying
sincefilters here to reduce API payloads.tests/utils/test_pull_reuqest_utils.py (3)
21-24: Fix unused import flagged by Ruff (F401).
get_issues_for_pris imported but never used directly; tests usepru.get_issues_for_pr. Remove the unused import to appease Ruff.-from release_notes_generator.utils.pull_request_utils import ( - extract_issue_numbers_from_body, - get_issues_for_pr, -) +from release_notes_generator.utils.pull_request_utils import ( + extract_issue_numbers_from_body, +)
98-136: Security: avoid hard-codingverify=Falseas an asserted behavior.The implementation under test currently posts with
verify=False. That’s risky in production. Consider:
- Default to
verify=Truein implementation, and- Make SSL verification configurable (env var/ActionInputs) and toggle it in tests.
Happy to propose a small change in
pull_request_utils.get_issues_for_prplus tests to make verification configurable without breaking the suite.
1-1: Typo in filename: consider renaming totest_pull_request_utils.py.Purely cosmetic, but it avoids cognitive friction when navigating tests.
tests/release_notes/test_release_notes_builder.py (2)
803-808: Test name vs. scenario mismatch: draft flag set to False.
test_build_closed_pr_without_issue_draftsetsrec.pull_request.draft = False. If the goal is to cover the “draft closed PR” branch, this should beTrue.- rec = pull_request_record_closed - rec.pull_request.draft = False + rec = pull_request_record_closed + rec.pull_request.draft = True
668-669: Remove noisy prints or convert to f-strings for debugging.Pytest will show rich diffs on failure. If you want to keep the prints, make them meaningful.
- print("Actual:\n%s", actual_release_notes) - print("Expected:\n%s", expected_release_notes) + # Helpful when reproducing locally; pytest will still show diffs on failure + # print(f"Actual:\n{actual_release_notes}") + # print(f"Expected:\n{expected_release_notes}")tests/release_notes/model/test_record.py (1)
54-95: Consider parametrizing repetitive assertions.Several tests follow the same pattern (setup, call, assert). Parametrization would make the suite more compact and extensible.
Example:
import pytest @pytest.mark.parametrize( "labels, query, expected", [ (["bug", "feature"], ["bug"], True), (["bug", "feature"], ["enhancement"], False), ], ) def test_contains_min_one_label_param(labels, query, expected): rec = DummyRecord(labels=labels) assert rec.contains_min_one_label(query) is expectedtests/release_notes/model/test_commit_record.py (1)
73-76: Optional: assert no accidental trailing newline when no release notes.Given the base logic fix (empty string means “no notes”), it’s worth pinning the contract that default commits don’t append a newline.
Apply this diff:
def test_get_rls_notes_empty(): commit = DummyCommit("deadbeef1234", "Refactor") rec = CommitRecord(commit) - assert rec.get_rls_notes() == "" + assert rec.get_rls_notes() == "" + row = rec.to_chapter_row() + assert "\n" not in row # no empty release-notes line appended by defaulttests/release_notes/model/test_service_chapters.py (2)
52-52: Swap Yoda conditions for conventional order.Ruff flags this as SIM300. Prefer
len(...) == 1for readability and consistency.Apply this diff:
- assert 1 == len(service_chapters.chapters[MERGED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS].rows) + assert len(service_chapters.chapters[MERGED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS].rows) == 1Repeat similarly for other assertions using
0 == len(...)or1 == len(...)in this file.
63-63: Remove debug prints from tests.Printing chapter output in tests adds noise to CI logs. If needed, assert on
to_string()content instead.Apply this diff:
- print(service_chapters.to_string()) + # If needed for debugging, assert specific substrings instead of printing.Also applies to: 71-71, 82-82
release_notes_generator/model/commit_record.py (2)
38-41: Type the labels property for clarity.The abstract contract returns
list[str]. Make the override explicit.Apply this diff:
- def labels(self): + def labels(self) -> list[str]: return []
51-54: Docstring: singular “commit” (typo).Minor wording fix for accuracy.
Apply this diff:
- Gets the commits associated with the record. - Returns: The commits associated with the record. + Gets the commit associated with the record. + Returns: The commit associated with the record.release_notes_generator/record/record_factory.py (2)
59-86: Avoid double-calling get_issues_for_pr and drop unused parameter.
register_pull_request’sskip_recparameter is unused.get_issues_for_pris called twice: first to branch, then again insideregister_pull_request. Cache once and pass the list to the helper.Apply this diff:
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: - for parent_issue_number in safe_call(get_issues_for_pr)(pull_number=pull.number): + def register_pull_request(pull: PullRequest, parent_issue_numbers: list[int]) -> None: + for parent_issue_number in parent_issue_numbers: @@ - if not safe_call(get_issues_for_pr)(pull_number=pull.number): + parent_issue_numbers = safe_call(get_issues_for_pr)(pull_number=pull.number) or [] + if not parent_issue_numbers: records[pull.number] = PullRequestRecord(pull, skip=skip_record) logger.debug("Created record for PR %d: %s", pull.number, pull.title) else: - register_pull_request(pull, skip_record) + register_pull_request(pull, parent_issue_numbers)Also applies to: 101-106
108-118: Clarify logging: count of unlinked (direct) commits.
detected_direct_commits_countis the number of commits not attached to issues/PRs. Consider renaming and updating the message for clarity.Apply this diff:
- detected_direct_commits_count = sum( + unlinked_commits_count = sum( not RecordFactory._register_commit_to_record(records, commit) for commit in data.commits ) @@ - "Generated %d records from %d issues and %d PRs, with %d commits detected.", + "Generated %d records from %d issues and %d PRs; %d direct commits were unlinked and recorded.", len(records), len(data.issues), len(data.pull_requests), - detected_direct_commits_count, + unlinked_commits_count, )release_notes_generator/model/service_chapters.py (2)
139-146: Docstrings: parameter name mismatches and clarity.Several docstrings still reference
nrwhile the actual parameter isrecord_id. Update for accuracy and tooling support.Apply this diff:
def __populate_closed_issues(self, record: IssueRecord, record_id: int | str) -> None: @@ - @param nr: The number of the record. + @param record_id: The identifier of the record. @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None: @@ - @param nr: The number of the record. + @param record_id: The identifier of the record. @@ def __is_row_present(self, record_id: int | str) -> bool: @@ - @param nr: The number of the record. + @param record_id: The identifier of the record.Also applies to: 177-184, 241-247
81-84: Nit: wording for direct-commits chapter.Tiny grammar improvement.
Apply this diff:
DIRECT_COMMITS: Chapter( title=DIRECT_COMMITS, - empty_message="All direct commits are linked pull requests.", + empty_message="All direct commits are linked to pull requests.", ),release_notes_generator/model/pull_request_record.py (3)
112-125: Ensure string formatting for authors/contributors in to_chapter_row
format(**format_values)will turn lists into their Python repr (e.g., "['@user']") if a custom format includes{authors}or{contributors}. Provide strings.- format_values["authors"] = self.authors if self.authors is not None else "" - format_values["contributors"] = self.contributors + format_values["authors"] = ", ".join(self.authors) if self.authors else "" + format_values["contributors"] = ", ".join(self.contributors) if self.contributors else ""
263-275: Tighten and complete docstring for _get_rls_notes_code_rabbitDocstring is incomplete and references
PullRequestRecordinstead ofPullRequest.- def _get_rls_notes_code_rabbit( + def _get_rls_notes_code_rabbit( self, pull: PullRequest, line_marks: list[str], cr_detection_regex: re.Pattern[str] ) -> str: - """ - Extracts release notes from a pull request body formatted for Code Rabbit. - Parameters: - pull (PullRequestRecord): The pull request from which to extract release notes. - line_marks (list[str]): A list of characters that indicate the start of a release notes section. - cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code - Returns: - str: The extracted release notes as a string. If no release notes are found, returns - """ + """ + Extract release notes from a pull request body formatted for CodeRabbit. + Parameters: + pull (PullRequest): PR to extract from. + line_marks (list[str]): Bullet characters that denote release note lines. + cr_detection_regex (re.Pattern[str]): Pattern matching the CodeRabbit section header. + Returns: + str: Extracted release notes, or an empty string if none found. + """
219-226: Minor: clean up docstring typo“contains_issue_mentionscontains” appears duplicated.
- """ - Checks if the pull request contains issue mentions. - bool: True if the pull request contains_issue_mentionscontains issue mentions, False otherwise. - """ + """ + Checks if the pull request contains issue mentions. + Returns: + bool: True if issue mentions are present, otherwise False. + """tests/conftest.py (2)
24-24: Remove unused import Commit
Commitisn’t used in this module.-from github.Commit import Commit
221-233: Don’t overwrite labels; include both skip labelsYou assign
pull.labelstwice, losing the first label.- pull.labels = [label1] + pull.labels = [label1] @@ - pull.labels = [label2] + pull.labels = [label1, label2]tests/release_notes/test_record_factory.py (1)
195-396: Prefer non-Yoda asserts; let Ruff auto-fix SIM300A number of assertions use Yoda-style comparisons (e.g.,
1 == x). Preferx == 1. Ruff SIM300 will auto-fix.If you want, I can run through and submit a follow-up commit flipping these across the file or provide a patch generated by ruff --fix.
Also applies to: 400-446
release_notes_generator/model/issue_record.py (3)
86-90: Ensure string formatting for authors in to_chapter_rowMirror the PR record change so custom formats that include
{authors}get a clean string.- format_values["authors"] = self.authors if self.authors is not None else "" + format_values["authors"] = ", ".join(self.authors) if self.authors else ""
15-18: Remove unused TYPE_CHECKING import
PullRequestRecordisn’t referenced in this module.-# Use TYPE_CHECKING for imports needed only for type hints -if TYPE_CHECKING: - from release_notes_generator.model.pull_request_record import PullRequestRecord
241-252: Complete and correct CodeRabbit docstringCopy the improved docstring used in PullRequestRecord for consistency.
- """ - Extracts release notes from a pull request body formatted for Code Rabbit. - Parameters: - pull (PullRequestRecord): The pull request from which to extract release notes. - line_marks (list[str]): A list of characters that indicate the start of a release notes section. - cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code - Returns: - str: The extracted release notes as a string. If no release notes are found, returns - """ + """ + Extract release notes from a pull request body formatted for CodeRabbit. + Parameters: + pull (PullRequest): PR to extract from. + line_marks (list[str]): Bullet characters that denote release note lines. + cr_detection_regex (re.Pattern[str]): Pattern matching the CodeRabbit section header. + Returns: + str: Extracted release notes, or an empty string if none found. + """release_notes_generator/model/record.py (2)
17-19: Fix module docstring (mentions BaseChapters, not Record)Correct the module docstring to describe the Record abstraction.
-""" -This module contains the BaseChapters class, which is responsible for representing the base chapters. -""" +""" +Abstraction for release-notes records (IssueRecord, PullRequestRecord, CommitRecord). +Defines the interface and common behaviors shared by all records. +"""
138-166: Consider aligning contain_all_labels semantics with name (optional)Current implementation enforces exact equality of label sets (same size and contents). If the intent is “contains all of the specified labels,” it should check that each requested label is present, regardless of extras.
- def contain_all_labels(self, labels: list[str]) -> bool: + def contain_all_labels(self, labels: list[str]) -> bool: @@ - if len(self.labels) != len(labels): - return False - - for lbl in self.labels: - if lbl not in labels: - return False - return True + return all(label in self.labels for label in labels)If you truly want exact match semantics, consider renaming to
labels_equal_to.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
release_notes_generator/miner.py(3 hunks)release_notes_generator/model/commit_record.py(1 hunks)release_notes_generator/model/custom_chapters.py(2 hunks)release_notes_generator/model/issue_record.py(1 hunks)release_notes_generator/model/pull_request_record.py(1 hunks)release_notes_generator/model/record.py(4 hunks)release_notes_generator/model/service_chapters.py(7 hunks)release_notes_generator/record/record_factory.py(5 hunks)release_notes_generator/utils/constants.py(0 hunks)tests/conftest.py(16 hunks)tests/release_notes/model/test_commit_record.py(1 hunks)tests/release_notes/model/test_record.py(1 hunks)tests/release_notes/model/test_service_chapters.py(2 hunks)tests/release_notes/test_record_factory.py(8 hunks)tests/release_notes/test_release_notes_builder.py(22 hunks)tests/utils/test_pull_reuqest_utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- release_notes_generator/utils/constants.py
🧰 Additional context used
🧬 Code graph analysis (15)
tests/release_notes/model/test_commit_record.py (2)
release_notes_generator/model/commit_record.py (9)
CommitRecord(13-75)commit(49-54)record_id(27-28)is_closed(31-32)is_open(35-36)labels(39-40)authors(43-44)to_chapter_row(58-69)get_rls_notes(71-73)release_notes_generator/model/record.py (9)
record_id(60-65)is_closed(69-74)is_open(78-83)labels(87-92)skip(54-56)authors(96-101)to_chapter_row(106-110)present_in_chapters(130-136)get_rls_notes(113-119)
tests/utils/test_pull_reuqest_utils.py (2)
release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(51-71)release_notes_generator/action_inputs.py (1)
ActionInputs(60-452)
release_notes_generator/model/pull_request_record.py (3)
release_notes_generator/action_inputs.py (8)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_link_pr(316-321)get_row_format_pr(303-313)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (8)
Record(29-184)skip(54-56)authors(96-101)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)release_notes_generator/model/issue_record.py (6)
authors(56-59)to_chapter_row(73-95)get_rls_notes(97-121)_get_rls_notes_default(206-239)_get_rls_notes_code_rabbit(241-290)issue(64-69)
release_notes_generator/model/custom_chapters.py (4)
release_notes_generator/model/base_chapters.py (1)
BaseChapters(26-90)release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)record_id(27-28)labels(39-40)release_notes_generator/model/issue_record.py (4)
IssueRecord(20-290)record_id(40-41)pull_requests_count(184-190)labels(52-53)release_notes_generator/model/record.py (3)
Record(29-184)record_id(60-65)labels(87-92)
tests/release_notes/model/test_record.py (1)
release_notes_generator/model/record.py (15)
Record(29-184)skip(54-56)labels(87-92)authors(96-101)record_id(60-65)is_closed(69-74)is_open(78-83)to_chapter_row(106-110)get_rls_notes(113-119)is_present_in_chapters(45-51)added_into_chapters(122-128)present_in_chapters(130-136)contains_min_one_label(138-149)contain_all_labels(151-165)contains_release_notes(167-184)
tests/release_notes/model/test_service_chapters.py (2)
tests/conftest.py (4)
service_chapters(72-73)pull_request_record_merged(455-458)pull_request_record_closed(480-483)issue_request_record_with_merged_pr_with_issue_mentioned(469-476)release_notes_generator/model/service_chapters.py (1)
populate(96-137)
release_notes_generator/model/issue_record.py (3)
release_notes_generator/action_inputs.py (7)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_issue(288-300)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (12)
Record(29-184)skip(54-56)record_id(60-65)is_closed(69-74)is_open(78-83)labels(87-92)authors(96-101)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)release_notes_generator/record/record_factory.py (1)
register_pull_request(59-84)
tests/release_notes/test_release_notes_builder.py (3)
release_notes_generator/model/pull_request_record.py (3)
record_id(34-35)pull_request(75-80)labels(50-51)release_notes_generator/model/record.py (2)
record_id(60-65)labels(87-92)tests/conftest.py (5)
pull_request_record_merged(455-458)pull_request_record_closed(480-483)pull_request_record_open(462-465)MockLabel(43-45)pull_request_record_closed_with_skip_label(487-493)
release_notes_generator/model/commit_record.py (2)
release_notes_generator/action_inputs.py (1)
get_duplicity_icon(175-179)release_notes_generator/model/record.py (12)
Record(29-184)skip(54-56)record_id(60-65)is_closed(69-74)is_open(78-83)labels(87-92)authors(96-101)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)
release_notes_generator/miner.py (3)
release_notes_generator/model/issue_record.py (1)
IssueRecord(20-290)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/pull_request_record.py (1)
PullRequestRecord(16-312)
tests/conftest.py (3)
release_notes_generator/model/issue_record.py (5)
IssueRecord(20-290)issue(64-69)register_pull_request(159-166)record_id(40-41)register_commit(168-182)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(16-312)record_id(34-35)register_commit(188-195)release_notes_generator/record/record_factory.py (1)
register_pull_request(59-84)
release_notes_generator/record/record_factory.py (7)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)commit(49-54)labels(39-40)release_notes_generator/model/issue_record.py (6)
register_pull_request(159-166)issue(64-69)get_pull_request_numbers(151-157)get_pull_request(125-135)register_commit(168-182)labels(52-53)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(60-452)get_skip_release_notes_labels(190-198)release_notes_generator/model/pull_request_record.py (3)
register_commit(188-195)is_commit_sha_present(197-205)labels(50-51)release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(51-71)release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)
release_notes_generator/model/service_chapters.py (4)
release_notes_generator/model/commit_record.py (5)
CommitRecord(13-75)is_closed(31-32)record_id(27-28)is_open(35-36)to_chapter_row(58-69)release_notes_generator/model/issue_record.py (6)
IssueRecord(20-290)is_closed(44-45)record_id(40-41)is_open(48-49)pull_requests_count(184-190)to_chapter_row(73-95)release_notes_generator/model/pull_request_record.py (7)
PullRequestRecord(16-312)is_closed(38-43)record_id(34-35)is_open(46-47)to_chapter_row(112-128)is_merged(62-72)contains_issue_mentions(219-226)release_notes_generator/model/record.py (6)
Record(29-184)is_closed(69-74)record_id(60-65)is_open(78-83)to_chapter_row(106-110)contains_min_one_label(138-149)
tests/release_notes/test_record_factory.py (5)
release_notes_generator/model/commit_record.py (2)
CommitRecord(13-75)commit(49-54)release_notes_generator/model/issue_record.py (4)
IssueRecord(20-290)pull_requests_count(184-190)get_pull_request(125-135)get_commit(137-149)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/pull_request_record.py (5)
PullRequestRecord(16-312)get_commit(167-177)issues_count(207-211)pull_request(75-80)commits_count(213-217)release_notes_generator/record/record_factory.py (2)
RecordFactory(43-161)generate(49-119)
release_notes_generator/model/record.py (4)
release_notes_generator/model/commit_record.py (7)
record_id(27-28)is_closed(31-32)is_open(35-36)labels(39-40)authors(43-44)to_chapter_row(58-69)get_rls_notes(71-73)release_notes_generator/model/issue_record.py (7)
record_id(40-41)is_closed(44-45)is_open(48-49)labels(52-53)authors(56-59)to_chapter_row(73-95)get_rls_notes(97-121)release_notes_generator/model/pull_request_record.py (7)
record_id(34-35)is_closed(38-43)is_open(46-47)labels(50-51)authors(54-57)to_chapter_row(112-128)get_rls_notes(130-151)tests/release_notes/model/test_record.py (7)
record_id(29-30)is_closed(33-34)is_open(37-38)labels(41-42)authors(45-46)to_chapter_row(48-49)get_rls_notes(51-52)
🪛 Ruff (0.12.2)
tests/utils/test_pull_reuqest_utils.py
23-23: release_notes_generator.utils.pull_request_utils.get_issues_for_pr imported but unused
Remove unused import: release_notes_generator.utils.pull_request_utils.get_issues_for_pr
(F401)
release_notes_generator/model/pull_request_record.py
97-97: Local variable contributors is assigned to but never used
Remove assignment to unused variable contributors
(F841)
tests/release_notes/model/test_service_chapters.py
52-52: Yoda condition detected
(SIM300)
release_notes_generator/model/issue_record.py
17-17: release_notes_generator.model.pull_request_record.PullRequestRecord imported but unused
Remove unused import: release_notes_generator.model.pull_request_record.PullRequestRecord
(F401)
146-147: Use a single if statement instead of nested if statements
(SIM102)
tests/conftest.py
24-24: github.Commit.Commit imported but unused
Remove unused import: github.Commit.Commit
(F401)
tests/release_notes/test_record_factory.py
205-205: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
251-251: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
252-252: Yoda condition detected
Rewrite as rec_i2.pull_requests_count() == 1
(SIM300)
282-282: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
292-292: Yoda condition detected
Rewrite as rec_issue1.pull_requests_count() == 0
(SIM300)
293-293: Yoda condition detected
Rewrite as rec_issue2.pull_requests_count() == 1
(SIM300)
321-321: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
330-330: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
331-331: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
337-337: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
338-338: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
363-363: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
375-375: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
376-376: Yoda condition detected
Rewrite as rec_pr2.issues_count() == 0
(SIM300)
377-377: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
378-378: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
395-395: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
400-400: Yoda condition detected
(SIM300)
401-401: Yoda condition detected
(SIM300)
439-439: Yoda condition detected
(SIM300)
440-440: Yoda condition detected
(SIM300)
443-443: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
444-444: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
🪛 GitHub Actions: Build and Test
release_notes_generator/model/pull_request_record.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to apply formatting.
release_notes_generator/model/commit_record.py
[error] 66-66: Step 'pytest --cov=. -v tests/ --cov-fail-under=80' failed. TypeError: 'bool' object is not callable in release_notes_generator/model/commit_record.py:66 when evaluating contains_release_notes() in CommitRecord.to_chapter_row(). This happened because the test patched contains_release_notes as a property (boolean) instead of a method.
release_notes_generator/model/record.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to apply formatting.
🔇 Additional comments (13)
release_notes_generator/miner.py (1)
32-35: Good migration to class-scoped state constants.Replacing module-level constants with
IssueRecord/PullRequestRecordclass attributes is clear and keeps ownership local to the model types.Also applies to: 67-71, 121-131
tests/release_notes/test_release_notes_builder.py (1)
536-540: Migration torecords={rec.record_id: rec}looks correct.This aligns tests with the abstract
Record.record_idcontract across Issue/PR/Commit types.Also applies to: 557-560, 575-578, 593-596, 611-614, 629-632, 645-649, 661-665, 680-684, 699-703, 718-721, 737-741, 756-759, 774-777, 792-795, 811-814, 830-833, 866-869, 884-887, 901-904, 919-922
tests/release_notes/model/test_record.py (1)
19-53: Nice lightweightDummyRecordfor exercising the abstract API.Clear, isolated, and keeps Record tests focused on the contract. Good use of controllable defaults to hit different branches.
tests/release_notes/model/test_commit_record.py (1)
35-46: LGTM: duplicity icon behavior verified correctly.Good use of monkeypatch to inject a custom icon and verify that the first row has no prefix and the second has it, aligned with the internal conversion-count semantics.
tests/release_notes/model/test_service_chapters.py (3)
49-51: LGTM: updated fixture and record-id usage.Switching to
pull_request_record_mergedand using an explicit key123aligns with the refactored model keyed by record_id.
56-57: LGTM: closed PR path still exercises the correct chapter.The fixture rename and usage remain semantically equivalent.
80-81: LGTM: fixture rename aligns with new IssueRecord/PR linkage.This test now correctly reflects the “open issue with merged PR” flow.
release_notes_generator/model/commit_record.py (1)
66-73: Ensurecontains_release_notesonly flags non-whitespace notesThe current implementation in
release_notes_generator/model/record.pytreats any string (even empty or whitespace-only) as “present,” which causesto_chapter_rowincommit_record.pyto append spurious blank lines. Update the detection logic to only consider truly non-empty, non-whitespace content as release notes:--- a/release_notes_generator/model/record.py +++ b/release_notes_generator/model/record.py @@ -43,7 +43,10 @@ class Record: def contains_release_notes(self, re_check: bool = False) -> bool: """ Checks if the record contains release notes. - self._is_release_note_detected = self._rls_notes is not None + # Only count non-empty, non-whitespace strings as “present” + self._is_release_note_detected = bool( + self._rls_notes and self._rls_notes.strip() + ) return self._is_release_note_detected• In
commit_record.py,get_rls_notes()still returns""for direct commits, so this change prevents empty notes from triggering a\nin the output.
• After applying this patch (and updating any test monkeypatches), verify locally that:
test_to_chapter_row_with_release_notes_injectedstill passes, injecting real notes correctlytest_get_rls_notes_emptyno longer produces a trailing newline for direct commitsPlease run your test suite (e.g.
python -m pytest -q tests/release_notes/model/test_commit_record.py) to confirm no regressions.release_notes_generator/model/service_chapters.py (1)
112-120: LGTM: type-aware routing via isinstance guards.The explicit dispatch to IssueRecord, PullRequestRecord, and CommitRecord improves readability and safety.
release_notes_generator/model/pull_request_record.py (2)
22-24: State constants and is_open/is_closed semantics look saneUsing class-local constants and distinguishing closed-unmerged vs. merged via
is_closed()vsis_merged()is clear.
130-151: Heads-up: empty string release notes are treated as “present” by base class
get_rls_notesreturns""when nothing is found. InRecord.contains_release_notes,self._is_release_note_detected = self._rls_notes is not Nonewill mark empty strings as present. See my fix suggested on Record.contains_release_notes.Do you want me to push a PR change to base
Recordto treat empty strings as “no release notes” and re-run the tests?tests/conftest.py (1)
446-452: Fixture name vs return type mismatch
pull_request_record_no_rls_notesreturns an IssueRecord, not a PullRequestRecord. Either rename for clarity or return a PR record.Would you like me to (a) rename the fixture to
issue_record_with_pull_no_rls_notesand update references, or (b) make it returnPullRequestRecord(pull=mock_pull_no_rls_notes)?release_notes_generator/model/record.py (1)
1-1: Run Black on this file (pipeline still failing)
CI indicates that Black would reformatrelease_notes_generator/model/record.py, but theblackcommand wasn’t found on your PATH. Please format and re-commit this file before pushing:
- Either add
~/.local/binto your PATH so thatblack release_notes_generator/model/record.pyworks- Or invoke Black via Python directly:
python -m black release_notes_generator/model/record.pyOnce formatted, re-run CI to confirm the pipeline passes.
Optional lint improvements (ruff SIM110)
Ruff suggests simplifying the manual loops in the label‐checking methods:
- At line 146:
- for lbl in self.labels:
if lbl in labels:return True- return False
- return any(lbl in labels for lbl in self.labels)
- At line 162: ```diff - for lbl in self.labels: - if lbl not in labels: - return False - return True + return all(lbl in labels for lbl in self.labels)
| monkeypatch.setattr( | ||
| CommitRecord, | ||
| "contains_release_notes", | ||
| property(lambda self: True), | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix monkeypatch: patch the method, not a property (pipeline failure).
CommitRecord.to_chapter_row calls self.contains_release_notes(). The test replaces it with a property returning True, which turns the callable into a bool and crashes with TypeError: 'bool' object is not callable. Patch the method signature instead.
Apply this diff:
- monkeypatch.setattr(
- CommitRecord,
- "contains_release_notes",
- property(lambda self: True),
- )
+ # Patch the method so it remains callable by production code
+ monkeypatch.setattr(
+ CommitRecord,
+ "contains_release_notes",
+ lambda self, re_check=False: True,
+ )Additionally, the root cause is that the base Record.contains_release_notes treats empty strings as present (see my comment and proposed fix in release_notes_generator/model/record.py). Once that is fixed, you won’t need to force this behavior in tests except where you explicitly test the injection path.
Run this to ensure no other tests patch it as a property:
🏁 Script executed:
#!/bin/bash
# Find improper property monkeypatches of contains_release_notes
rg -nP 'contains_release_notes"\s*,\s*property\(' -C2Length of output: 55
I’ll run a broader search to catch any multiline/property patches for contains_release_notes across the tests.
🏁 Script executed:
#!/bin/bash
# 1) Find all monkeypatch.setattr calls touching contains_release_notes
rg -nP 'monkeypatch\.setattr.*contains_release_notes' -C4
# 2) Multiline search for property usage on contains_release_notes
rg -nP '(?s)contains_release_notes".*?property\(' -C3Length of output: 115
I’ll verify the actual definitions in the code to confirm how contains_release_notes is implemented and called:
🏁 Script executed:
#!/bin/bash
# 1) Locate CommitRecord and inspect its methods
rg -nP "class CommitRecord" -C5 --type py
# 2) Find the implementation of contains_release_notes
rg -nP "def contains_release_notes" -C5 --type py
# 3) Find where to_chapter_row calls contains_release_notes
rg -nP "to_chapter_row" -C5 --type pyLength of output: 25714
Fix monkeypatch in test_commit_record.py: patch the method, not a property
The test at tests/release_notes/model/test_commit_record.py (lines 62–66) currently replaces contains_release_notes with a property returning True. However, CommitRecord.to_chapter_row invokes self.contains_release_notes() (and may pass the optional re_check argument), so replacing it with a property causes a TypeError ('bool' object is not callable).
Please apply this diff:
--- a/tests/release_notes/model/test_commit_record.py
+++ b/tests/release_notes/model/test_commit_record.py
@@ -62,7 +62,8 @@ def test_to_chapter_row_with_release_notes_injected(monkeypatch):
- monkeypatch.setattr(
- CommitRecord,
- "contains_release_notes",
- property(lambda self: True),
- )
+ # Patch the method so it remains callable by production code
+ monkeypatch.setattr(
+ CommitRecord,
+ "contains_release_notes",
+ lambda self, re_check=False: True,
+ )Additionally, once the base implementation in release_notes_generator/model/record.py is updated to correctly handle empty strings in Record.contains_release_notes, you can remove this forced override except in tests explicitly targeting the injection path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| monkeypatch.setattr( | |
| CommitRecord, | |
| "contains_release_notes", | |
| property(lambda self: True), | |
| ) | |
| # Patch the method so it remains callable by production code | |
| monkeypatch.setattr( | |
| CommitRecord, | |
| "contains_release_notes", | |
| lambda self, re_check=False: True, | |
| ) |
🤖 Prompt for AI Agents
In tests/release_notes/model/test_commit_record.py around lines 62–66, the test
incorrectly monkeypatches CommitRecord.contains_release_notes as a property
returning True, but the code calls self.contains_release_notes(...) so patch the
method instead: replace the property patch with
monkeypatch.setattr(CommitRecord, "contains_release_notes", lambda self,
re_check=False: True) (i.e., a callable that accepts self and the optional
re_check argument). After the production Record.contains_release_notes is fixed
to handle empty strings, remove this forced override except in tests explicitly
verifying the injection path.
There was a problem hiding this comment.
@miroslavpojer Thank you for confirming the fix! The monkeypatch should now work correctly since contains_release_notes remains callable as expected by the production code.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
release_notes_generator/model/record.py (2)
151-166: Fix “contain_all_labels” logic (currently requires exact length match).Semantics of “contains all” should not require equal lengths; allow records to have extra labels. Also consider renaming to contains_all_labels for consistency with contains_min_one_label.
Apply this diff:
- def contain_all_labels(self, labels: list[str]) -> bool: + def contain_all_labels(self, labels: list[str]) -> bool: """ Check if the record contains all of the specified labels. @@ - if len(self.labels) != len(labels): - return False - - for lbl in self.labels: - if lbl not in labels: - return False - return True + return all(lbl in self.labels for lbl in labels)Optionally add a correctly named alias without breaking callers:
+ # Backward-compatible alias with correct grammar + def contains_all_labels(self, labels: list[str]) -> bool: + return self.contain_all_labels(labels)
167-185: Bug: contains_release_notes() treats empty string as “detected” and re_check does not force recomputation.get_rls_notes() returns "" when none are present; current code caches this as detected because it's not None. Also, re_check=True should invalidate cache.
Apply this diff:
- def contains_release_notes(self, re_check: bool = False) -> bool: + def contains_release_notes(self, re_check: bool = False) -> bool: @@ - if self._is_release_note_detected is not None and not re_check: - return self._is_release_note_detected + if self._is_release_note_detected is not None and not re_check: + return self._is_release_note_detected + + if re_check: + # Invalidate cached value and force recomputation + self._rls_notes = None + self._is_release_note_detected = None @@ - if self._rls_notes is None: - self._rls_notes = self.get_rls_notes() + if self._rls_notes is None: + self._rls_notes = self.get_rls_notes() @@ - self._is_release_note_detected = self._rls_notes is not None - return self._is_release_note_detected + # Consider non-empty, non-whitespace content as "present" + self._is_release_note_detected = bool(self._rls_notes and self._rls_notes.strip()) + return self._is_release_note_detected
♻️ Duplicate comments (4)
release_notes_generator/model/pull_request_record.py (4)
220-227: Fix truncated/duplicated docstring.Clean up wording and add a proper Returns section (duplicate of prior review).
Apply this diff:
def contains_issue_mentions(self) -> bool: """ - Checks if the pull request contains issue mentions. - bool: True if the pull request contains_issue_mentionscontains issue mentions, False otherwise. + Checks if the pull request contains issue mentions. + + Returns: + bool: True if the pull request contains issue mentions, False otherwise. """
268-275: Complete and correct the _get_rls_notes_code_rabbit docstring.The parameter type for pull is PullRequest, and the Returns section is cut off (duplicate of prior review).
Apply this diff:
def _get_rls_notes_code_rabbit( self, pull: PullRequest, line_marks: list[str], cr_detection_regex: re.Pattern[str] ) -> str: """ Extracts release notes from a pull request body formatted for Code Rabbit. Parameters: - pull (PullRequestRecord): The pull request from which to extract release notes. + pull (PullRequest): The pull request from which to extract release notes. line_marks (list[str]): A list of characters that indicate the start of a release notes section. - cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code + cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code Rabbit section. Returns: - str: The extracted release notes as a string. If no release notes are found, returns + str: The extracted release notes as a string. If no release notes are found, returns an empty string. """
83-110: Remove unreachable code and Ruff F841 (duplicate of prior review).Early returns make the rest unreachable; local variable contributors is unused. Keep this minimal until contributors are implemented from registered commits.
Apply this diff:
@property def contributors(self) -> list[str]: """ Gets the GitHub usernames of all contributors to the pull request. Returns: list[str]: A sorted list of GitHub usernames of contributors, excluding the main author. """ - if not self._pull_request: - return [] - - return [] - - # TODO - contributors have to be managed in future, make a research if get_commit can return some - # new information in compare to mined ones - # Get unique contributors from commits - contributors = set() - # for commit in self._pull_request.get_commits(): - # if commit.author and commit.author.login: - # contributors.add(f"@{commit.author.login}") - # - # # Remove the main author if they're also in contributors - # if self._pull_request.user and self._pull_request.user.login: - # author_name = f"@{self._pull_request.user.login}" - # if author_name in contributors: - # contributors.remove(author_name) - # - # return sorted(list(contributors)) + # TODO: compute contributors from registered commits in the future + return []
156-167: Fix incorrect indexing of issues dict (duplicate of prior review).self._issues is keyed by issue.number; list-style access by index is incorrect. Return by sorted position or introduce get_issue_by_number.
Apply this diff:
def get_issue(self, index: int = 0) -> Optional[Issue]: @@ - if index < 0 or index >= len(self._issues): - return None - return self._issues[index] + if index < 0 or index >= len(self._issues): + return None + issue_numbers = sorted(self._issues.keys()) + return self._issues[issue_numbers[index]]Optionally add a direct lookup by number:
+ def get_issue_by_number(self, number: int) -> Optional[Issue]: + return self._issues.get(number)
🧹 Nitpick comments (5)
release_notes_generator/model/record.py (2)
17-19: Fix incorrect module docstring (mentions BaseChapters instead of Record).The docstring should describe the abstract Record base class, not BaseChapters.
Apply this diff:
-""" -This module contains the BaseChapters class, which is responsible for representing the base chapters. -""" +""" +Abstract base class for records used in release notes (e.g., IssueRecord, PullRequestRecord, CommitRecord). +Defines the common interface and shared behavior across concrete record types. +"""
121-129: Counting conversions, not chapters — make counting explicit or deduplicate by chapter id.added_into_chapters() is currently called from to_chapter_row(), so it increments per render, not per unique chapter. Consider either passing a chapter identifier and counting uniques or renaming to reflect “render count”.
Apply this diff to support unique chapter ids while keeping backward compatibility:
- def added_into_chapters(self) -> None: + def added_into_chapters(self, chapter_id: Optional[str] = None) -> None: """ - Increments the count of chapters in which the record is present. + Registers presence in a chapter (unique by chapter_id if provided), otherwise increments render count. Returns: None """ - self._present_in_chapters += 1 - # TODO - this is wrong - it does not count chapters but conversions + if not hasattr(self, "_chapters_seen"): + self._chapters_seen: set[str] = set() + if chapter_id: + if chapter_id not in self._chapters_seen: + self._chapters_seen.add(chapter_id) + self._present_in_chapters += 1 + else: + # Fallback: keep legacy behavior (render count) + self._present_in_chapters += 1release_notes_generator/model/pull_request_record.py (3)
50-53: Add return type annotation to labels for consistency with Record.labels.Minor typing polish; aids static analysis and readability.
Apply this diff:
- def labels(self): + def labels(self) -> list[str]: return [label.name for label in self._pull_request.labels]
113-129: Optional: join authors for formatting instead of passing a raw list.Row format placeholders likely expect strings; passing a list will render as "['@user']". Joining improves output and keeps backward compatibility with default format.
Apply this diff:
- format_values["authors"] = self.authors if self.authors is not None else "" - format_values["contributors"] = self.contributors + format_values["authors"] = " ".join(self.authors) if self.authors else "" + format_values["contributors"] = " ".join(self.contributors) if self.contributors else ""
239-263: Default parser looks solid; consider tolerating variable indentation.If upstream content uses tabs or 4 spaces, strictly prefixing with two spaces may miss bullets. Not urgent; track as enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
release_notes_generator/model/pull_request_record.py(1 hunks)release_notes_generator/model/record.py(4 hunks)tests/release_notes/model/test_commit_record.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/release_notes/model/test_commit_record.py
🧰 Additional context used
🧬 Code graph analysis (2)
release_notes_generator/model/record.py (5)
tests/release_notes/model/test_base_chapters.py (1)
Record(22-23)release_notes_generator/model/pull_request_record.py (7)
record_id(35-36)is_closed(39-44)is_open(47-48)labels(51-52)authors(55-58)to_chapter_row(113-129)get_rls_notes(131-152)release_notes_generator/model/issue_record.py (7)
record_id(40-41)is_closed(44-45)is_open(48-49)labels(52-53)authors(56-59)to_chapter_row(73-95)get_rls_notes(97-121)release_notes_generator/model/commit_record.py (7)
record_id(27-28)is_closed(31-32)is_open(35-36)labels(39-40)authors(43-44)to_chapter_row(58-69)get_rls_notes(71-73)tests/release_notes/model/test_record.py (7)
record_id(29-30)is_closed(33-34)is_open(37-38)labels(41-42)authors(45-46)to_chapter_row(48-49)get_rls_notes(51-52)
release_notes_generator/model/pull_request_record.py (2)
release_notes_generator/action_inputs.py (8)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_link_pr(316-321)get_row_format_pr(303-313)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (7)
Record(29-184)skip(54-56)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)
🪛 Ruff (0.12.2)
release_notes_generator/model/pull_request_record.py
98-98: Local variable contributors is assigned to but never used
Remove assignment to unused variable contributors
(F841)
🔇 Additional comments (4)
release_notes_generator/model/record.py (2)
34-42: Good: centralizing release-notes defaults and caching fields.Class-level RELEASE_NOTE_LINE_MARKS and the cached fields (_is_release_note_detected, _rls_notes) set a clean foundation for concrete implementations.
130-137: Getter naming is fine; consider also exposing is_present_in_chapters to tests.present_in_chapters() returns the counter; combined with is_present_in_chapters, this is clear and useful.
release_notes_generator/model/pull_request_record.py (2)
131-153: LGTM: release-notes detection flow and fallback.Good separation of default vs CodeRabbit parsing controlled by ActionInputs.
198-206: Clarify semantics of is_commit_sha_present (merge SHA vs registered commits).Name implies “present among registered commits”, but implementation compares only to merge_commit_sha. If the intent is to check association, prefer dictionary membership; otherwise rename to is_merge_commit_sha.
Apply this diff if you intend membership among registered commits:
- return self._pull_request.merge_commit_sha == sha + return sha in self._commitsIf you intended to check merge commit only, consider:
- def is_commit_sha_present(self, sha: str) -> bool: + def is_merge_commit_sha(self, sha: str) -> bool: ...
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release_notes/test_record_factory.py (1)
212-214: Patch path is wrong: ActionInputs mocked under builder, but used by record_factoryRecordFactory imports/uses ActionInputs in its own module. Patching release_notes_generator.builder.ActionInputs won’t affect it; skip labels may not be applied in these tests.
Apply this change in both tests:
- mocker.patch("release_notes_generator.builder.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"]) + mocker.patch("release_notes_generator.record.record_factory.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"])- mocker.patch("release_notes_generator.builder.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes", "another-skip-label"]) + mocker.patch("release_notes_generator.record.record_factory.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes", "another-skip-label"])Also applies to: 343-345
♻️ Duplicate comments (2)
tests/release_notes/test_release_notes_builder.py (1)
802-808: Rename and behavior match: non-draft closed PR testRenaming to test_build_closed_pr_without_issue_non_draft and explicitly setting rec.pull_request.draft = False resolves the earlier naming mismatch.
release_notes_generator/model/pull_request_record.py (1)
84-110: Remove unreachable code and fix Ruff F841 in contributors propertyEarly return makes the rest of the method dead; also contributes an unused variable assignment flagged by Ruff.
Apply this cleanup:
@property def contributors(self) -> list[str]: @@ - if not self._pull_request: - return [] - - return [] - - # TODO - contributors have to be managed in future, make a research if get_commit can return some - # new information in compare to mined ones - # Get unique contributors from commits - contributors = set() - # for commit in self._pull_request.get_commits(): - # if commit.author and commit.author.login: - # contributors.add(f"@{commit.author.login}") - # - # # Remove the main author if they're also in contributors - # if self._pull_request.user and self._pull_request.user.login: - # author_name = f"@{self._pull_request.user.login}" - # if author_name in contributors: - # contributors.remove(author_name) - # - # return sorted(list(contributors)) + # TODO: compute contributors from registered commits in the future + return []
🧹 Nitpick comments (8)
tests/release_notes/test_release_notes_builder.py (2)
75-77: Deduplicate MockLabel definition; import the shared one from conftestMockLabel is already defined in tests/conftest.py. Reuse it to avoid drift if its behavior changes.
Apply this diff here to remove the duplicate class:
-class MockLabel: - def __init__(self, name): - self.name = nameAnd add this import near the other imports (outside this range):
from tests.conftest import MockLabel
821-822: Cover draft-but-not-merged PR scenario with a dedicated testYou left a TODO for the “PR not merged and in draft state” case. Happy path and non-draft paths are covered; this negative path would harden the service-chapter routing.
Do you want me to sketch a pytest for draft=True, merged=False closed PR to assert it lands in the correct service chapter (or is ignored), using pull_request_record_closed with rec.pull_request.draft = True?
release_notes_generator/model/pull_request_record.py (2)
50-53: Type annotation nit: declare labels -> list[str] for parity with Record.labelsSmall consistency improvement; helps static checkers.
- def labels(self): + def labels(self) -> list[str]: return [label.name for label in self._pull_request.labels]
118-126: Ensure authors formatting renders as string (not list repr) in custom row formatsIf a user enables {authors} in the PR row format, formatting a list will render "['@user']". Safer to join.
- format_values["authors"] = self.authors if self.authors is not None else "" + format_values["authors"] = ", ".join(self.authors) if self.authors else ""tests/release_notes/test_record_factory.py (2)
330-341: Style: avoid Yoda conditions in assertsUse expression == constant for readability and to match common project style. Repeat the change throughout the file where applicable.
- assert 0 == rec_pr1.issues_count() - assert 0 == rec_pr2.issues_count() + assert rec_pr1.issues_count() == 0 + assert rec_pr2.issues_count() == 0 @@ - assert 1 == rec_pr1.commits_count() - assert 1 == rec_pr2.commits_count() - assert commit1 == rec_pr1.get_commit('abc123') - assert commit2 == rec_pr2.get_commit('def456') + assert rec_pr1.commits_count() == 1 + assert rec_pr2.commits_count() == 1 + assert rec_pr1.get_commit('abc123') == commit1 + assert rec_pr2.get_commit('def456') == commit2
41-46: Optional: make closed PR mocks internally consistentstate="closed" but closed_at=None is inconsistent with PullRequestRecord.is_closed logic and GitHub semantics. Consider setting closed_at to datetime.now() in these helpers to avoid brittle tests later.
Also applies to: 58-63, 126-133, 142-147
release_notes_generator/model/issue_record.py (2)
15-18: Remove unused TYPE_CHECKING importPullRequestRecord isn’t referenced in this module. Drop the TYPE_CHECKING block to satisfy linters.
-# Use TYPE_CHECKING for imports needed only for type hints -if TYPE_CHECKING: - from release_notes_generator.model.pull_request_record import PullRequestRecord
86-90: Stringify authors before formatting the rowAs in PR records, join authors to render cleanly if the row format includes {authors}.
- format_values["authors"] = self.authors if self.authors is not None else "" + format_values["authors"] = ", ".join(self.authors) if self.authors else ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
release_notes_generator/model/commit_record.py(1 hunks)release_notes_generator/model/issue_record.py(1 hunks)release_notes_generator/model/pull_request_record.py(1 hunks)tests/release_notes/test_record_factory.py(8 hunks)tests/release_notes/test_release_notes_builder.py(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/model/commit_record.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/release_notes/test_release_notes_builder.py (4)
release_notes_generator/model/pull_request_record.py (3)
record_id(35-36)pull_request(76-81)labels(51-52)release_notes_generator/model/record.py (2)
record_id(60-65)labels(87-92)tests/conftest.py (5)
pull_request_record_merged(455-458)pull_request_record_closed(480-483)pull_request_record_open(462-465)MockLabel(43-45)pull_request_record_closed_with_skip_label(487-493)release_notes_generator/builder.py (1)
ReleaseNotesBuilder(32-86)
tests/release_notes/test_record_factory.py (4)
release_notes_generator/model/commit_record.py (2)
CommitRecord(13-75)commit(49-54)release_notes_generator/model/issue_record.py (4)
IssueRecord(20-291)pull_requests_count(185-191)get_pull_request(125-135)get_commit(137-150)release_notes_generator/model/pull_request_record.py (5)
PullRequestRecord(16-316)get_commit(168-178)issues_count(208-212)pull_request(76-81)commits_count(214-218)release_notes_generator/record/record_factory.py (2)
RecordFactory(43-161)generate(49-119)
release_notes_generator/model/issue_record.py (2)
release_notes_generator/action_inputs.py (7)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_issue(288-300)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (6)
Record(29-184)skip(54-56)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)
release_notes_generator/model/pull_request_record.py (3)
release_notes_generator/action_inputs.py (8)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_link_pr(316-321)get_row_format_pr(303-313)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (12)
Record(29-184)skip(54-56)record_id(60-65)is_closed(69-74)is_open(78-83)labels(87-92)authors(96-101)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)release_notes_generator/model/issue_record.py (10)
record_id(40-41)is_closed(44-45)is_open(48-49)labels(52-53)authors(56-59)to_chapter_row(73-95)get_rls_notes(97-121)_get_rls_notes_default(207-240)_get_rls_notes_code_rabbit(242-291)issue(64-69)
🪛 Ruff (0.12.2)
tests/release_notes/test_record_factory.py
205-205: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
251-251: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
252-252: Yoda condition detected
Rewrite as rec_i2.pull_requests_count() == 1
(SIM300)
282-282: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
292-292: Yoda condition detected
Rewrite as rec_issue1.pull_requests_count() == 0
(SIM300)
293-293: Yoda condition detected
Rewrite as rec_issue2.pull_requests_count() == 1
(SIM300)
321-321: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
330-330: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
331-331: Yoda condition detected
Rewrite as rec_pr2.issues_count() == 0
(SIM300)
337-337: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
338-338: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
363-363: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
375-375: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
376-376: Yoda condition detected
Rewrite as rec_pr2.issues_count() == 0
(SIM300)
377-377: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
378-378: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
395-395: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
400-400: Yoda condition detected
(SIM300)
401-401: Yoda condition detected
(SIM300)
439-439: Yoda condition detected
(SIM300)
440-440: Yoda condition detected
(SIM300)
443-443: Yoda condition detected
Rewrite as rec_pr1.issues_count() == 0
(SIM300)
444-444: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
release_notes_generator/model/issue_record.py
17-17: release_notes_generator.model.pull_request_record.PullRequestRecord imported but unused
Remove unused import: release_notes_generator.model.pull_request_record.PullRequestRecord
(F401)
release_notes_generator/model/pull_request_record.py
98-98: Local variable contributors is assigned to but never used
Remove assignment to unused variable contributors
(F841)
🔇 Additional comments (5)
tests/release_notes/test_release_notes_builder.py (3)
537-541: LGTM: Switched to record_id-based indexing in all ReleaseNotesBuilder callsConsistently passing records as {rec.record_id: rec} aligns with the new Record abstraction and avoids relying on entity-specific fields. Good sweep.
Also applies to: 557-561, 575-579, 593-597, 611-615, 629-633, 645-649, 661-665, 680-684, 699-703, 718-722, 738-742, 756-759, 774-777, 792-795, 811-814, 832-835, 868-871, 886-889, 903-906, 921-924
586-597: PR fixtures and expectations align with refactorUsing pull_request_record_merged and validating the service chapter output is consistent with splitting into specialized record types. No issues spotted here.
268-275: Expectation update for merged PRs linked to open issues is consistentBreaking out lines per issue (“I1 open in #101” and “I2 open in #102”) matches the model: PRs reference issues; issues don’t back-reference PRs. This looks correct.
release_notes_generator/model/pull_request_record.py (1)
38-45: Semantics check: is_closed relies on closed_at being setSome mocks initialize state="closed" but closed_at=None. Upstream GitHub objects typically set closed_at when state == "closed". Verify consumers/tests align with this, or relax the check if needed.
Would you like me to scan tests for closed PR mocks with closed_at=None and propose consistent values?
tests/release_notes/test_record_factory.py (1)
195-206: LGTM: Mapping semantics (IssueRecord + CommitRecord) are correctAsserting three records (issue + two direct commits) and verifying PR/commit registration through IssueRecord reflects the new factory behavior accurately.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
release_notes_generator/model/pull_request_record.py (1)
82-109: Delete unreachable code in contributors property (and fix Ruff F841).Early
return []makes the rest of the block dead;contributorsis unused.Apply this diff:
@property def contributors(self) -> list[str]: @@ - if not self._pull_request: - return [] - - return [] - - # TODO - contributors have to be managed in future, make a research if get_commit can return some - # new information in compare to mined ones - # Get unique contributors from commits - contributors = set() - # for commit in self._pull_request.get_commits(): - # if commit.author and commit.author.login: - # contributors.add(f"@{commit.author.login}") - # - # # Remove the main author if they're also in contributors - # if self._pull_request.user and self._pull_request.user.login: - # author_name = f"@{self._pull_request.user.login}" - # if author_name in contributors: - # contributors.remove(author_name) - # - # return sorted(list(contributors)) + if not self._pull_request: + return [] + # TODO: compute contributors from registered commits in the future + return []
🧹 Nitpick comments (10)
release_notes_generator/model/pull_request_record.py (5)
117-125: Provide strings for authors/contributors in row formatting (avoid list repr in output).Currently these are lists; formatting will render like "['@user']".
Apply this diff:
format_values["number"] = f"#{self._pull_request.number}" format_values["title"] = self._pull_request.title - format_values["authors"] = self.authors if self.authors is not None else "" - format_values["contributors"] = self.contributors + format_values["authors"] = ", ".join(self.authors) if self.authors else "" + format_values["contributors"] = ", ".join(self.contributors) if self.contributors else ""
130-151: Confirm whether release-notes title is a regex or a literal.You compile
ActionInputs.get_release_notes_title()directly. If this input is meant to be a literal string, it should be escaped withre.escapeto avoid unintended regex behavior.Would you like me to submit a patch that toggles escaping based on a new
RELEASE_NOTES_TITLE_IS_REGEXflag?
155-165: Remove default SHA value to avoid accidental “always-None” lookups.A default
"0"SHA is not meaningful and can mask call-site errors.Apply this diff:
- def get_commit(self, sha: str = "0") -> Optional[Commit]: + def get_commit(self, sha: str) -> Optional[Commit]:
176-185: Clarify merge-commit semantics of PR→commit association.
is_commit_sha_presentmatches onlymerge_commit_sha. That’s fine if you intend to register only the merge commit, but it’s worth documenting to avoid confusion with “all commits in the PR”.Add a short note to the docstring: “Note: this checks only the PR’s merge commit SHA.”
59-73: PR model lacks an issues registry; misaligned with linked issue #152 constraints.The objectives say: “PullRequest should maintain lists of associated issues and commits” and “Issues/Commits should not hold references back to PullRequests.” Currently, IssueRecord holds PRs, while PullRequestRecord does not track issues.
I can propose a minimal addition to PullRequestRecord:
_issues: dict[int, Issue]register_issue(issue: Issue) -> Noneissues_count() -> int,get_issue_numbers() -> list[int]…and adjust RecordFactory to populate PR issues, while trimming backreferences in IssueRecord if desired. Should I draft that refactor (plus test updates)?
tests/release_notes/test_record_factory.py (2)
201-201: Prefer isinstance over exact class identity to allow subclassing.Apply this diff:
- assert records[1].__class__ is IssueRecord + assert isinstance(records[1], IssueRecord)
205-206: Rewrite Yoda-style assertions for readability and Ruff SIM300 compliance.Apply this diff:
- assert 1 == rec_i1.pull_requests_count() + assert rec_i1.pull_requests_count() == 1 @@ - assert 1 == rec_i1.pull_requests_count() - assert 1 == rec_i2.pull_requests_count() + assert rec_i1.pull_requests_count() == 1 + assert rec_i2.pull_requests_count() == 1 @@ - assert 2 == len(records) + assert len(records) == 2 @@ - assert 0 == rec_issue1.pull_requests_count() - assert 1 == rec_issue2.pull_requests_count() + assert rec_issue1.pull_requests_count() == 0 + assert rec_issue2.pull_requests_count() == 1 @@ - assert 2 == len(records) + assert len(records) == 2 @@ - assert 1 == rec_pr1.commits_count() - assert 1 == rec_pr2.commits_count() + assert rec_pr1.commits_count() == 1 + assert rec_pr2.commits_count() == 1 @@ - assert 2 == len(records) + assert len(records) == 2 @@ - assert 1 == rec_pr1.commits_count() - assert 1 == rec_pr2.commits_count() + assert rec_pr1.commits_count() == 1 + assert rec_pr2.commits_count() == 1 @@ - assert 2 == len(records) + assert len(records) == 2 @@ - assert 0 == cast(IssueRecord, records[1]).pull_requests_count() - assert 0 == cast(IssueRecord, records[2]).pull_requests_count() + assert cast(IssueRecord, records[1]).pull_requests_count() == 0 + assert cast(IssueRecord, records[2]).pull_requests_count() == 0 @@ - assert 0 == cast(IssueRecord, records[1]).pull_requests_count() - assert 1 == cast(IssueRecord, records[2]).pull_requests_count() + assert cast(IssueRecord, records[1]).pull_requests_count() == 0 + assert cast(IssueRecord, records[2]).pull_requests_count() == 1 @@ - assert 1 == rec_pr1.commits_count() + assert rec_pr1.commits_count() == 1Also applies to: 251-252, 282-282, 292-293, 321-321, 334-335, 360-360, 372-373, 390-390, 395-396, 434-435, 438-438
release_notes_generator/model/issue_record.py (3)
78-91: Provide strings for authors in row formatting (avoid list repr).Apply this diff:
format_values["authors"] = self.authors if self.authors is not None else "" + # normalize to string for formatting + format_values["authors"] = ", ".join(self.authors) if self.authors else ""Alternatively, replace the first assignment entirely with the normalized one.
246-253: Fix and complete the CodeRabbit parser docstring.Parameter type/name and the “Returns” text are currently inconsistent/truncated.
Apply this diff:
- Extracts release notes from a pull request body formatted for Code Rabbit. - Parameters: - pull (PullRequestRecord): The pull request from which to extract release notes. + Extracts release notes from a pull request body formatted for CodeRabbit. + Parameters: + pull (PullRequest): The pull request from which to extract release notes. line_marks (list[str]): A list of characters that indicate the start of a release notes section. - cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the Code + cr_detection_regex (re.Pattern[str]): A regex pattern to detect the start of the CodeRabbit release notes section. Returns: str: The extracted release notes as a string. If no release notes are found, returns an empty string.
97-121: Confirm whether release-notes title is a regex or a literal.Same concern as in PullRequestRecord: If the title is not a regex, it should be escaped to avoid accidental matches.
I can add a small helper in Record to compile with/without escaping based on a flag, so both IssueRecord and PullRequestRecord share the logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
release_notes_generator/model/issue_record.py(1 hunks)release_notes_generator/model/pull_request_record.py(1 hunks)tests/release_notes/test_record_factory.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/release_notes/test_record_factory.py (4)
release_notes_generator/model/commit_record.py (2)
CommitRecord(13-75)commit(49-54)release_notes_generator/model/issue_record.py (4)
IssueRecord(20-292)pull_requests_count(185-191)get_pull_request(125-135)get_commit(137-150)release_notes_generator/model/pull_request_record.py (4)
PullRequestRecord(16-288)get_commit(155-165)pull_request(75-80)commits_count(186-190)release_notes_generator/record/record_factory.py (2)
RecordFactory(43-161)generate(49-119)
release_notes_generator/model/issue_record.py (3)
release_notes_generator/action_inputs.py (7)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_issue(288-300)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (6)
Record(29-184)skip(54-56)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)release_notes_generator/model/pull_request_record.py (3)
get_rls_notes(130-151)_get_rls_notes_default(203-236)_get_rls_notes_code_rabbit(238-288)
release_notes_generator/model/pull_request_record.py (2)
release_notes_generator/action_inputs.py (8)
ActionInputs(60-452)get_duplicity_icon(175-179)get_row_format_link_pr(316-321)get_row_format_pr(303-313)get_release_notes_title(212-217)is_coderabbit_support_active(220-224)get_coderabbit_release_notes_title(227-233)get_coderabbit_summary_ignore_groups(236-251)release_notes_generator/model/record.py (12)
Record(29-184)skip(54-56)record_id(60-65)is_closed(69-74)is_open(78-83)labels(87-92)authors(96-101)to_chapter_row(106-110)added_into_chapters(122-128)present_in_chapters(130-136)contains_release_notes(167-184)get_rls_notes(113-119)
🪛 Ruff (0.12.2)
tests/release_notes/test_record_factory.py
205-205: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
251-251: Yoda condition detected
Rewrite as rec_i1.pull_requests_count() == 1
(SIM300)
252-252: Yoda condition detected
Rewrite as rec_i2.pull_requests_count() == 1
(SIM300)
282-282: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
292-292: Yoda condition detected
Rewrite as rec_issue1.pull_requests_count() == 0
(SIM300)
293-293: Yoda condition detected
Rewrite as rec_issue2.pull_requests_count() == 1
(SIM300)
321-321: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
334-334: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
335-335: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
360-360: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
372-372: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
373-373: Yoda condition detected
Rewrite as rec_pr2.commits_count() == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as len(records) == 2
(SIM300)
395-395: Yoda condition detected
(SIM300)
396-396: Yoda condition detected
(SIM300)
434-434: Yoda condition detected
(SIM300)
435-435: Yoda condition detected
(SIM300)
438-438: Yoda condition detected
Rewrite as rec_pr1.commits_count() == 1
(SIM300)
release_notes_generator/model/issue_record.py
17-17: release_notes_generator.model.pull_request_record.PullRequestRecord imported but unused
Remove unused import: release_notes_generator.model.pull_request_record.PullRequestRecord
(F401)
release_notes_generator/model/pull_request_record.py
9-9: github.Issue.Issue imported but unused
Remove unused import: github.Issue.Issue
(F401)
97-97: Local variable contributors is assigned to but never used
Remove assignment to unused variable contributors
(F841)
| if self.contains_release_notes(): | ||
| row = f"{row}\n{self.get_rls_notes()}" | ||
| return row |
There was a problem hiding this comment.
Bug: empty release-notes still counted as present; causes spurious newline.
Record.contains_release_notes() treats empty string as present because it checks “is not None”. This makes to_chapter_row append a stray “\n” even when no notes exist.
Apply this fix in Record (outside this file) to compute truthiness correctly:
--- a/release_notes_generator/model/record.py
+++ b/release_notes_generator/model/record.py
@@
- def contains_release_notes(self, re_check: bool = False) -> bool:
+ def contains_release_notes(self, re_check: bool = False) -> bool:
@@
- if self._rls_notes is None:
- self._rls_notes = self.get_rls_notes()
-
- self._is_release_note_detected = self._rls_notes is not None
+ if self._rls_notes is None or re_check:
+ self._rls_notes = self.get_rls_notes()
+ self._is_release_note_detected = bool(self._rls_notes and self._rls_notes.strip())
return self._is_release_note_detectedIf you prefer a local guard here until Record is fixed, check notes := self.get_rls_notes() and append only if notes.strip() is truthy.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.contains_release_notes(): | |
| row = f"{row}\n{self.get_rls_notes()}" | |
| return row | |
| def contains_release_notes(self, re_check: bool = False) -> bool: | |
| if self._rls_notes is None or re_check: | |
| self._rls_notes = self.get_rls_notes() | |
| self._is_release_note_detected = bool(self._rls_notes and self._rls_notes.strip()) | |
| return self._is_release_note_detected |
🤖 Prompt for AI Agents
In release_notes_generator/model/pull_request_record.py around lines 126 to 128,
the method appends a newline and release notes even when get_rls_notes() returns
an empty or whitespace-only string because contains_release_notes() only checks
for None; to fix locally, call notes = self.get_rls_notes() and only append
f"\n{notes}" if notes and notes.strip() are truthy (otherwise return row
unchanged); alternatively, fix Record.contains_release_notes() to return a
truthy check of the notes value so this local guard isn’t needed.
There was a problem hiding this comment.
🧩 Analysis chain
@miroslavpojer Thank you for the update! Let me check the current implementation to verify that the issue has been properly addressed.
🏁 Script executed:
#!/bin/bash
# Check the current implementation of the to_chapter_row method in PullRequestRecord
ast-grep --pattern 'def to_chapter_row(self) -> str:
$$$'
Length of output: 5486
🏁 Script executed:
#!/bin/bash
# Also check the Record.contains_release_notes implementation to see if it was fixed there
ast-grep --pattern 'def contains_release_notes(self, re_check: bool = False) -> bool:
$$$'
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# Get the current content of the PullRequestRecord to_chapter_row method specifically
rg -A 10 "def to_chapter_row" release_notes_generator/model/pull_request_record.py
Length of output: 613
Perfect! I can see that you've properly addressed the issue. You fixed it at the Record base class level in contains_release_notes() by updating line 179 to:
self._is_release_note_detected = bool(self._rls_notes and self._rls_notes.strip())This is actually the better approach than applying local guards in each subclass because:
- Centralized fix: It solves the spurious newline issue for all record types (
PullRequestRecord,IssueRecord,CommitRecord) in one place - Proper logic: Now
contains_release_notes()correctly returnsFalsefor empty or whitespace-only strings, not justNone - No code duplication: Avoids repeating the same guard logic in multiple
to_chapter_row()implementations
The fix ensures that empty release notes won't cause unwanted newlines to be appended to the chapter rows across all record types. Well done! 👍
Release Notes:
Closes #152
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests