#129 - PR added to warning category despite having a registered label #157
Conversation
- Reported error cannot be re-simulated. Origin PR was closed without merge. - Fixed presence of pr among returned issues list from github API. - Fixed skipped closed PR during filtration. - Now PRs are in Release Notes visible as PR not an Issues.
WalkthroughInternalizes release-based filtering inside ReleaseNotesGenerator.generate(), removes the external filter parameter, expands FilterByRelease to include PRs closed since the cutoff, deduplicates issues that are PRs during mining, always creates issue records, and changes PR-body issue extraction to return a set. main.py callsite updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI / main.py
participant RNG as ReleaseNotesGenerator
participant Miner as Miner
participant Filter as FilterByRelease
participant RF as RecordFactory
participant Builder as ReleaseNotesBuilder
CLI->>RNG: generate()
RNG->>Miner: mine_data()
Miner-->>RNG: mined_data (issues, PRs, commits)
note over RNG,Miner: Deduplicate issues that correspond to PRs
RNG->>Filter: FilterByRelease.filter(mined_data)
Filter-->>RNG: data_filtered_by_release
RNG->>RF: RecordFactory.generate(data_filtered_by_release)
RF-->>RNG: records
RNG->>Builder: build(records, repo, release, changelog_url)
Builder-->>RNG: release_notes_str
RNG-->>CLI: release_notes_str
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
release_notes_generator/filter.py (1)
70-85: Fix TypeError and deduplicate PRs when including closed PRs.Comparisons against MagicMock in tests cause TypeError; also extending with closed PRs can duplicate PRs present in both merged_at and closed_at. Guard datetime types and de-dup by PR number.
Apply:
@@ -from copy import deepcopy +from copy import deepcopy +from datetime import datetime @@ - issues_list = list( - filter(lambda issue: issue.closed_at is not None and issue.closed_at >= data.since, data.issues) - ) + issues_list = [ + issue + for issue in data.issues + if getattr(issue, "closed_at", None) is not None + and isinstance(issue.closed_at, datetime) + and issue.closed_at >= data.since + ] @@ - pulls_list = list( - filter(lambda pull: pull.merged_at is not None and pull.merged_at >= data.since, data.pull_requests) - ) - pulls_list.extend( - filter(lambda pull: pull.closed_at is not None and pull.closed_at >= data.since, data.pull_requests) - ) + pulls_list_merged = [ + pr + for pr in data.pull_requests + if getattr(pr, "merged_at", None) is not None + and isinstance(pr.merged_at, datetime) + and pr.merged_at >= data.since + ] + pulls_list_closed = [ + pr + for pr in data.pull_requests + if getattr(pr, "closed_at", None) is not None + and isinstance(pr.closed_at, datetime) + and pr.closed_at >= data.since + ] + # de-duplicate by PR number (preserves order of first occurrence) + pulls_list = list({pr.number: pr for pr in [*pulls_list_merged, *pulls_list_closed]}.values())release_notes_generator/utils/pull_request_utils.py (1)
32-48: Restore list return type to satisfy callers/tests; keep uniqueness.Pipeline expects a list; returning a set breaks type/behavior. Return a sorted unique list.
Apply:
-@lru_cache(maxsize=None) -def extract_issue_numbers_from_body(pr: PullRequest) -> set[int]: +@lru_cache(maxsize=None) +def extract_issue_numbers_from_body(pr: PullRequest) -> list[int]: @@ - # Extract the issue numbers from the matches - issue_numbers = {int(match[-1]) for match in issue_matches} - - return issue_numbers + # Extract unique issue numbers and return as a sorted list + issue_numbers = sorted({int(match[-1]) for match in issue_matches}) + return issue_numbersrelease_notes_generator/generator.py (1)
68-76: Update docstring & remove stalefiltererargs in testsThe
generate()signature no longer accepts an externalfilterer, so both the docstring and the test call sites must be updated:• In
release_notes_generator/generator.py
– Remove the@Parameters:block describingfilterer.
– Ensure the docstring only documents the return value (filtering is now internal).• In
tests/test_release_notes_generator.py
– At lines 42, 77, 127, and 173, drop thefilterer=FilterByRelease()argument from allgenerate()calls.
– e.g.
diff - release_notes = ReleaseNotesGenerator(github_mock, custom_chapters).generate(filterer=FilterByRelease()) + release_notes = ReleaseNotesGenerator(github_mock, custom_chapters).generate()
🧹 Nitpick comments (4)
release_notes_generator/miner.py (1)
163-182: Avoid mutating input MinedData in a helper; return a new instance instead.Mutating
datain-place can surprise callers and complicate debugging. Prefer returning a copy with filtered issues.Apply:
- def __filter_duplicated_issues(self, data: MinedData) -> "MinedData": + def __filter_duplicated_issues(self, data: MinedData) -> "MinedData": @@ - pr_numbers = {pr.number for pr in data.pull_requests} - filtered_issues = [issue for issue in data.issues if issue.number not in pr_numbers] + pr_numbers = {pr.number for pr in data.pull_requests} + filtered_issues = [issue for issue in data.issues if issue.number not in pr_numbers] @@ - data.issues = filtered_issues - - return data + from copy import deepcopy + new_data = deepcopy(data) + new_data.issues = filtered_issues + return new_datamain.py (1)
59-59: LGTM on switching to internal filtering. Remove unused import.
FilterByReleaseis no longer used here.Apply:
-from release_notes_generator.filter import FilterByReleaserelease_notes_generator/record/record_factory.py (1)
100-109: Avoid duplicate GraphQL calls per PR.
get_issues_for_pris called twice for the same PR (in the if-check and inregister_pull_request). Cache per-PR within this loop to cut latency and rate-limit usage.Apply:
- for pull in data.pull_requests: + for pull in data.pull_requests: pull_labels = [label.name for label in pull.labels] skip_record: bool = any(item in pull_labels for item in ActionInputs.get_skip_release_notes_labels()) - - if not safe_call(get_issues_for_pr)(pull_number=pull.number) and not extract_issue_numbers_from_body(pull): + pr_linked = safe_call(get_issues_for_pr)(pull_number=pull.number) + body_linked = extract_issue_numbers_from_body(pull) + if not pr_linked and not body_linked: records[pull.number] = PullRequestRecord(pull, skip=skip_record) logger.debug("Created record for PR %d: %s", pull.number, pull.title) else: logger.debug(f"Registering pull number: {pull.number}, title : {pull.title}") - register_pull_request(pull, skip_record) + register_pull_request(pull, skip_record)release_notes_generator/generator.py (1)
28-28: Remove unused import (ruff F401).
Filteris no longer referenced after inlining the release filter; drop it to satisfy linting.-from release_notes_generator.filter import Filter, FilterByRelease +from release_notes_generator.filter import FilterByRelease
📜 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 (6)
main.py(1 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(2 hunks)release_notes_generator/record/record_factory.py(3 hunks)release_notes_generator/utils/pull_request_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
release_notes_generator/generator.py (4)
release_notes_generator/filter.py (4)
filter(32-39)filter(50-109)Filter(27-39)FilterByRelease(42-109)release_notes_generator/record/record_factory.py (2)
generate(49-123)RecordFactory(43-165)release_notes_generator/utils/utils.py (1)
get_change_url(31-56)release_notes_generator/action_inputs.py (2)
ActionInputs(60-452)get_tag_name(118-122)
release_notes_generator/record/record_factory.py (1)
release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(52-72)
main.py (1)
release_notes_generator/generator.py (1)
generate(68-101)
release_notes_generator/miner.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
🪛 GitHub Actions: Build and Test
release_notes_generator/utils/pull_request_utils.py
[error] 1-1: extract_issue_numbers_from_body() returned a set() instead of [] for PR body with no issues.
[error] 1-1: extract_issue_numbers_from_body() returned a set {123} instead of [123] for single issue.
[error] 1-1: extract_issue_numbers_from_body() returned a set {123, 456} instead of [123, 456] for multiple issues.
[error] 1-1: extract_issue_numbers_from_body() returned a set {123, 456} instead of [123, 456] for mixed-case keywords.
[error] 1-1: extract_issue_numbers_from_body() returned a set() instead of [] when PR body is None.
[error] 1-1: extract_issue_numbers_from_body() returned a set {123} instead of [123] for complex text with wrong syntax.
release_notes_generator/generator.py
[error] 1-1: Black formatting check failed. Command 'black --check $(git ls-files '*.py')' would reformat 1 file: release_notes_generator/generator.py. Run 'black --write' to fix formatting.
release_notes_generator/filter.py
[error] 80-80: TypeError: '>=' not supported between instances of 'MagicMock' and 'datetime.datetime' during filtering (PRs by release date).
🪛 Ruff (0.12.2)
release_notes_generator/generator.py
28-28: release_notes_generator.filter.Filter imported but unused
Remove unused import: release_notes_generator.filter.Filter
(F401)
🔇 Additional comments (4)
release_notes_generator/miner.py (1)
72-76: Good: run de-dup after mining to strip PRs from issues.This ordering avoids downstream double-counting. LGTM.
release_notes_generator/record/record_factory.py (1)
97-98: LGTM: always create issue records after upstream de-dup.Works well with the miner’s de-dup step.
release_notes_generator/generator.py (2)
82-83: LGTM: Centralizing release filtering here simplifies the API surface.
91-93: LGTM: Using the release-filtered data for record generation is correct.
release_notes_generator/generator.py
Outdated
| changelog_url: str = get_change_url( | ||
| tag_name=ActionInputs.get_tag_name(), repository=filtered_data.repository, git_release=filtered_data.release | ||
| tag_name=ActionInputs.get_tag_name(), repository=data_filtered_by_release.repository, git_release=data_filtered_by_release.release | ||
| ) | ||
|
|
||
| assert filtered_data.repository is not None, "Repository must not be None" | ||
| assert data_filtered_by_release.repository is not None, "Repository must not be None" | ||
|
|
There was a problem hiding this comment.
Fix Black failure and replace assert with a proper runtime guard.
- Black is flagging the long argument line; wrap the call.
- Avoid
assertfor essential runtime checks; use an explicit guard and returnNone. Also check before callingget_change_url.
- changelog_url: str = get_change_url(
- tag_name=ActionInputs.get_tag_name(), repository=data_filtered_by_release.repository, git_release=data_filtered_by_release.release
- )
-
- assert data_filtered_by_release.repository is not None, "Repository must not be None"
+ if data_filtered_by_release.repository is None:
+ logger.error("Repository must not be None")
+ return None
+
+ changelog_url: str = get_change_url(
+ tag_name=ActionInputs.get_tag_name(),
+ repository=data_filtered_by_release.repository,
+ git_release=data_filtered_by_release.release,
+ )📝 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.
| changelog_url: str = get_change_url( | |
| tag_name=ActionInputs.get_tag_name(), repository=filtered_data.repository, git_release=filtered_data.release | |
| tag_name=ActionInputs.get_tag_name(), repository=data_filtered_by_release.repository, git_release=data_filtered_by_release.release | |
| ) | |
| assert filtered_data.repository is not None, "Repository must not be None" | |
| assert data_filtered_by_release.repository is not None, "Repository must not be None" | |
| if data_filtered_by_release.repository is None: | |
| logger.error("Repository must not be None") | |
| return None | |
| changelog_url: str = get_change_url( | |
| tag_name=ActionInputs.get_tag_name(), | |
| repository=data_filtered_by_release.repository, | |
| git_release=data_filtered_by_release.release, | |
| ) |
🤖 Prompt for AI Agents
In release_notes_generator/generator.py around lines 85 to 90, the long
get_change_url call should be wrapped to satisfy Black and you must replace the
assert with an explicit runtime guard: first check if
data_filtered_by_release.repository is None and if so log or handle the error
and return None, then call get_change_url (with its arguments split across
multiple lines to avoid a long single line). Ensure you perform the repository
None check before invoking get_change_url and return early on failure instead of
using assert.
| def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: | ||
| detected_issues = extract_issue_numbers_from_body(pull) | ||
| detected_issues.extend(safe_call(get_issues_for_pr)(pull_number=pull.number)) | ||
| logger.debug(f"Detected issues - from body: {detected_issues}") | ||
| detected_issues.update(safe_call(get_issues_for_pr)(pull_number=pull.number)) | ||
| logger.debug(f"Detected issues - final: {detected_issues}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adapt to list return type and ensure uniqueness before updates.
Convert extracted issues to a set locally; logs remain correct.
Apply:
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None:
- detected_issues = extract_issue_numbers_from_body(pull)
- logger.debug(f"Detected issues - from body: {detected_issues}")
- detected_issues.update(safe_call(get_issues_for_pr)(pull_number=pull.number))
+ def register_pull_request(pull: PullRequest, skip_rec: bool) -> None:
+ detected_issues = set(extract_issue_numbers_from_body(pull))
+ logger.debug(f"Detected issues - from body: {detected_issues}")
+ detected_issues.update(safe_call(get_issues_for_pr)(pull_number=pull.number))
logger.debug(f"Detected issues - final: {detected_issues}")📝 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.
| def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: | |
| detected_issues = extract_issue_numbers_from_body(pull) | |
| detected_issues.extend(safe_call(get_issues_for_pr)(pull_number=pull.number)) | |
| logger.debug(f"Detected issues - from body: {detected_issues}") | |
| detected_issues.update(safe_call(get_issues_for_pr)(pull_number=pull.number)) | |
| logger.debug(f"Detected issues - final: {detected_issues}") | |
| def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: | |
| detected_issues = set(extract_issue_numbers_from_body(pull)) | |
| logger.debug(f"Detected issues - from body: {detected_issues}") | |
| detected_issues.update(safe_call(get_issues_for_pr)(pull_number=pull.number)) | |
| logger.debug(f"Detected issues - final: {detected_issues}") |
🤖 Prompt for AI Agents
In release_notes_generator/record/record_factory.py around lines 59 to 64,
extract_issue_numbers_from_body returns a list so convert it to a set
immediately (e.g., detected_issues = set(extract_issue_numbers_from_body(pull)))
to enforce uniqueness, and when merging issues from get_issues_for_pr wrap that
result in set(...) before calling detected_issues.update(...) (or coerce the
safe_call return into a set). Keep the existing debug logs unchanged so they
show the set values.
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/test_release_notes_generator.py (1)
26-26: Remove unused import to satisfy linters.FilterByRelease is no longer used in this test.
-from release_notes_generator.filter import FilterByRelease
♻️ Duplicate comments (1)
release_notes_generator/generator.py (1)
85-91: Replace assert with a runtime guard and call get_change_url only when repo is present.This mirrors prior feedback and avoids asserting in production paths.
- changelog_url: str = get_change_url( - tag_name=ActionInputs.get_tag_name(), - repository=data_filtered_by_release.repository, - git_release=data_filtered_by_release.release, - ) - - assert data_filtered_by_release.repository is not None, "Repository must not be None" + if data_filtered_by_release.repository is None: + logger.error("Repository must not be None") + return None + + changelog_url: str = get_change_url( + tag_name=ActionInputs.get_tag_name(), + repository=data_filtered_by_release.repository, + git_release=data_filtered_by_release.release, + )
🧹 Nitpick comments (2)
tests/release_notes/test_record_factory.py (1)
302-302: Test name now contradicts its intent; adjust for clarity or add a true mismatch case.You changed the body to "Closes #2", which matches the mocked GraphQL result. The test name suggests a wrong issue number in the PR body. Either rename the test or add a scenario where body mentions a different issue than GraphQL to validate merge semantics.
release_notes_generator/generator.py (1)
68-76: Docstring still references a filterer parameter; update for accuracy.def generate(self) -> Optional[str]: """ Generates the Release Notes for a given repository. - @Parameters: - - filterer: An instance of Filter that will be used to filter the mined data. - @return: The generated release notes as a string, or None if the repository could not be found. """
📜 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 (6)
release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(3 hunks)tests/release_notes/test_record_factory.py(2 hunks)tests/test_filter.py(1 hunks)tests/test_release_notes_generator.py(6 hunks)tests/utils/test_pull_reuqest_utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_filter.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/utils/test_pull_reuqest_utils.py (1)
release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-48)
tests/test_release_notes_generator.py (2)
release_notes_generator/generator.py (3)
ReleaseNotesGenerator(41-103)custom_chapters(59-61)generate(68-103)release_notes_generator/record/record_factory.py (1)
generate(49-123)
release_notes_generator/filter.py (1)
release_notes_generator/model/commit_record.py (1)
commit(49-54)
release_notes_generator/generator.py (4)
release_notes_generator/filter.py (4)
filter(32-39)filter(50-109)Filter(27-39)FilterByRelease(42-109)release_notes_generator/record/record_factory.py (2)
generate(49-123)RecordFactory(43-165)release_notes_generator/utils/utils.py (1)
get_change_url(31-56)release_notes_generator/action_inputs.py (2)
ActionInputs(60-452)get_tag_name(118-122)
🪛 Ruff (0.12.2)
release_notes_generator/generator.py
28-28: release_notes_generator.filter.Filter imported but unused
Remove unused import: release_notes_generator.filter.Filter
(F401)
🔇 Additional comments (5)
tests/release_notes/test_record_factory.py (1)
323-324: LGTM — minor formatting.No functional impact.
tests/utils/test_pull_reuqest_utils.py (1)
51-51: Switching assertions to sets matches the new return type.Good alignment with extract_issue_numbers_from_body returning Set[int].
Also update the function’s docstring in release_notes_generator/utils/pull_request_utils.py to say “set[int]” instead of “list[int]” to avoid confusion.
Also applies to: 58-58, 65-65, 72-72, 79-79, 91-91
tests/test_release_notes_generator.py (2)
42-42: LGTM — updated to new generate() API without external filterer.Also applies to: 77-77, 128-128, 176-176
108-111: LGTM — include closed_at to reflect filter behavior on closed PRs.Also applies to: 159-162
release_notes_generator/generator.py (1)
94-95: LGTM — RecordFactory now consumes filtered data.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
release_notes_generator/filter.py (1)
77-85: Order-preserving PR dedupe by number — LGTM.Switching from a set of PullRequest objects to a seen-number set preserves order and avoids hashability pitfalls. Matches prior feedback.
🧹 Nitpick comments (1)
release_notes_generator/filter.py (1)
79-85: Collapse nested if (Ruff SIM102) to simplify control flow.Equivalent logic, fewer branches.
- for pull in data.pull_requests: - if (pull.merged_at is not None and pull.merged_at >= data.since) or ( - pull.closed_at is not None and pull.closed_at >= data.since - ): - if pull.number not in pulls_seen: - pulls_seen.add(pull.number) - pulls_list.append(pull) + for pull in data.pull_requests: + if ( + ( + (pull.merged_at is not None and pull.merged_at >= data.since) + or (pull.closed_at is not None and pull.closed_at >= data.since) + ) + and pull.number not in pulls_seen + ): + pulls_seen.add(pull.number) + pulls_list.append(pull)
📜 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 (2)
release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/generator.py
🧰 Additional context used
🪛 Ruff (0.12.2)
release_notes_generator/filter.py
80-83: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (2)
release_notes_generator/filter.py (2)
88-89: Boundary consistency: use '>=' for commits to match “since” semantics of PR/issue filters.Currently commits use '>' while PRs/issues use '>='. Aligning avoids off-by-one at the cutoff timestamp.
- commits_list = list(filter(lambda commit: commit.commit.author.date > data.since, data.commits)) + commits_list = list(filter(lambda commit: commit.commit.author.date >= data.since, data.commits))Please run the test suite to confirm no behavior regressions around release boundaries.
77-78: Verify project specifies python_requires ≥3.9
I didn’t find anypython_requiresorrequires-pythondeclaration inpyproject.toml,setup.cfg, orsetup.py; ensure your project metadata includespython_requires=">=3.9"so that built-in generics (set[int],list[str], etc.) are supported per PEP 585.
miroslavpojer
deleted the
feature/129-PR-added-to-warning-category-despite-having-a-registered-label
branch
Release Notes:
Closes #129
Summary by CodeRabbit
New Features
Bug Fixes
Tests