#174 - Add logic for fetching cross-repo sub issues's PRs #186
Conversation
- Added mining of Pull Requests for cross-repo issues.
WalkthroughThe miner now returns a pair: fetched cross-repo issues and their PR mappings. MinedData gains a dict to store PRs of fetched cross-issues. Generator updates to consume the new return type and persist the PR mapping. DefaultRecordFactory registers those cross-repo PRs to corresponding issue records, introducing a new helper method (duplicated in the class). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Generator
participant DataMiner
participant MinedData
participant RecordFactory
User->>Generator: generate(release, hierarchy=True)
Generator->>DataMiner: mine_missing_sub_issues(data)
DataMiner-->>Generator: (fetchedIssues, crossIssuePRs)
Generator->>MinedData: issues.update(fetchedIssues)
Generator->>MinedData: set pull_requests_of_fetched_cross_issues = crossIssuePRs
Generator->>RecordFactory: build records from MinedData
RecordFactory->>RecordFactory: register PRs for in-repo issues
RecordFactory->>RecordFactory: register PRs for cross-repo issues (new)
RecordFactory-->>User: records
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/data/miner.py (1)
1-1: Fix Black formatting to unblock CI/CD.The Black formatting check failed, preventing the pipeline from passing. Run the formatter to resolve this blocking issue.
#!/bin/bash # Format the code with Black black release_notes_generator/data/miner.py
🧹 Nitpick comments (2)
release_notes_generator/data/miner.py (1)
87-104: LGTM with suggestions.The method signature change to return a tuple is correctly implemented and properly integrates the cross-repo PR fetching functionality.
Consider renaming
_fetch_missing_issues_and_prs(line 99) to_fetch_missing_issuessince it no longer returns PRs. The current name could be misleading to future maintainers.- fetched_issues = self._fetch_missing_issues_and_prs(data) + fetched_issues = self._fetch_missing_issues(data)And update the method name at its definition (line 128):
- def _fetch_missing_issues_and_prs(self, data: MinedData) -> dict[Issue, Repository]: + def _fetch_missing_issues(self, data: MinedData) -> dict[Issue, Repository]:release_notes_generator/record/factory/default_record_factory.py (1)
100-103: Simplify the condition check.Using
.items()in a boolean context is unnecessarily verbose. An empty dict is falsy in Python.Apply this diff to simplify the condition:
- if data.pull_requests_of_fetched_cross_issues.items(): + if data.pull_requests_of_fetched_cross_issues: logger.debug("Register cross-repo Pull Requests to its issues") for iid, prs in data.pull_requests_of_fetched_cross_issues.items(): self._register_cross_repo_prs_to_issue(iid, prs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
release_notes_generator/data/miner.py(5 hunks)release_notes_generator/generator.py(2 hunks)release_notes_generator/model/mined_data.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T14:13:38.979Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.979Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
Applied to files:
release_notes_generator/record/factory/default_record_factory.py
🧬 Code graph analysis (3)
release_notes_generator/data/miner.py (2)
release_notes_generator/model/mined_data.py (1)
MinedData(38-81)release_notes_generator/utils/record_utils.py (1)
get_id(24-45)
release_notes_generator/generator.py (1)
release_notes_generator/data/miner.py (1)
mine_missing_sub_issues(87-104)
release_notes_generator/record/factory/default_record_factory.py (1)
release_notes_generator/model/issue_record.py (1)
register_pull_request(199-206)
🪛 GitHub Actions: Build and Test
release_notes_generator/data/miner.py
[error] 1-1: Black formatting check failed. Command 'black --check $(git ls-files "*.py")' exited with code 1. 1 file would be reformatted.
🪛 Ruff (0.13.3)
release_notes_generator/data/miner.py
362-362: Unused method argument: data
(ARG002)
🔇 Additional comments (3)
release_notes_generator/generator.py (1)
96-98: LGTM!The tuple unpacking and assignment logic correctly integrates the new cross-repo PR fetching functionality. The code properly updates the filtered data with both fetched issues and their associated PRs.
release_notes_generator/model/mined_data.py (1)
53-54: LGTM!The new attribute
pull_requests_of_fetched_cross_issuesis properly typed, documented, and initialized. It follows the existing patterns in theMinedDataclass and correctly supports the cross-repo PR mapping feature.release_notes_generator/data/miner.py (1)
176-177: LGTM!The else clause provides appropriate error logging when the repository cannot be fetched for a cross-repo issue.
| def _register_cross_repo_prs_to_issue(self, iid: str, prs: list[PullRequest]) -> None: | ||
| for pr in prs: | ||
| cast(IssueRecord, self._records[iid]).register_pull_request(pr) |
There was a problem hiding this comment.
Add validation before casting and accessing records.
The method lacks validation that could prevent runtime errors:
- No check that
iidexists inself._records(could raiseKeyError) - No check that the record is an
IssueRecordbefore casting (could raiseAttributeErrorif the wrong record type)
Apply this diff to add defensive checks:
def _register_cross_repo_prs_to_issue(self, iid: str, prs: list[PullRequest]) -> None:
+ if iid not in self._records:
+ logger.warning("Issue %s not found in records. Skipping cross-repo PR registration.", iid)
+ return
+ if not isinstance(self._records[iid], IssueRecord):
+ logger.warning("Record %s is not an IssueRecord. Skipping cross-repo PR registration.", iid)
+ return
for pr in prs:
cast(IssueRecord, self._records[iid]).register_pull_request(pr)📝 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_cross_repo_prs_to_issue(self, iid: str, prs: list[PullRequest]) -> None: | |
| for pr in prs: | |
| cast(IssueRecord, self._records[iid]).register_pull_request(pr) | |
| def _register_cross_repo_prs_to_issue(self, iid: str, prs: list[PullRequest]) -> None: | |
| - for pr in prs: | |
| if iid not in self._records: | |
| logger.warning( | |
| "Issue %s not found in records. Skipping cross-repo PR registration.", | |
| iid, | |
| ) | |
| return | |
| if not isinstance(self._records[iid], IssueRecord): | |
| logger.warning( | |
| "Record %s is not an IssueRecord. Skipping cross-repo PR registration.", | |
| iid, | |
| ) | |
| return | |
| for pr in prs: | |
| cast(IssueRecord, self._records[iid]).register_pull_request(pr) |
🤖 Prompt for AI Agents
In release_notes_generator/record/factory/default_record_factory.py around lines
188 to 190, the helper assumes self._records[iid] exists and is an IssueRecord;
add defensive checks before casting and calling register_pull_request: first
verify iid in self._records (if missing either log a warning and return or raise
a KeyError with a clear message), then fetch the record and validate
isinstance(record, IssueRecord) (if not, log or raise a TypeError indicating the
unexpected record type), and only then iterate prs and call
record.register_pull_request(pr); keep behavior consistent with surrounding
error-handling/logging conventions.
There was a problem hiding this comment.
@miroslavpojer, great! Thank you for adding the defensive checks to ensure the method handles edge cases safely. This will prevent potential runtime errors when dealing with cross-repo PRs.
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.979Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
release_notes_generator/data/miner.py (1)
367-376: Narrow the exception handling scope.Line [375] currently catches bare
Exception, which masks programming errors and Ruff flags it (BLE001). Please guard the attribute access up front and only swallow the expected API/attribute failures so unexpected bugs still surface.Apply this diff:
try: - for ev in i.get_timeline(): # timeline includes cross-references - if ev.event == "cross-referenced" and getattr(ev, "source", None): - # <- this is a github.Issue.Issue - src_issue = ev.source.issue # type: ignore[union-attr] - if getattr(src_issue, "pull_request", None): - pr = src_issue.as_pull_request() # github.PullRequest.PullRequest - prs_of_cross_repo_issues[iid].append(pr) - except Exception as e: - logger.warning("Failed to fetch timeline events for issue %s: %s", iid, str(e)) + for ev in i.get_timeline(): # timeline includes cross-references + if ev.event != "cross-referenced": + continue + source_issue = getattr(getattr(ev, "source", None), "issue", None) + if source_issue is None: + continue + if getattr(source_issue, "pull_request", None): + pr = source_issue.as_pull_request() # github.PullRequest.PullRequest + prs_of_cross_repo_issues[iid].append(pr) + except (GithubException, AttributeError, TypeError) as exc: + logger.warning("Failed to fetch timeline events for issue %s: %s", iid, exc)Add alongside the other imports:
from github.GithubException import GithubExceptionBased on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/data/miner.py(5 hunks)release_notes_generator/generator.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T14:13:38.979Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.979Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
Applied to files:
release_notes_generator/record/factory/default_record_factory.py
🧬 Code graph analysis (3)
release_notes_generator/generator.py (1)
release_notes_generator/data/miner.py (1)
mine_missing_sub_issues(87-105)
release_notes_generator/record/factory/default_record_factory.py (1)
release_notes_generator/model/issue_record.py (1)
register_pull_request(199-206)
release_notes_generator/data/miner.py (2)
release_notes_generator/model/mined_data.py (1)
MinedData(38-81)release_notes_generator/utils/record_utils.py (1)
get_id(24-45)
🪛 Ruff (0.13.3)
release_notes_generator/data/miner.py
375-375: Do not catch blind exception: Exception
(BLE001)
miroslavpojer
deleted the
feature/174-Add-logic-for-fetching-cross-repo-sub-issues's-PRs
branch
1 participant
Release Notes:
Summary by CodeRabbit
Fixes #174