#167 - Introduce regime for hierarchical release note #168
Conversation
- Introduced new regime `issue-hierarchy`
WalkthroughReplaces the prior "regime" concept with a boolean Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH_Action as GitHub Action
participant Gen as Generator
participant AI as ActionInputs
participant Min as Miner
participant Fil as FilterByRelease
participant Fact as RecordFactory
participant Chap as CustomChapters
participant Bld as ReleaseNotesBuilder
GH_Action->>Gen: generate()
Gen->>AI: get_hierarchy()
AI-->>Gen: bool
Gen->>Min: mine data (issues, PRs, commits)
Min-->>Gen: MinedData (with since)
Gen->>Chap: set since(data.since)
Gen->>Fil: filter(data)
Fil-->>Gen: filtered data
Gen->>Fact: _get_record_factory(hierarchy) → instantiate appropriate factory
Fact->>Fact: generate(data)
Fact-->>Gen: records
Gen->>Bld: ReleaseNotesBuilder(records, changelog_url, chapters).build()
Bld-->>GH_Action: release notes string
sequenceDiagram
autonumber
participant IH as IssueHierarchyRecordFactory
participant PR as PullRequest
participant API as GitHub API
participant Rec as Records
loop Process issues
IH->>IH: classify as HierarchyIssueRecord or SubIssueRecord
end
loop Process PRs
IH->>API: fetch linked issues (body + GraphQL)
API-->>IH: set(issue_numbers)
IH->>IH: associate PR and commits to issues
end
IH->>IH: rebind sub-issues to parents
IH->>IH: order_hierarchy_levels()
IH-->>Rec: hierarchical records map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (8)
🔇 Additional comments (12)
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 |
…_one_custom_label" is funvtional. Release Notes builder provides expected output for mocked data.
- Reduced inputs. Login refactored to be more generic. - One hierarchy unit test is working. - Known problem with multiple calls of labels API during rls notes generation. It is unexpected.
- Applied black.
… and issue types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
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/utils/pull_request_utils.py (1)
31-33: Remove lru_cache on function taking PullRequest (unhashable).Using lru_cache with a PullRequest instance key will raise TypeError at runtime. Cache by PR number or drop caching.
Apply:
-@lru_cache(maxsize=None) -def extract_issue_numbers_from_body(pr: PullRequest) -> set[int]: +def extract_issue_numbers_from_body(pr: PullRequest) -> set[int]:release_notes_generator/builder/builder.py (1)
67-73: Wrong attribute: populated_record_numbers vs populated_record_numbers_list.BaseChapters exposes populated_record_numbers_list. Current code likely AttributeError at runtime.
- used_record_numbers=self.custom_chapters.populated_record_numbers, + used_record_numbers=self.custom_chapters.populated_record_numbers_list,tests/conftest.py (1)
176-189: Spec’d Mock for Issue blocks non-existent attributes (get_sub_issues, type)mocker.Mock(spec=Issue) will raise when assigning get_sub_issues or .type, which are not on PyGithub’s Issue. This can break fixture creation.
Two options:
- Drop spec where you need extra attrs.
- Or introduce a stub with required API and spec against it.
Minimal fix here:
- issue = mocker.Mock(spec=Issue) + issue = mocker.Mock() # do not spec to allow test-only attrs like get_sub_issues/typeRepeat for fixtures that set get_sub_issues or .type.
🧹 Nitpick comments (61)
release_notes_generator/model/commit_record.py (1)
68-71: Silence Ruff ARG002: unused parameterRename the parameter to underscore to indicate it’s intentionally unused.
- def get_rls_notes(self, line_marks: Optional[list[str]] = None) -> str: + def get_rls_notes(self, _line_marks: Optional[list[str]] = None) -> str: # Hint: direct commits does not support release notes return ""release_notes_generator/chapters/base_chapters.py (1)
91-92: Tiny style tweak: truthiness over len(...)Prefer a direct truthiness check.
- if len(chapter_string) > 0: + if chapter_string: result += chapter_string + "\n\n"tests/release_notes/chapters/test_base_chapters.py (1)
17-17: Import path update — OK. Consider adding a test for since.Nice move to the new module path. Suggest adding a focused test for the since property default and setter.
from datetime import datetime def test_since_default_and_setter(): chapters = Chapters() assert chapters.since == datetime.min now = datetime(2024, 1, 1, 0, 0, 0) chapters.since = now assert chapters.since == nowrelease_notes_generator/record/factory/record_factory.py (2)
17-19: Fix module docstring to name the correct classDocstring mentions DefaultRecordFactory, but this module defines RecordFactory.
-""" -This module contains the DefaultRecordFactory class which is responsible for generating -""" +""" +This module contains the RecordFactory base class used to generate records. +"""
20-27: Unused loggerlogger is defined but not used. Either remove it or log within this module (e.g., during abstract method calls).
release_notes_generator/model/record.py (2)
36-41: Avoid re-computing labels when empty; use a None sentinelWith the current caching, records that truly have no labels (e.g., CommitRecord) will call get_labels() on every access. Use Optional[list[str]] and a None sentinel.
- self._labels = labels if labels is not None else [] + self._labels: Optional[list[str]] = labels @@ - if len(self._labels) == 0: - self._labels = self.get_labels() - - return self._labels + if self._labels is None: + self._labels = self.get_labels() + return self._labelsAlso applies to: 65-69
120-126: Docstring return type mismatchThe docstring says set[str] but the API uses list[str].
- Returns: - set[str]: A list of labels associated with the record. + Returns: + list[str]: Labels associated with the record.release_notes_generator/action_inputs.py (3)
172-180: Simplify boolean parsing; remove dead None check.
get_action_input(..., "false")never returns None. Also normalize whitespace.Apply this diff:
- val = get_action_input("hierarchy", "false") - if val is None: - return False - return val.lower() == "true" + val = get_action_input("hierarchy", "false") + return str(val).strip().lower() == "true"
308-322: Validate hierarchy row format and remove trailing space in the default.
- Default contains a trailing space.
validate_inputs()doesn’t validate the new hierarchy row format; add parity with issue/PR.Apply these diffs:
- ROW_FORMAT_HIERARCHY_ISSUE, "{type}: _{title}_ {number} " + ROW_FORMAT_HIERARCHY_ISSUE, "{type}: _{title}_ {number}"And in
validate_inputs()after PR row checks:- ActionInputs._detect_row_format_invalid_keywords(row_format_pr, row_type="PR") + ActionInputs._detect_row_format_invalid_keywords(row_format_pr, row_type=ActionInputs.ROW_TYPE_PR) + + row_format_hier_issue = ActionInputs.get_row_format_hierarchy_issue() + if not isinstance(row_format_hier_issue, str) or not row_format_hier_issue.strip(): + errors.append("Hierarchy Issue row format must be a non-empty string.") + ActionInputs._detect_row_format_invalid_keywords( + row_format_hier_issue, row_type=ActionInputs.ROW_TYPE_HIERARCHY_ISSUE + )
483-491: Add a safe default for unknown row_type.Unknown types currently yield an empty allow‑list, stripping all keys. Default to Issue keys and warn.
Apply this diff:
match row_type: case ActionInputs.ROW_TYPE_ISSUE: supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_ISSUE case ActionInputs.ROW_TYPE_PR: supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_PULL_REQUEST case ActionInputs.ROW_TYPE_HIERARCHY_ISSUE: supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_HIERARCHY_ISSUE + case _: + logger.warning( + "Unknown row_type '%s' in _detect_row_format_invalid_keywords; defaulting to Issue keys.", + row_type, + ) + supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_ISSUErelease_notes_generator/model/issue_record.py (2)
16-16: Unused logger.
loggeris defined but not used. Either remove it or add useful debug logs (e.g., when formatting rows).
233-239: Minor: simplify empty check.Use truthiness for the dict; avoids building a
.values()view.Apply this diff:
- if len(self._pull_requests.values()) == 0: + if not self._pull_requests: return []tests/release_notes/chapters/test_custom_chapters.py (1)
73-85: Test may no longer reflect populate() semantics (pulls count).
CustomChapters.populate()now derives PR count viapull_requests_count()for Issue‑like records; generic Record mocks default to 1, so this test can give false positives.Consider changing the mock to an IssueRecord spec and stub
pull_requests_count():-from release_notes_generator.chapters.custom_chapters import CustomChapters +from release_notes_generator.chapters.custom_chapters import CustomChapters +from release_notes_generator.model.issue_record import IssueRecord @@ -def test_populate_no_pulls_count(custom_chapters, mocker): - record1 = mocker.Mock(spec=Record) - record1.labels = ["bug"] - record1.pulls_count = 0 - record1.is_present_in_chapters = False +def test_populate_no_pulls_count(custom_chapters, mocker): + record1 = mocker.Mock(spec=IssueRecord) + record1.labels = ["bug"] + record1.pull_requests_count.return_value = 0 + record1.is_present_in_chapters = Falsetests/test_action_inputs.py (2)
28-28: Add negative case for hierarchy validation.We validate hierarchy is boolean; include it in failure_cases.
Apply this diff:
failure_cases = [ @@ - ("get_duplicity_icon", "Oj", "Duplicity icon must be a non-empty string and have a length of 1."), + ("get_duplicity_icon", "Oj", "Duplicity icon must be a non-empty string and have a length of 1."), + ("get_hierarchy", "not_bool", "Hierarchy must be a boolean."), ]
267-277: Add a test for hierarchy row format cleaning.Covers
{ROW_FORMAT_HIERARCHY_ISSUE}invalid keys and logging under the HierarchyIssue row type.Apply this diff:
def test_clean_row_format_invalid_keywords_nested_braces(): @@ assert expected_format == actual_format + +def test_get_row_format_hierarchy_issue_cleans_invalid_keywords(mocker, caplog): + caplog.set_level(logging.ERROR) + mocker.patch( + "release_notes_generator.action_inputs.get_action_input", + return_value="{type}: _{title}_ {number} {invalid}", + ) + fmt = ActionInputs.get_row_format_hierarchy_issue() + assert "{invalid}" not in fmt + assert any("HierarchyIssue" in rec.getMessage() for rec in caplog.records)tests/utils/test_pull_reuqest_utils.py (1)
128-129: Set-based assertions: LGTM.Matches the new
set[int]return type and preserves cache behavior checks.Nit: consider renaming the file to tests/utils/test_pull_request_utils.py to fix the “reuqest” typo.
Also applies to: 155-156, 214-214, 245-246
show_only.md (2)
2-31: Fix Markdown list style/indentation (MD004, MD007).Normalize bullets to dashes and 2-space indentation to satisfy markdownlint and render correctly.
Apply this diff:
- - - None: _HI302 open_ #302 -- _Release Notes_: - - Hierarchy level release note -- #451 _SI451 open_ in #150 - - Hierarchy level release note - - Fixed bug - - Improved performance - + More nice code - * Awesome architecture - - - None: _HI303 open_ #303 -- _Release Notes_: - - Hierarchy level release note -- #452 _SI452 open_ in #151 - - Hierarchy level release note - - Fixed bug - - Improved performance - + More nice code - * Awesome architecture - - - None: _HI304 open_ #304 -- _Release Notes_: - - Hierarchy level release note -- None: _HI350 open_ #350 - - _Release Notes_: - - Sub-hierarchy level release note - - #453 _SI453 open_ in #152 - - Hierarchy level release note - - Fixed bug - - Improved performance - + More nice code - * Awesome architecture +- None: _HI302 open_ #302 + - _Release Notes_: + - Hierarchy level release note + - #451 _SI451 open_ in #150 + - Hierarchy level release note + - Fixed bug + - Improved performance + - More nice code + - Awesome architecture +- None: _HI303 open_ #303 + - _Release Notes_: + - Hierarchy level release note + - #452 _SI452 open_ in #151 + - Hierarchy level release note + - Fixed bug + - Improved performance + - More nice code + - Awesome architecture +- None: _HI304 open_ #304 + - _Release Notes_: + - Hierarchy level release note +- None: _HI350 open_ #350 + - _Release Notes_: + - Sub-hierarchy level release note + - #453 _SI453 open_ in #152 + - Hierarchy level release note + - Fixed bug + - Improved performance + - More nice code + - Awesome architecture
1-32: Consider relocating this generated sample under docs/examples.Keeps root clean and clarifies that this is illustrative output.
release_notes_generator/chapters/service_chapters.py (2)
147-149: Docstring param name mismatch.Update
@param nrto@param record_id.Apply this diff:
- @param nr: The number of the record. + @param record_id: The number of the record.
59-60: Speed up membership checks with a set.
used_record_numbersis used for frequent membership tests; consider a companionsetto make these O(1) while preserving the list for ordering if needed.Example:
self._used_record_numbers_set: set[int | str] = set(self.used_record_numbers) # keep both in sync on appendtests/release_notes/record/factory/test_default_record_factory.py (1)
173-175: Tests updated to new factory path and constructor look good.Patches and expectations match the refactor to
DefaultRecordFactory(mock_github_client).You can DRY the repeated
safe_call_decoratorpatches via a fixture returning the desired wrapper.Also applies to: 210-213, 280-282, 315-317, 344-346, 376-378, 426-427
release_notes_generator/generator.py (1)
106-119: Factory selection is correct; tweak docstring for accuracy.Replace “RecordFactory” with “DefaultRecordFactory” in the return description.
Apply this diff:
- Returns: - RecordFactory: An instance of either IssueHierarchyRecordFactory or RecordFactory. + Returns: + RecordFactory: An instance of either IssueHierarchyRecordFactory or DefaultRecordFactory.README.md (1)
51-51: Mention{type}placeholder for hierarchy rows.Description currently lists only
numberandtitle, but default uses{type}too.Apply this diff:
-| `row-format-hierarchy-issue` | The format of the row for the hierarchy issue in the release notes. The format can contain placeholders for the issue `number` and `title`. The placeholders are case-sensitive. | No | `"{type}: _{title}_ {number}"` | +| `row-format-hierarchy-issue` | The format of the row for the hierarchy issue in the release notes. Placeholders: `type`, `number`, `title` (case-sensitive). | No | `"{type}: _{title}_ {number}"` |release_notes_generator/utils/pull_request_utils.py (5)
36-38: Docstring return type is wrong.Says “list” but function returns set[int].
- @return: The numbers of the issues mentioned in the body of the pull request as a list of integers. + @return: The numbers of the issues mentioned in the body of the pull request as a set of integers.
48-50: Drop hardcoded debug print (leaks to stdout).Use logging if needed; magic PR 3645 shouldn’t be special‑cased.
- if pr.number == 3645: - print(f"PR #{pr.number} - Extracted issue numbers from body: {issue_numbers}")
56-56: Nit: fix typo in docstring.- """Update the placeholder values and formate the graphQL query""" + """Update the placeholder values and format the GraphQL query"""
76-78: Drop leftover API debug print.- if pull_number == 3645: - print(f"PR #{pull_number} - Extracted issue numbers from GitHub API: {numbers}")
55-55: Optional: bound cache size for API calls.Unbounded cache can grow over time. 1024 is plenty here.
-@lru_cache(maxsize=None) +@lru_cache(maxsize=1024) def get_issues_for_pr(pull_number: int) -> set[int]:tests/release_notes/model/test_commit_record.py (4)
1-5: Remove unused imports; avoid fixture shadowing.These trigger Ruff F811 and are unnecessary.
-import types - -from tests.conftest import mock_commit -
56-56: Silence ARG005 by not naming unused arg.- lambda self: True, + lambda _: True,
59-59: Silence ARG005 for unused parameter in lambda.- monkeypatch.setattr(rec, "get_rls_notes", lambda line_marks=None: "Extra release notes.") + monkeypatch.setattr(rec, "get_rls_notes", lambda _line_marks=None: "Extra release notes.")
7-11: Drop dead test helper.DummyInnerCommit is unused.
-class DummyInnerCommit: - def __init__(self, sha: str, message: str): - self.sha = sha - self.message = message -tests/release_notes/model/test_record.py (2)
49-51: Use the parameter to satisfy linters.- def to_chapter_row(self, add_into_chapters: bool = True): - return f"Row for {self._record_id}" + def to_chapter_row(self, add_into_chapters: bool = True): + _ = add_into_chapters + return f"Row for {self._record_id}"
52-56: Mark unused argument as used.- def get_rls_notes(self, line_marks=None): - return self._rls_notes + def get_rls_notes(self, line_marks=None): + _ = line_marks + return self._rls_notesrelease_notes_generator/filter.py (2)
21-24: Remove unused Optional import.-from typing import Optional - -from release_notes_generator.action_inputs import ActionInputs +from release_notes_generator.action_inputs import ActionInputs
140-149: Docstring doesn’t match implementation.The code doesn’t “always include certain types”; it includes open or recently closed issues.
- """ - Hierarchy filtering for issues: filter out closed issues before the release date, - but always include issues of certain types (e.g., "Epic", "Story"). - """ + """ + Hierarchy filtering for issues: include issues closed since the release date + or still open at generation time. + """release_notes_generator/model/hierarchy_issue_record.py (2)
1-3: Fix module docstring (wrong class name).-""" -A module that defines the IssueRecord class, which represents an issue record in the release notes. -""" +""" +A module that defines the HierarchyIssueRecord class for hierarchical issue rendering. +"""
26-27: Forward reference in annotation may break on older Python.Use a quoted name to avoid evaluating before class definition.
- self._sub_hierarchy_issues: dict[int, HierarchyIssueRecord] = {} + self._sub_hierarchy_issues: dict[int, "HierarchyIssueRecord"] = {}release_notes_generator/model/pull_request_record.py (2)
77-104: Remove unreachable contributor code (always returns []).contributors currently returns [] and the rest is dead code. Either implement or remove the unreachable block to avoid confusion.
Apply:
- 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)) + return [] + # TODO: Implement contributors once the source of truth (PR vs commits) is finalized.
110-122: Format authors/contributors as strings, not lists.If custom row formats ever include {authors}/{contributors}, the current list objects will render poorly.
Apply:
- 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 ""tests/release_notes/builder/test_release_notes_builder.py (3)
24-25: Deduplicate MockLabel (reuse the one from conftest).Avoid drifting duplicates by importing MockLabel from conftest.
Apply:
-from tests.conftest import mock_safe_call_decorator +from tests.conftest import mock_safe_call_decorator, MockLabel @@ -# pylint: disable=too-few-public-methods -class MockLabel: - def __init__(self, name): - self.name = name +Also applies to: 77-81
1300-1302: Fix debug prints (current output shows tuples).Use f-strings to print actual strings.
Apply:
- print("Actual:\n%s", actual_release_notes) - print("Expected:\n%s", expected_release_notes) + print(f"Actual:\n{actual_release_notes}") + print(f"Expected:\n{expected_release_notes}")
1835-1839: Tighten temporary debug prints.Minor clean-up for readability.
Apply:
- print("XXX") - print(actual_release_notes) - print("XXX") + print(f"XXX\n{actual_release_notes}\nXXX")action.yml (5)
17-18: Typo: “scrapper” → “scraper”.Apply:
-name: 'Release notes scrapper' +name: 'Release notes scraper'
79-83: Include {type} in placeholder list.Default uses {type} but description omits it.
Apply:
- row-format-hierarchy-issue: - description: 'Format of the hierarchy issue in the release notes. Available placeholders: {number}, {title}. Placeholders are case-insensitive.' + row-format-hierarchy-issue: + description: 'Format of the hierarchy issue in the release notes. Available placeholders: {number}, {title}, {type}. Placeholders are case-insensitive.'
88-91: PR row placeholders: remove {pull-requests}.PR rows don’t render that placeholder.
Apply:
- row-format-pr: - description: 'Format of the pr row in the release notes. Available placeholders: {number}, {title}, {pull-requests}. Placeholders are case-insensitive.' + row-format-pr: + description: 'Format of the pr row in the release notes. Available placeholders: {number}, {title}. Placeholders are case-insensitive.'
131-136: PYTHONPATH export doesn’t persist across steps.Write to GITHUB_ENV so the next step inherits it, or drop the step if unnecessary.
Apply:
- name: Set PROJECT_ROOT and update PYTHONPATH run: | ACTION_ROOT="${{ github.action_path }}" - export PYTHONPATH="${PYTHONPATH}:${ACTION_ROOT}/release_notes_generator" + echo "PYTHONPATH=${PYTHONPATH}:${ACTION_ROOT}/release_notes_generator" >> "$GITHUB_ENV" shell: bash
35-39: Back-compat with legacy “regime” input.Switching to hierarchy is good, but existing users passing regime may break. Recommend accepting regime as deprecated and mapping it in ActionInputs.get_hierarchy().
Proposed change outside this file (release_notes_generator/action_inputs.py):
def get_hierarchy() -> bool: val = get_action_input("hierarchy", None) if val is None: # Back-compat: regime='issue-hierarchy' implies hierarchy regime = get_action_input("regime", "") return regime.strip().lower() == "issue-hierarchy" return val.strip().lower() == "true"release_notes_generator/record/factory/default_record_factory.py (3)
17-19: Incomplete module docstring.Finish the sentence for clarity.
Apply:
-""" -This module contains the DefaultRecordFactory class which is responsible for generating -""" +""" +DefaultRecordFactory builds Record objects (issues, pulls, commits) from mined GitHub data. +"""
153-171: Stale docstring params.Method no longer accepts ‘records’; update doc.
Apply:
- Parameters: - records (dict[int|str, Record]): The dictionary of records to add the issue record to. - issue (Issue): The issue to create a record for. - issue_labels (Optional[list[str]]): Optional set of labels for the issue. If not provided, labels will be - fetched from the issue. - Returns: - None + Parameters: + issue (Issue): The issue to create a record for. + issue_labels (Optional[list[str]]): Labels for the issue; fetched from GitHub if not provided. + Returns: None
65-97: Unused parameter skip_rec.skip_rec is never used in register_pull_request; remove it or apply it when creating standalone PR records in fallback paths.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
17-19: Module docstring references the wrong class.Update to reflect IssueHierarchyRecordFactory.
Apply:
-""" -This module contains the DefaultRecordFactory class which is responsible for generating -""" +""" +IssueHierarchyRecordFactory builds hierarchical issue records (Epics/Features/Tasks) and associates PRs/commits. +"""
241-243: Recursive rebind: safe but consider an iterative loop.Current recursion is fine; if the chain is very deep, an iterative while loop over self.__sub_issue_parents would avoid recursion depth concerns.
tests/conftest.py (8)
48-54: Safe-call test shim looks goodWrapper selectively swaps get_issues_for_pr; simple and readable for tests. Consider functools.wraps if you later need metadata preserved.
55-59: Silence ARG001 and allow keyword-only invocationRename the param to underscore to satisfy Ruff while keeping signature compatible with pull_number=... calls.
Apply:
-def mock_get_issues_for_pr(pull_number: int) -> set[int]: +def mock_get_issues_for_pr(_pull_number: int) -> set[int]: # if pull_number == 150: # return [451] return set()
226-241: Literal “/n” instead of newline “\n” in bodyUse “\n” so release-notes parsing isn’t tripped by a stray “/n”.
- issue.body = "I400 open/nRelease Notes:\n- Hierarchy level release note" + issue.body = "I400 open\nRelease Notes:\n- Hierarchy level release note"Apply across similar fixtures.
243-258: Same “/n” newline typo in sub-issue bodyFix as above to ensure parsers see the header on a new line.
- issue.body = "I450 closed/nRelease Notes:\n- Hierarchy level release note" + issue.body = "I450 closed\nRelease Notes:\n- Hierarchy level release note"
260-275: Same “/n” typo in hierarchy issue bodyReplace “/n” with “\n”.
- issue.body = "I300 open/nRelease Notes:\n- Hierarchy level release note" + issue.body = "I300 open\nRelease Notes:\n- Hierarchy level release note"
710-755: Comment vs. data mismatch; label case choices
- Comment says “enhancement to one of the PRs” but only bug is assigned. Either add enhancement to a PR or update the comment.
- Label names are lower-case (“epic”, “feature”), which is fine; just ensure tests don’t rely on case-sensitive comparisons.
Suggested tweak to match the comment:
@@ - data.pull_requests[0].get_labels.return_value = [l_bug] + data.pull_requests[0].get_labels.return_value = [l_bug] + data.pull_requests[1].get_labels.return_value = [l_enh]
820-860: Inconsistent IssueType casing (“feature” vs “Feature”)If code normalizes type names, fine; otherwise prefer consistent capitalization to avoid brittle assertions.
- t_feature = mocker.Mock(spec=IssueType) - t_feature.name = "feature" + t_feature = mocker.Mock(spec=IssueType) + t_feature.name = "Feature"
907-937: Avoid mutating protected _labels; use ctor’s issue_labelsHierarchyIssueRecord accepts issue_labels; prefer that over touching _labels.
- rec_epic_issue = HierarchyIssueRecord(issue=request.getfixturevalue("mock_open_hierarchy_issue_epic")) # nr:200 - rec_epic_issue._labels = ["epic"] # override labels to have epic label + rec_epic_issue = HierarchyIssueRecord( + issue=request.getfixturevalue("mock_open_hierarchy_issue_epic"), + issue_labels=["epic"] + ) # nr:200Also, nit: the TODO in Line 902 is stale; consider removing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.pylintrc(2 hunks)README.md(2 hunks)action.yml(4 hunks)main.py(2 hunks)release_notes_generator/action_inputs.py(8 hunks)release_notes_generator/builder/base_builder.py(0 hunks)release_notes_generator/builder/builder.py(1 hunks)release_notes_generator/chapters/base_chapters.py(4 hunks)release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/chapters/service_chapters.py(5 hunks)release_notes_generator/filter.py(4 hunks)release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/commit_record.py(1 hunks)release_notes_generator/model/hierarchy_issue_record.py(1 hunks)release_notes_generator/model/issue_record.py(5 hunks)release_notes_generator/model/pull_request_record.py(2 hunks)release_notes_generator/model/record.py(3 hunks)release_notes_generator/model/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(6 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)release_notes_generator/record/factory/record_factory.py(1 hunks)release_notes_generator/utils/constants.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(3 hunks)show_only.md(1 hunks)tests/conftest.py(9 hunks)tests/release_notes/builder/test_release_notes_builder.py(1 hunks)tests/release_notes/chapters/test_base_chapters.py(1 hunks)tests/release_notes/chapters/test_custom_chapters.py(1 hunks)tests/release_notes/model/test_commit_record.py(1 hunks)tests/release_notes/model/test_record.py(2 hunks)tests/release_notes/record/factory/test_default_record_factory.py(12 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(1 hunks)tests/release_notes/test_release_notes_builder.py(0 hunks)tests/test_action_inputs.py(1 hunks)tests/test_filter.py(1 hunks)tests/test_release_notes_generator.py(1 hunks)tests/utils/test_pull_reuqest_utils.py(4 hunks)
💤 Files with no reviewable changes (2)
- release_notes_generator/builder/base_builder.py
- tests/release_notes/test_release_notes_builder.py
🧰 Additional context used
🧬 Code graph analysis (28)
tests/release_notes/chapters/test_custom_chapters.py (1)
release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)
release_notes_generator/utils/pull_request_utils.py (1)
tests/utils/test_pull_reuqest_utils.py (6)
json(104-115)json(143-152)json(164-165)json(178-180)json(194-203)json(224-233)
tests/release_notes/record/factory/test_default_record_factory.py (2)
release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(44-170)generate(56-125)tests/conftest.py (1)
mock_safe_call_decorator(48-53)
release_notes_generator/chapters/service_chapters.py (3)
release_notes_generator/chapters/base_chapters.py (2)
BaseChapters(28-112)add_row(67-78)release_notes_generator/model/issue_record.py (2)
record_id(46-47)to_chapter_row(102-125)release_notes_generator/model/hierarchy_issue_record.py (1)
to_chapter_row(87-140)
tests/utils/test_pull_reuqest_utils.py (1)
release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(55-79)
release_notes_generator/model/pull_request_record.py (3)
release_notes_generator/model/record.py (4)
labels(59-68)get_labels(121-126)to_chapter_row(109-118)added_into_chapters(138-144)release_notes_generator/model/hierarchy_issue_record.py (2)
get_labels(71-84)to_chapter_row(87-140)release_notes_generator/model/issue_record.py (2)
get_labels(84-85)to_chapter_row(102-125)
release_notes_generator/model/sub_issue_record.py (2)
release_notes_generator/model/issue_record.py (2)
IssueRecord(19-330)issue(66-71)release_notes_generator/model/record.py (1)
skip(54-56)
release_notes_generator/record/factory/record_factory.py (5)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/record.py (1)
Record(29-196)release_notes_generator/record/factory/default_record_factory.py (1)
generate(56-125)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(55-103)release_notes_generator/generator.py (1)
generate(70-104)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (9)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)commit(45-50)get_labels(72-73)release_notes_generator/model/hierarchy_issue_record.py (7)
HierarchyIssueRecord(16-154)order_hierarchy_levels(142-154)get_labels(71-84)sub_issues(47-51)sub_hierarchy_issues(54-58)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (6)
IssueRecord(19-330)issue(66-71)get_labels(84-85)register_pull_request(194-201)register_commit(203-217)issue_type(74-80)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(64-504)get_skip_release_notes_labels(211-219)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(15-289)get_labels(107-108)register_commit(167-174)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(17-41)issue(34-39)release_notes_generator/record/factory/default_record_factory.py (4)
DefaultRecordFactory(44-170)generate(56-125)register_pull_request(65-95)_create_record_for_issue(153-170)release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(55-79)extract_issue_numbers_from_body(32-51)
release_notes_generator/miner.py (2)
release_notes_generator/model/issue_record.py (2)
IssueRecord(19-330)issue(66-71)release_notes_generator/action_inputs.py (1)
get_published_at(203-208)
release_notes_generator/model/commit_record.py (5)
release_notes_generator/model/hierarchy_issue_record.py (2)
to_chapter_row(87-140)get_labels(71-84)release_notes_generator/model/issue_record.py (3)
to_chapter_row(102-125)get_rls_notes(127-155)get_labels(84-85)release_notes_generator/model/pull_request_record.py (3)
to_chapter_row(110-128)get_rls_notes(130-151)get_labels(107-108)release_notes_generator/model/record.py (6)
to_chapter_row(109-118)added_into_chapters(138-144)present_in_chapters(146-152)contains_release_notes(183-196)get_rls_notes(129-135)get_labels(121-126)release_notes_generator/action_inputs.py (2)
ActionInputs(64-504)get_duplicity_icon(196-200)
main.py (3)
tests/conftest.py (1)
custom_chapters(63-69)release_notes_generator/generator.py (1)
custom_chapters(61-63)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)
release_notes_generator/model/issue_record.py (6)
release_notes_generator/model/sub_issue_record.py (1)
issue(34-39)release_notes_generator/model/record.py (4)
skip(54-56)get_labels(121-126)to_chapter_row(109-118)added_into_chapters(138-144)release_notes_generator/model/hierarchy_issue_record.py (2)
get_labels(71-84)to_chapter_row(87-140)release_notes_generator/model/pull_request_record.py (2)
get_labels(107-108)to_chapter_row(110-128)release_notes_generator/model/commit_record.py (2)
get_labels(72-73)to_chapter_row(54-66)tests/release_notes/model/test_record.py (2)
get_labels(55-56)to_chapter_row(49-50)
tests/release_notes/model/test_commit_record.py (2)
tests/conftest.py (1)
mock_commit(573-578)release_notes_generator/model/commit_record.py (6)
CommitRecord(13-75)record_id(27-28)is_closed(31-32)is_open(35-36)authors(39-40)to_chapter_row(54-66)
release_notes_generator/generator.py (7)
release_notes_generator/builder/builder.py (2)
ReleaseNotesBuilder(32-86)build(52-86)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(44-170)generate(56-125)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-256)generate(55-103)release_notes_generator/record/factory/record_factory.py (2)
RecordFactory(29-45)generate(35-42)release_notes_generator/chapters/base_chapters.py (2)
since(52-61)since(64-65)release_notes_generator/action_inputs.py (2)
ActionInputs(64-504)get_hierarchy(172-179)
release_notes_generator/builder/builder.py (5)
release_notes_generator/action_inputs.py (3)
ActionInputs(64-504)get_warnings(276-281)get_print_empty_chapters(284-289)tests/conftest.py (2)
custom_chapters(63-69)service_chapters(86-87)release_notes_generator/generator.py (1)
custom_chapters(61-63)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/chapters/service_chapters.py (1)
ServiceChapters(43-270)
tests/release_notes/model/test_record.py (1)
release_notes_generator/model/record.py (4)
skip(54-56)labels(59-68)to_chapter_row(109-118)get_labels(121-126)
tests/release_notes/builder/test_release_notes_builder.py (5)
release_notes_generator/builder/builder.py (2)
ReleaseNotesBuilder(32-86)build(52-86)tests/conftest.py (19)
custom_chapters(63-69)mock_safe_call_decorator(48-53)MockLabel(44-46)custom_chapters_not_print_empty_chapters(73-81)record_with_issue_closed_two_pulls(901-905)record_with_issue_closed_no_pull(868-869)pull_request_record_merged(977-980)pull_request_record_closed(1002-1005)record_with_issue_open_two_pulls_closed(947-951)record_with_issue_open_no_pull(864-865)pull_request_record_open(984-987)record_with_issue_closed_one_pull(880-883)record_with_issue_closed_one_pull_merged(887-890)record_with_issue_closed_one_pull_merged_skip(894-897)pull_request_record_closed_with_skip_label(1009-1015)mined_data_isolated_record_types_no_labels_no_type_defined(582-707)mined_data_isolated_record_types_with_labels_no_type_defined(711-754)mined_data_isolated_record_types_no_labels_with_type_defined(758-817)mined_data_isolated_record_types_with_labels_with_type_defined(821-859)release_notes_generator/chapters/custom_chapters.py (2)
CustomChapters(37-90)from_yaml_array(75-90)release_notes_generator/record/factory/default_record_factory.py (1)
DefaultRecordFactory(44-170)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-256)generate(55-103)
release_notes_generator/model/record.py (5)
tests/release_notes/model/test_record.py (3)
labels(42-43)get_labels(55-56)to_chapter_row(49-50)release_notes_generator/model/hierarchy_issue_record.py (2)
get_labels(71-84)to_chapter_row(87-140)release_notes_generator/model/issue_record.py (2)
get_labels(84-85)to_chapter_row(102-125)release_notes_generator/model/pull_request_record.py (2)
get_labels(107-108)to_chapter_row(110-128)release_notes_generator/model/commit_record.py (2)
get_labels(72-73)to_chapter_row(54-66)
release_notes_generator/model/hierarchy_issue_record.py (3)
release_notes_generator/action_inputs.py (3)
ActionInputs(64-504)get_duplicity_icon(196-200)get_row_format_hierarchy_issue(309-321)release_notes_generator/model/issue_record.py (5)
issue(66-71)pull_requests_count(219-225)get_labels(84-85)get_pr_links(227-239)get_rls_notes(127-155)release_notes_generator/model/record.py (4)
get_labels(121-126)labels(59-68)contains_release_notes(183-196)get_rls_notes(129-135)
release_notes_generator/filter.py (2)
release_notes_generator/action_inputs.py (1)
get_hierarchy(172-179)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
tests/conftest.py (8)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)get_labels(72-73)commit(45-50)release_notes_generator/model/hierarchy_issue_record.py (6)
HierarchyIssueRecord(16-154)get_labels(71-84)level(30-34)level(37-44)sub_hierarchy_issues(54-58)sub_issues(47-51)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/chapters/service_chapters.py (1)
ServiceChapters(43-270)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(17-41)issue(34-39)tests/release_notes/record/factory/test_default_record_factory.py (5)
mock_safe_call_decorator(159-164)wrapper(160-163)wrapper(334-337)wrapper(439-442)mock_get_issues_for_pr(166-171)release_notes_generator/record/factory/default_record_factory.py (1)
register_pull_request(65-95)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (6)
release_notes_generator/model/commit_record.py (1)
CommitRecord(13-75)release_notes_generator/model/hierarchy_issue_record.py (6)
HierarchyIssueRecord(16-154)pull_requests_count(60-69)sub_hierarchy_issues(54-58)sub_issues(47-51)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (1)
IssueRecord(19-330)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-256)generate(55-103)tests/conftest.py (5)
mock_safe_call_decorator(48-53)mined_data_isolated_record_types_no_labels_no_type_defined(582-707)mined_data_isolated_record_types_with_labels_no_type_defined(711-754)mined_data_isolated_record_types_no_labels_with_type_defined(758-817)mined_data_isolated_record_types_with_labels_with_type_defined(821-859)
release_notes_generator/action_inputs.py (1)
release_notes_generator/utils/gh_action.py (1)
get_action_input(26-38)
tests/release_notes/chapters/test_base_chapters.py (1)
release_notes_generator/chapters/base_chapters.py (1)
BaseChapters(28-112)
tests/test_release_notes_generator.py (3)
tests/conftest.py (1)
custom_chapters(63-69)release_notes_generator/generator.py (1)
custom_chapters(61-63)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)
release_notes_generator/record/factory/default_record_factory.py (6)
release_notes_generator/record/factory/record_factory.py (2)
RecordFactory(29-45)generate(35-42)release_notes_generator/generator.py (2)
rate_limiter(66-68)generate(70-104)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(30-76)release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(55-103)_create_record_for_issue(196-201)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/chapters/custom_chapters.py (6)
release_notes_generator/action_inputs.py (1)
ActionInputs(64-504)release_notes_generator/chapters/base_chapters.py (2)
BaseChapters(28-112)add_row(67-78)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-154)pull_requests_count(60-69)to_chapter_row(87-140)release_notes_generator/model/issue_record.py (4)
IssueRecord(19-330)record_id(46-47)pull_requests_count(219-225)to_chapter_row(102-125)release_notes_generator/model/record.py (5)
Record(29-196)record_id(72-77)labels(59-68)is_present_in_chapters(45-51)to_chapter_row(109-118)release_notes_generator/model/sub_issue_record.py (1)
SubIssueRecord(17-41)
🪛 markdownlint-cli2 (0.18.1)
show_only.md
2-2: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
4-4: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
9-9: Unordered list style
Expected: dash; Actual: plus
(MD004, ul-style)
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
10-10: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list style
Expected: dash; Actual: plus
(MD004, ul-style)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
19-19: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
30-30: Unordered list style
Expected: dash; Actual: plus
(MD004, ul-style)
30-30: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
31-31: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
31-31: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
🪛 Ruff (0.13.1)
release_notes_generator/model/commit_record.py
68-68: Unused method argument: line_marks
(ARG002)
tests/release_notes/model/test_commit_record.py
12-12: Redefinition of unused mock_commit from line 4
(F811)
19-19: Redefinition of unused mock_commit from line 4
(F811)
23-23: Redefinition of unused mock_commit from line 4
(F811)
28-28: Redefinition of unused mock_commit from line 4
(F811)
39-39: Redefinition of unused mock_commit from line 4
(F811)
51-51: Redefinition of unused mock_commit from line 4
(F811)
56-56: Unused lambda argument: self
(ARG005)
59-59: Unused lambda argument: line_marks
(ARG005)
63-63: Redefinition of unused mock_commit from line 4
(F811)
tests/release_notes/model/test_record.py
49-49: Unused method argument: add_into_chapters
(ARG002)
52-52: Unused method argument: line_marks
(ARG002)
tests/conftest.py
55-55: Unused function argument: pull_number
(ARG001)
🪛 GitHub Actions: Check PR Release Notes in Description
main.py
[error] 1-1: Release notes presence check failed. Release notes title '(?:[Rr]elease [Nn]otes:|Summary by CodeRabbit)' not found in pull request body. Command: python /home/runner/work/_actions/AbsaOSS/release-notes-presence-check/v0.3.0/main.py
release_notes_generator/miner.py
Outdated
| # Derive 'since' from release | ||
| data.since = data.release.created_at | ||
| if ActionInputs.get_published_at(): | ||
| data.since = data.release.published_at | ||
|
|
There was a problem hiding this comment.
❓ Verification inconclusive
Handle None published_at to avoid passing None to since.
If published-at is true and the latest release has published_at is None (draft/unpublished), since=None may break filtering.
Apply this diff:
- data.since = data.release.created_at
- if ActionInputs.get_published_at():
- data.since = data.release.published_at
+ # Prefer published_at when requested and available, otherwise fallback to created_at
+ prefer_published = ActionInputs.get_published_at()
+ data.since = (
+ data.release.published_at
+ if prefer_published and getattr(data.release, "published_at", None)
+ else data.release.created_at
+ )Also log which timestamp was used for traceability.
Use published_at only when present — avoid setting data.since to None
File: release_notes_generator/miner.py (lines 135-139)
- data.since = data.release.created_at
- if ActionInputs.get_published_at():
- data.since = data.release.published_at
+ # Prefer published_at when requested and available, otherwise fallback to created_at
+ prefer_published = ActionInputs.get_published_at()
+ data.since = (
+ data.release.published_at
+ if prefer_published and getattr(data.release, "published_at", None)
+ else data.release.created_at
+ )Also log which timestamp was used for traceability.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In release_notes_generator/miner.py around lines 135 to 139, don't blindly
assign data.release.published_at because it may be None; instead set data.since
= data.release.published_at only if published_at is present/truthy, otherwise
keep data.since = data.release.created_at, and add a log statement indicating
which timestamp (published_at or created_at) was used for traceability; ensure
the log includes the actual timestamp value and use the module's logger.
There was a problem hiding this comment.
@miroslavpojer Great! Thanks for implementing the fix to handle the potential None published_at scenario. This ensures data.since always has a valid timestamp value for the filtering logic.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| def __init__(self, issue: Issue, issue_labels: Optional[list[str]] = None, skip: bool = False): | ||
| super().__init__(skip=skip) | ||
|
|
||
| self._issue: Issue = issue | ||
| self._issue_type: Optional[str] = None | ||
| self._labels = issue_labels if issue_labels is not None else [] | ||
|
|
There was a problem hiding this comment.
issue_labels parameter is ignored; use as a fallback.
You accept issue_labels but get_labels() always reads GitHub labels. Prefer provided labels when present (helps factories/tests and offline scenarios).
Apply this diff:
def get_labels(self) -> list[str]:
- return [label.name for label in self._issue.get_labels()]
+ # Prefer explicit labels if provided; otherwise read from GitHub.
+ if self._labels:
+ return self._labels
+ return [label.name for label in self._issue.get_labels()]Also applies to: 84-86
🤖 Prompt for AI Agents
In release_notes_generator/model/issue_record.py around lines 29-34 (and also
adjust lines 84-86), the constructor stores issue_labels but the class currently
always reads labels from the GitHub Issue; change logic so provided issue_labels
are used when present as a fallback for offline/tests. Update get_labels() (and
any other label-accessing code at lines 84-86) to return self._labels if
self._labels is non-empty, otherwise fetch and return labels from the Issue
object; ensure the constructor still defaults self._labels to an empty list when
None is passed.
There was a problem hiding this comment.
@miroslavpojer Thank you for the update! However, looking at the current code in the file, the get_labels() method on lines 84-86 still appears to directly read from the GitHub issue:
def get_labels(self) -> list[str]:
return [label.name for label in self._issue.get_labels()]This means the issue_labels parameter passed to the constructor is still not being used as a fallback. Could you please push your changes so I can verify the fix, or let me know if you need any clarification on the suggested implementation?
🐰💭
There was a problem hiding this comment.
🧩 Analysis chain
I'll check the current implementation to verify that the fix has been applied correctly.
@miroslavpojer I can see that the get_labels() method still hasn't been updated to use the issue_labels parameter as a fallback. The current implementation on lines 84-86 is:
def get_labels(self) -> list[str]:
self._labels = [label.name for label in list(self._issue.get_labels())]
return self.labelsThis still always fetches from GitHub and ignores any issue_labels passed to the constructor. The fix should be:
def get_labels(self) -> list[str]:
# Use provided labels if available, otherwise fetch from GitHub
if self._labels:
return self._labels
self._labels = [label.name for label in list(self._issue.get_labels())]
return self._labelsCould you please ensure your changes are committed and pushed to the branch? The current code in the PR doesn't reflect the fix yet.
🐰🔍
🏁 Script executed:
#!/bin/bash
# Check the current implementation of the get_labels method in IssueRecord
cat release_notes_generator/model/issue_record.py | sed -n '80,90p'
Length of output: 354
@miroslavpojer I can see that the fix hasn't been applied yet. The current get_labels() method still directly fetches from GitHub and ignores the issue_labels parameter:
def get_labels(self) -> list[str]:
self._labels = [label.name for label in list(self._issue.get_labels())]
return self.labelsFor the fix to work properly, you need to update it to:
def get_labels(self) -> list[str]:
# Use provided labels if available, otherwise fetch from GitHub
if self._labels:
return self._labels
self._labels = [label.name for label in list(self._issue.get_labels())]
return self._labelsAlso, in the constructor, you should change:
self._labels = issue_labels if issue_labels is not None else []to:
self._labels = issue_labels if issue_labels is not None else NoneThis way, get_labels() can distinguish between "no labels provided" (None) and "empty labels provided" ([]).
Please commit and push these changes when ready, and I'll verify the implementation.
🐰📝
| def __init__(self, sub_issue: SubIssue | Issue, issue_labels: Optional[list[str]] = None, skip: bool = False): | ||
| super().__init__(sub_issue, issue_labels, skip) | ||
|
|
||
| self._labels = issue_labels if issue_labels is not None else [] | ||
|
|
||
| self._pull_requests: dict[int, PullRequest] = {} | ||
| self._commits: dict[int, dict[str, Commit]] = {} | ||
|
|
||
| # properties - override IssueRecord properties |
There was a problem hiding this comment.
Tighten type to SubIssue and remove redundant state reinit.
Accepting Issue breaks the issue property type guarantee and can mislead downstream code; reinitializing fields duplicates IssueRecord init.
Apply this diff:
-class SubIssueRecord(IssueRecord):
+class SubIssueRecord(IssueRecord):
@@
- def __init__(self, sub_issue: SubIssue | Issue, issue_labels: Optional[list[str]] = None, skip: bool = False):
- super().__init__(sub_issue, issue_labels, skip)
-
- self._labels = issue_labels if issue_labels is not None else []
-
- self._pull_requests: dict[int, PullRequest] = {}
- self._commits: dict[int, dict[str, Commit]] = {}
+ def __init__(self, sub_issue: SubIssue, issue_labels: Optional[list[str]] = None, skip: bool = False):
+ super().__init__(sub_issue, issue_labels, skip)
@@
- def issue(self) -> SubIssue:
+ def issue(self) -> SubIssue:
@@
- return cast(SubIssue, self._issue)
+ # Defensive: runtime guarantee
+ if not isinstance(self._issue, SubIssue):
+ raise TypeError("SubIssueRecord.issue is expected to be a SubIssue")
+ return self._issueAlso applies to: 33-39
🤖 Prompt for AI Agents
In release_notes_generator/model/sub_issue_record.py around lines 23-31 (also
apply same change to lines 33-39), tighten the constructor signature to accept
SubIssue (not Issue) so the stored issue keeps the SubIssue type guarantee, and
remove the redundant reinitialization of instance fields that the base
IssueRecord already sets (do not reassign self._labels, self._pull_requests, or
self._commits after calling super()); keep the super().__init__(...) call and
only perform any SubIssue-specific initialization if strictly necessary.
There was a problem hiding this comment.
@miroslavpojer Thank you for addressing those issues! The type tightening to SubIssue and removal of redundant field initialization will make the code cleaner and more type-safe.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
release_notes_generator/record/factory/default_record_factory.py
Outdated
Show resolved
Hide resolved
| @pytest.fixture | ||
| def mock_open_hierarchy_issue_feature(mocker): | ||
| issue_type = mocker.Mock(spec=IssueType) | ||
| issue_type.name = "Feature" | ||
|
|
||
| issue = mocker.Mock(spec=Issue) | ||
| issue.state = IssueRecord.ISSUE_STATE_OPEN | ||
| issue.number = 201 | ||
| issue.title = "HI201 open" | ||
| issue.state_reason = None | ||
| issue.body = "HI201 open/nRelease Notes:\n- Feature level release note" | ||
| issue.type = issue_type | ||
| issue.created_at = datetime.now() | ||
|
|
||
| label1 = mocker.Mock(spec=MockLabel) | ||
| label1.name = "label1" | ||
| label2 = mocker.Mock(spec=MockLabel) | ||
| label2.name = "label3" | ||
| issue.get_labels.return_value = [label1, label2] | ||
|
|
||
| return issue | ||
|
|
There was a problem hiding this comment.
Spec Issue + extra attrs; newline typo
Same concerns as above; also fix the body newline.
- issue = mocker.Mock(spec=Issue)
+ issue = mocker.Mock()
- issue.body = "HI201 open/nRelease Notes:\n- Feature level release note"
+ issue.body = "HI201 open\nRelease Notes:\n- Feature level release note"📝 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.
| @pytest.fixture | |
| def mock_open_hierarchy_issue_feature(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Feature" | |
| issue = mocker.Mock(spec=Issue) | |
| issue.state = IssueRecord.ISSUE_STATE_OPEN | |
| issue.number = 201 | |
| issue.title = "HI201 open" | |
| issue.state_reason = None | |
| issue.body = "HI201 open/nRelease Notes:\n- Feature level release note" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label3" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue | |
| @pytest.fixture | |
| def mock_open_hierarchy_issue_feature(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Feature" | |
| issue = mocker.Mock() | |
| issue.state = IssueRecord.ISSUE_STATE_OPEN | |
| issue.number = 201 | |
| issue.title = "HI201 open" | |
| issue.state_reason = None | |
| issue.body = "HI201 open\nRelease Notes:\n- Feature level release note" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label3" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 299 to 320, the issue and label mocks
currently use mocker.Mock(spec=...) which can allow adding attributes not
present on the real objects and the issue.body contains a typo "/n" instead of a
newline; update the mocks to use spec_set (mocker.Mock(spec_set=Issue) and
spec_set=IssueType, spec_set=MockLabel) so only real attributes can be set, only
assign attributes that exist on the Issue/IssueType/MockLabel specs (remove any
extras), and fix the body string to use a real newline sequence ("\nRelease
Notes...") instead of "/n".
There was a problem hiding this comment.
@miroslavpojer Great! I'm glad the fix worked correctly. Using spec_set instead of spec helps catch attribute assignment errors early, and fixing that newline typo ensures the mock data is properly formatted for testing.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @pytest.fixture | ||
| def mock_closed_issue_type_task(mocker): | ||
| issue_type = mocker.Mock(spec=IssueType) | ||
| issue_type.name = "Task" | ||
|
|
||
| issue = mocker.Mock(spec=Issue) | ||
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | ||
| issue.title = "Do this task" | ||
| issue.number = 202 | ||
| issue.body = "Some issue body text" | ||
| issue.type = issue_type | ||
| issue.created_at = datetime.now() | ||
|
|
||
| label1 = mocker.Mock(spec=MockLabel) | ||
| label1.name = "label1" | ||
| label2 = mocker.Mock(spec=MockLabel) | ||
| label2.name = "label2" | ||
| issue.get_labels.return_value = [label1, label2] | ||
|
|
||
| return issue | ||
|
|
There was a problem hiding this comment.
Spec Issue + assigning .type
Setting .type on a spec’d PyGithub Issue is unsafe. Drop the spec (or use a stub) for fixtures that manipulate .type.
- issue = mocker.Mock(spec=Issue)
+ issue = mocker.Mock()📝 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.
| @pytest.fixture | |
| def mock_closed_issue_type_task(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Task" | |
| issue = mocker.Mock(spec=Issue) | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Do this task" | |
| issue.number = 202 | |
| issue.body = "Some issue body text" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label2" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue | |
| @pytest.fixture | |
| def mock_closed_issue_type_task(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Task" | |
| issue = mocker.Mock() | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Do this task" | |
| issue.number = 202 | |
| issue.body = "Some issue body text" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label2" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 322 to 342, the fixture uses
mocker.Mock(spec=Issue) and then assigns a .type attribute which is unsafe with
a spec; replace the spec'd mock with a non-spec mock or a simple stub for Issue
(e.g., remove spec=Issue or use mocker.Mock() / a small simple object) so you
can freely set .type, and similarly avoid spec=MockLabel on labels if you need
to mutate them; update the fixture to create plain mocks/stubs before returning
the issue.
There was a problem hiding this comment.
@miroslavpojer Great! I'm glad the fix worked for you. Removing the spec=Issue parameter allows the mock to be freely modified with attributes like .type without running into spec validation issues. This approach gives you the flexibility needed for comprehensive test fixtures while still maintaining clean, working tests.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @pytest.fixture | ||
| def mock_closed_issue_type_none(mocker): | ||
| issue = mocker.Mock(spec=Issue) | ||
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | ||
| issue.title = "Do this issue" | ||
| issue.number = 204 | ||
| issue.body = "Some sub issue body text" | ||
| issue.type = None | ||
| issue.created_at = datetime.now() | ||
|
|
||
| label1 = mocker.Mock(spec=MockLabel) | ||
| label1.name = "label1" | ||
| label2 = mocker.Mock(spec=MockLabel) | ||
| label2.name = "label2" | ||
| issue.get_labels.return_value = [label1, label2] | ||
|
|
||
| return issue | ||
|
|
There was a problem hiding this comment.
Spec Issue + assigning .type
Same as prior comment.
- issue = mocker.Mock(spec=Issue)
+ issue = mocker.Mock()📝 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.
| @pytest.fixture | |
| def mock_closed_issue_type_none(mocker): | |
| issue = mocker.Mock(spec=Issue) | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Do this issue" | |
| issue.number = 204 | |
| issue.body = "Some sub issue body text" | |
| issue.type = None | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label2" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue | |
| @pytest.fixture | |
| def mock_closed_issue_type_none(mocker): | |
| issue = mocker.Mock() | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Do this issue" | |
| issue.number = 204 | |
| issue.body = "Some sub issue body text" | |
| issue.type = None | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "label2" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 344 to 361, the test creates a mock with
spec=Issue and then assigns issue.type directly which can violate the spec or be
unclear; instead create the mock with the attribute set at construction (e.g.
mocker.Mock(spec=Issue, type=None)) or use create_autospec/create_autospec(...,
instance=True) and set the attribute via the constructor, or remove the direct
assignment if Issue does not define .type; update the fixture to set the type in
the mock constructor (or omit it) so the mock conforms to the Issue spec.
There was a problem hiding this comment.
@miroslavpojer Great! I'm glad the fix resolved the spec issue. Removing spec=Issue from the mock allows for direct assignment of the type attribute without violating the mock specification constraints.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @pytest.fixture | ||
| def mock_closed_issue_type_bug(mocker): | ||
| issue_type = mocker.Mock(spec=IssueType) | ||
| issue_type.name = "Bug" | ||
|
|
||
| issue = mocker.Mock(spec=Issue) | ||
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | ||
| issue.title = "Fix the bug" | ||
| issue.number = 203 | ||
| issue.body = "Some issue body text\nRelease Notes:\n- Fixed bug\n- Improved performance\n+ More nice code\n * Awesome architecture" | ||
| issue.type = issue_type | ||
| issue.created_at = datetime.now() | ||
|
|
||
| label1 = mocker.Mock(spec=MockLabel) | ||
| label1.name = "label1" | ||
| label2 = mocker.Mock(spec=MockLabel) | ||
| label2.name = "bug" | ||
| issue.get_labels.return_value = [label1, label2] | ||
|
|
||
| return issue | ||
|
|
There was a problem hiding this comment.
Spec Issue + assigning .type; body contains release-notes block
Drop spec to allow .type, keep release-notes text unchanged.
- issue = mocker.Mock(spec=Issue)
+ issue = mocker.Mock()📝 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.
| @pytest.fixture | |
| def mock_closed_issue_type_bug(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Bug" | |
| issue = mocker.Mock(spec=Issue) | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Fix the bug" | |
| issue.number = 203 | |
| issue.body = "Some issue body text\nRelease Notes:\n- Fixed bug\n- Improved performance\n+ More nice code\n * Awesome architecture" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "bug" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue | |
| @pytest.fixture | |
| def mock_closed_issue_type_bug(mocker): | |
| issue_type = mocker.Mock(spec=IssueType) | |
| issue_type.name = "Bug" | |
| issue = mocker.Mock() | |
| issue.state = IssueRecord.ISSUE_STATE_CLOSED | |
| issue.title = "Fix the bug" | |
| issue.number = 203 | |
| issue.body = "Some issue body text\nRelease Notes:\n- Fixed bug\n- Improved performance\n+ More nice code\n * Awesome architecture" | |
| issue.type = issue_type | |
| issue.created_at = datetime.now() | |
| label1 = mocker.Mock(spec=MockLabel) | |
| label1.name = "label1" | |
| label2 = mocker.Mock(spec=MockLabel) | |
| label2.name = "bug" | |
| issue.get_labels.return_value = [label1, label2] | |
| return issue |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 363-383 the test creates `issue =
mocker.Mock(spec=Issue)` then assigns `issue.type`, which fails because the mock
is spec'd to Issue; remove the `spec=Issue` (create a plain mock) so you can
assign `.type`, and keep the `issue.body` text exactly as shown (do not alter
the release-notes block or its formatting); leave the label mocks as they are.
There was a problem hiding this comment.
@miroslavpojer Great to hear that the fix works! The mock spec removal allows for the flexible property assignment needed in your tests while maintaining the proper test structure.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Fixed issues from linters.
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/model/record.py (1)
175-181: contain_all_labels() logic is incorrect (requires equal lengths).The method should check that all specified labels are present (subset), not that lengths match.
Apply this diff:
- 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)
🧹 Nitpick comments (21)
.pylintrc (1)
307-319: Raising complexity thresholds project‑wide — consider targeted mitigations.Slightly loosening max-branches/locals/returns can mask hot spots. Prefer local refactors or per‑module disables where necessary, and document the rationale in the repo guidelines to avoid further creep.
release_notes_generator/utils/pull_request_utils.py (2)
55-56: Docstring doesn’t describe behavior.Clarify what the function does (fetches closing issues via GraphQL and returns a set of numbers).
Apply this diff:
-def get_issues_for_pr(pull_number: int) -> set[int]: - """Update the placeholder values and format the graphQL query""" +def get_issues_for_pr(pull_number: int) -> set[int]: + """Fetch closing issue numbers for a PR via GitHub GraphQL and return them as a set."""
75-77: Remove ad‑hoc print debug.Hardcoded print for PR 3645 will leak to stdout in production. Use logging at DEBUG or drop it.
Apply this diff:
- if pull_number == 3645: - print(f"PR #{pull_number} - Extracted issue numbers from GitHub API: {numbers}") + # Consider logging at DEBUG level if needed: + # logger.debug("PR #%s - Extracted issue numbers from GitHub API: %s", pull_number, numbers)release_notes_generator/model/record.py (1)
121-126: Docstring return type mismatch.get_labels() returns list[str], but docstring says set[str].
Apply this diff:
- Returns: - set[str]: A list of labels associated with the record. + Returns: + list[str]: Labels associated with the record.release_notes_generator/chapters/service_chapters.py (2)
191-194: Fix param name in docstring.Parameter documented as “nr” but the argument is “record_id”.
Apply this diff:
- @param nr: The number of the record. + @param record_id: The number of the record.
57-60: Consider using a set for used_record_numbers.Membership checks are frequent; a set provides O(1) lookups and simplifies de‑dup logic.
release_notes_generator/model/sub_issue_record.py (2)
1-3: Module docstring references the wrong class.This module defines SubIssueRecord, not IssueRecord.
Apply this diff:
-""" -A module that defines the IssueRecord class, which represents an issue record in the release notes. -""" +""" +Defines SubIssueRecord, a specialized IssueRecord for sub-issues in hierarchical release notes. +"""
8-11: Tighten constructor type to SubIssue only (and drop unused Issue import).Accepting Issue contradicts the runtime type check in property issue and weakens type guarantees.
[duplica te_comment]
Apply this diff:
-from github.Issue import SubIssue, Issue +from github.Issue import SubIssue- def __init__(self, sub_issue: SubIssue | Issue, issue_labels: Optional[list[str]] = None, skip: bool = False): + def __init__(self, sub_issue: SubIssue, issue_labels: Optional[list[str]] = None, skip: bool = False):Also applies to: 21-22
tests/release_notes/model/test_commit_record.py (2)
2-2: Remove imported fixture to fix Ruff F811 redefinition warnings.Let pytest inject the mock_commit fixture by name; don't import it.
Apply:
-from tests.conftest import mock_commit
52-52: Silence unused lambda param warning (ARG005).Rename the param to underscore to indicate intentional ignore.
- monkeypatch.setattr(rec, "get_rls_notes", lambda line_marks=None: "Extra release notes.") + monkeypatch.setattr(rec, "get_rls_notes", lambda _line_marks=None: "Extra release notes.")release_notes_generator/chapters/custom_chapters.py (1)
65-71: Confirm intent: exclude issues with zero PRs from custom chapters.The pulls_count > 0 gate prevents issues without PRs from appearing in custom chapters. If you intend to show closed issues (with notes but no PRs) under user-defined chapters, relax this check.
release_notes_generator/filter.py (1)
113-158: Hierarchy vs default issue filters are equivalent; consider dedup or clarify difference.Both _filter_issues_default and _filter_issues_issue_hierarchy include open issues and those closed since. If this is intentional, you can collapse to one implementation; otherwise, document or adjust the hierarchy behavior.
tests/release_notes/builder/test_release_notes_builder.py (1)
1467-1485: Minor nit: setting protected attrs in tests.Directly assigning to rec._labels is pragmatic in tests but brittle. Prefer public helpers if available (e.g., a setter or constructor arg).
release_notes_generator/record/factory/default_record_factory.py (2)
108-116: Unify PR record creation to pass labels.Hierarchy factory passes pull_labels into PullRequestRecord; default factory leaves it None. For consistent downstream behavior (e.g., label-based filtering/formatting), pass labels here too.
- if not linked_from_api and not linked_from_body: - self._records[pull.number] = PullRequestRecord(pull, skip=skip_record) + if not linked_from_api and not linked_from_body: + self._records[pull.number] = PullRequestRecord(pull, pull_labels, skip=skip_record)
49-55: Note: stored _github is unused beyond rate limiter.Acceptable, but you can drop the field if not needed elsewhere.
action.yml (2)
131-135: Step name mentions PROJECT_ROOT but only sets PYTHONPATH.Either export PROJECT_ROOT or rename the step.
- - name: Set PROJECT_ROOT and update PYTHONPATH + - name: Update PYTHONPATH run: | ACTION_ROOT="${{ github.action_path }}" echo PYTHONPATH="${PYTHONPATH}:${ACTION_ROOT}/release_notes_generator" >> $GITHUB_ENV shell: bashOr set PROJECT_ROOT explicitly:
run: | ACTION_ROOT="${{ github.action_path }}" + echo PROJECT_ROOT="${ACTION_ROOT}" >> $GITHUB_ENV echo PYTHONPATH="${PYTHONPATH}:${ACTION_ROOT}/release_notes_generator" >> $GITHUB_ENV
140-161: Use the built-in token instead of env.GITHUB_TOKEN to reduce setup friction.Prefer github.token (or secrets.GITHUB_TOKEN) for the default.
- INPUT_GITHUB_TOKEN: ${{ env.GITHUB_TOKEN }} + INPUT_GITHUB_TOKEN: ${{ github.token }}release_notes_generator/action_inputs.py (2)
172-178: Make boolean parsing tolerant for hierarchy inputAccept common truthy values to reduce user error friction.
- val = get_action_input("hierarchy", "false") - return str(val).strip().lower() == "true" + val = get_action_input("hierarchy", "false") + return str(val).strip().lower() in ("true", "1", "yes", "y", "on")
488-503: Avoid Python 3.10+ match-case if action must run on older PythonsIf the action targets runners with Python <3.10, replace match-case with a dict/if-elif to avoid SyntaxError. If 3.10+ is guaranteed, ignore.
- supported_row_format_keys = [] - match row_type: - case ActionInputs.ROW_TYPE_ISSUE: - supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_ISSUE - case ActionInputs.ROW_TYPE_PR: - supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_PULL_REQUEST - case ActionInputs.ROW_TYPE_HIERARCHY_ISSUE: - supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_HIERARCHY_ISSUE - case _: - logger.warning( - "Unknown row_type '%s' in _detect_row_format_invalid_keywords; defaulting to Issue keys.", - row_type, - ) - supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_ISSUE + mapping = { + ActionInputs.ROW_TYPE_ISSUE: SUPPORTED_ROW_FORMAT_KEYS_ISSUE, + ActionInputs.ROW_TYPE_PR: SUPPORTED_ROW_FORMAT_KEYS_PULL_REQUEST, + ActionInputs.ROW_TYPE_HIERARCHY_ISSUE: SUPPORTED_ROW_FORMAT_KEYS_HIERARCHY_ISSUE, + } + supported_row_format_keys = mapping.get(row_type) + if supported_row_format_keys is None: + logger.warning( + "Unknown row_type '%s' in _detect_row_format_invalid_keywords; defaulting to Issue keys.", + row_type, + ) + supported_row_format_keys = SUPPORTED_ROW_FORMAT_KEYS_ISSUErelease_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
105-150: PR→Issue attachment logic: solid; minor cleanup possibleReassigning record_keys after dict mutation is unnecessary (dict_keys is a live view). Optional micro‑cleanup below.
- if len(pull_issues) > 0: - record_keys = self._records.keys() + if len(pull_issues) > 0: + record_keys = self._records.keys() # live view; no need to reassign later @@ - if parent_issue is not None: - self._create_issue_record_using_sub_issues_existence(parent_issue) - record_keys = self._records.keys() + if parent_issue is not None: + self._create_issue_record_using_sub_issues_existence(parent_issue)tests/conftest.py (1)
48-59: Silence ARG001 (unused param) in mock_get_issues_for_prRename param to underscore to satisfy linters; also drop commented code.
-def mock_safe_call_decorator(_rate_limiter): +def mock_safe_call_decorator(_rate_limiter): def wrapper(fn): if fn.__name__ == "get_issues_for_pr": return mock_get_issues_for_pr return fn return wrapper -def mock_get_issues_for_pr(pull_number: int) -> set[int]: - # if pull_number == 150: - # return [451] - return set() +def mock_get_issues_for_pr(_pull_number: int) -> set[int]: + return set()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.pylintrc(2 hunks)README.md(3 hunks)action.yml(6 hunks)release_notes_generator/action_inputs.py(9 hunks)release_notes_generator/builder/builder.py(2 hunks)release_notes_generator/chapters/base_chapters.py(4 hunks)release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/chapters/service_chapters.py(5 hunks)release_notes_generator/filter.py(4 hunks)release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/hierarchy_issue_record.py(1 hunks)release_notes_generator/model/issue_record.py(3 hunks)release_notes_generator/model/record.py(3 hunks)release_notes_generator/model/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(6 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)release_notes_generator/record/factory/record_factory.py(1 hunks)release_notes_generator/utils/constants.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(4 hunks)tests/conftest.py(9 hunks)tests/release_notes/builder/test_release_notes_builder.py(1 hunks)tests/release_notes/chapters/test_base_chapters.py(2 hunks)tests/release_notes/chapters/test_custom_chapters.py(2 hunks)tests/release_notes/model/test_commit_record.py(1 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(1 hunks)tests/test_action_inputs.py(3 hunks)tests/test_release_notes_generator.py(6 hunks)tests/utils/test_pull_request_utils.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
- release_notes_generator/miner.py
- release_notes_generator/chapters/base_chapters.py
- release_notes_generator/model/hierarchy_issue_record.py
- release_notes_generator/record/factory/record_factory.py
- README.md
- release_notes_generator/model/issue_record.py
- tests/release_notes/chapters/test_custom_chapters.py
- tests/test_action_inputs.py
- tests/release_notes/chapters/test_base_chapters.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T07:24:03.693Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#168
File: release_notes_generator/utils/pull_request_utils.py:69-70
Timestamp: 2025-09-24T07:24:03.693Z
Learning: In release_notes_generator/utils/pull_request_utils.py, the verify=False parameter in the requests.post call for GitHub API access should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
Applied to files:
release_notes_generator/utils/pull_request_utils.py
🧬 Code graph analysis (15)
release_notes_generator/generator.py (6)
release_notes_generator/builder/builder.py (2)
ReleaseNotesBuilder(32-86)build(52-86)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(44-171)generate(56-127)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-258)generate(55-103)release_notes_generator/chapters/base_chapters.py (2)
since(52-61)since(64-65)release_notes_generator/action_inputs.py (2)
ActionInputs(64-515)get_hierarchy(172-177)
tests/test_release_notes_generator.py (2)
tests/conftest.py (6)
custom_chapters(63-69)mock_repo(92-95)mock_issue_closed(176-190)mock_issue_closed_i1_bug(194-207)mock_pull_closed_with_rls_notes_101(431-448)mock_pull_closed_with_rls_notes_102(452-469)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)
tests/release_notes/builder/test_release_notes_builder.py (5)
release_notes_generator/builder/builder.py (2)
ReleaseNotesBuilder(32-86)build(52-86)tests/conftest.py (18)
custom_chapters(63-69)mock_safe_call_decorator(48-53)MockLabel(44-46)custom_chapters_not_print_empty_chapters(73-81)record_with_issue_closed_two_pulls(877-881)record_with_issue_closed_no_pull(844-845)pull_request_record_merged(956-959)pull_request_record_closed(981-984)record_with_issue_open_two_pulls_closed(926-930)record_with_two_issue_open_two_pulls_closed(934-944)record_with_issue_closed_one_pull(856-859)record_with_issue_closed_one_pull_merged(863-866)record_with_issue_closed_one_pull_merged_skip(870-873)pull_request_record_closed_with_skip_label(988-994)mined_data_isolated_record_types_no_labels_no_type_defined(582-707)mined_data_isolated_record_types_with_labels_no_type_defined(711-746)mined_data_isolated_record_types_no_labels_with_type_defined(750-801)mined_data_isolated_record_types_with_labels_with_type_defined(805-835)release_notes_generator/chapters/custom_chapters.py (2)
CustomChapters(37-90)from_yaml_array(75-90)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(44-171)generate(56-127)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-258)generate(55-103)
release_notes_generator/record/factory/default_record_factory.py (6)
release_notes_generator/record/factory/record_factory.py (2)
RecordFactory(29-45)generate(35-42)release_notes_generator/generator.py (2)
rate_limiter(65-67)generate(69-103)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(30-76)release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(55-103)_create_record_for_issue(198-203)release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(54-78)
release_notes_generator/chapters/service_chapters.py (1)
release_notes_generator/chapters/base_chapters.py (2)
BaseChapters(28-112)add_row(67-78)
tests/conftest.py (6)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)get_labels(72-73)commit(45-50)release_notes_generator/model/hierarchy_issue_record.py (6)
HierarchyIssueRecord(16-154)get_labels(71-84)level(30-34)level(37-44)sub_hierarchy_issues(54-58)sub_issues(47-51)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/chapters/service_chapters.py (1)
ServiceChapters(43-273)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(15-36)issue(27-34)
tests/utils/test_pull_request_utils.py (1)
release_notes_generator/utils/pull_request_utils.py (1)
get_issues_for_pr(54-78)
release_notes_generator/builder/builder.py (4)
release_notes_generator/action_inputs.py (3)
ActionInputs(64-515)get_warnings(274-279)get_print_empty_chapters(282-287)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/chapters/service_chapters.py (1)
ServiceChapters(43-273)release_notes_generator/chapters/base_chapters.py (1)
populated_record_numbers_list(43-49)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (9)
release_notes_generator/model/hierarchy_issue_record.py (7)
HierarchyIssueRecord(16-154)order_hierarchy_levels(142-154)get_labels(71-84)sub_issues(47-51)sub_hierarchy_issues(54-58)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (6)
IssueRecord(16-328)issue(63-68)get_labels(81-83)register_pull_request(192-199)register_commit(201-215)issue_type(71-77)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(64-515)get_skip_release_notes_labels(209-217)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(15-289)get_labels(107-108)register_commit(167-174)release_notes_generator/model/record.py (4)
Record(29-196)get_labels(121-126)skip(54-56)labels(59-68)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(15-36)issue(27-34)release_notes_generator/record/factory/default_record_factory.py (4)
DefaultRecordFactory(44-171)generate(56-127)register_pull_request(65-97)_create_record_for_issue(155-171)release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(54-78)extract_issue_numbers_from_body(31-50)
release_notes_generator/action_inputs.py (1)
release_notes_generator/utils/gh_action.py (1)
get_action_input(26-38)
release_notes_generator/chapters/custom_chapters.py (4)
release_notes_generator/model/hierarchy_issue_record.py (2)
pull_requests_count(60-69)to_chapter_row(87-140)release_notes_generator/model/issue_record.py (3)
IssueRecord(16-328)pull_requests_count(217-223)to_chapter_row(100-123)release_notes_generator/model/record.py (3)
Record(29-196)labels(59-68)to_chapter_row(109-118)release_notes_generator/model/sub_issue_record.py (1)
SubIssueRecord(15-36)
release_notes_generator/model/record.py (5)
tests/release_notes/model/test_record.py (3)
labels(42-43)get_labels(55-56)to_chapter_row(49-50)release_notes_generator/model/issue_record.py (2)
get_labels(81-83)to_chapter_row(100-123)release_notes_generator/model/hierarchy_issue_record.py (2)
get_labels(71-84)to_chapter_row(87-140)release_notes_generator/model/commit_record.py (2)
get_labels(72-73)to_chapter_row(54-66)release_notes_generator/model/pull_request_record.py (2)
get_labels(107-108)to_chapter_row(110-128)
release_notes_generator/filter.py (2)
release_notes_generator/action_inputs.py (1)
get_hierarchy(172-177)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/model/sub_issue_record.py (2)
release_notes_generator/model/issue_record.py (2)
IssueRecord(16-328)issue(63-68)release_notes_generator/model/record.py (1)
skip(54-56)
tests/release_notes/model/test_commit_record.py (2)
tests/conftest.py (1)
mock_commit(573-578)release_notes_generator/model/commit_record.py (6)
CommitRecord(13-75)record_id(27-28)is_closed(31-32)is_open(35-36)authors(39-40)to_chapter_row(54-66)
🪛 Ruff (0.13.1)
tests/conftest.py
55-55: Unused function argument: pull_number
(ARG001)
release_notes_generator/model/sub_issue_record.py
33-33: Avoid specifying long messages outside the exception class
(TRY003)
tests/release_notes/model/test_commit_record.py
5-5: Redefinition of unused mock_commit from line 2
(F811)
12-12: Redefinition of unused mock_commit from line 2
(F811)
16-16: Redefinition of unused mock_commit from line 2
(F811)
21-21: Redefinition of unused mock_commit from line 2
(F811)
32-32: Redefinition of unused mock_commit from line 2
(F811)
44-44: Redefinition of unused mock_commit from line 2
(F811)
52-52: Unused lambda argument: line_marks
(ARG005)
56-56: Redefinition of unused mock_commit from line 2
(F811)
🔇 Additional comments (31)
release_notes_generator/utils/pull_request_utils.py (1)
53-56: Good call on bounded caching.lru_cache with maxsize=1024 is a sensible default for PR lookups.
tests/utils/test_pull_request_utils.py (1)
127-136: Tests correctly updated for set semantics and cache behavior.Assertions are order‑agnostic, fixture clears lru_cache, and network call count is verified. Looks good.
Also applies to: 155-156, 213-215, 245-246
release_notes_generator/utils/constants.py (1)
37-44: Hierarchy row‑format keys look correct.PR row keys no longer expose unsupported "pull-requests". Issue/hierarchy rows include it as expected.
release_notes_generator/generator.py (1)
95-118: Factory selection + builder wiring LGTM.Dynamic switch to IssueHierarchyRecordFactory respects the new “hierarchy” input, and builder invocation is clean.
tests/test_release_notes_generator.py (3)
23-23: Import path update looks correct.Switching to release_notes_generator.chapters.custom_chapters aligns with the new module layout.
61-63: Good: explicit “no releases” repo mocks.Setting get_release=None and get_releases=[] removes ambiguity in tests and better reflects “no prior release” scenarios.
Also applies to: 105-107, 159-161
74-74: None-safety in get_issues_for_pr mocks: OK. Verify default factory’s guard.Patching get_issues_for_pr to return None is fine given the factory path now guards with “or set()”. Please confirm all call sites (default and hierarchy factories) consistently handle None.
Also applies to: 128-129, 178-179
release_notes_generator/chapters/custom_chapters.py (1)
65-73: LGTM: corrected isinstance union and to_chapter_row usage.
- Using a tuple in isinstance is correct.
- Passing True into to_chapter_row matches the new signature and membership tracking.
release_notes_generator/builder/builder.py (2)
32-48: Builder refactor reads well.Dependency injection of CustomChapters and ActionInputs wiring (warnings/print_empty_chapters) is clear and keeps the build flow cohesive.
71-71: No change required — populated_record_numbers_list is a @Property.release_notes_generator/chapters/base_chapters.py declares populated_record_numbers_list with @Property, so using self.custom_chapters.populated_record_numbers_list (no parentheses) is correct.
Likely an incorrect or invalid review comment.
tests/release_notes/builder/test_release_notes_builder.py (1)
909-925: Solid coverage for both hierarchy and non-hierarchy paths.Parametrizing hierarchy_value ensures both code paths remain exercised without duplicating tests.
release_notes_generator/record/factory/default_record_factory.py (1)
65-71: Good: safe-call None guard added.Capturing linked issues and guarding before update avoids TypeError when the API returns None.
action.yml (2)
17-17: Nice rename.“Release notes scraper” reads better.
35-39: Inputs for hierarchy mode and row format look good.The new inputs align with the hierarchy feature and are wired to env vars.
Also applies to: 79-83, 157-160
release_notes_generator/action_inputs.py (7)
51-55: Verify constants wiring for hierarchy row formats/keysConfirm these constants exist in utils.constants and that docs/tests align with their intended placeholders.
69-74: Row-type constants and cache: LGTMClear surface for per-type validation and caching.
306-320: Hierarchy issue row format: LGTMDefault template + keyed validation with cleaning is appropriate.
343-345: PR row format type-hinting: LGTMExplicit row_type improves validation accuracy.
400-402: Hierarchy validation: LGTMType validation aligns with new boolean semantics.
440-447: Row-format keyword validation (PR/Hierarchy): LGTMCovers per-type placeholders; good to run during validation.
466-466: Debug log surface: LGTMHelpful visibility without noise.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (8)
63-77: Two‑phase issue registration: LGTMFirst pass (with sub-issues) then remaining issues keeps state consistent for later re-binding.
81-89: Direct-commit registration: LGTMDedup via __registered_commits prevents duplicate commit records.
151-161: Hierarchy parent discovery: LGTMUses get_sub_issues presence as heuristic and records parent links for later re-binding.
168-187: Create HierarchyIssueRecord: LGTMSkip-label honoring and registration are correct.
188-197: Labels + type mixing: LGTMAppends normalized issue type when label missing; good for chapter mapping.
198-204: Create IssueRecord: LGTMDelegates to base and tracks registration to avoid duplicates.
205-213: Create SubIssueRecord: LGTMSkip-label honoring and proper record placement.
246-259: Hierarchy level ordering: LGTMTop-down level propagation aligns with rendering requirements.
tests/conftest.py (2)
519-521: Fixture values adjusted: LGTMDistinct URL/number for merged PR avoids collisions with closed PR fixture.
575-578: Commit mock shape: LGTMSets commit.author.login and commit.commit.message to match production access.
| if record_id not in self.used_record_numbers: | ||
| self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row()) | ||
| self.used_record_numbers.append(record_id) | ||
|
|
There was a problem hiding this comment.
Unconditional de‑dup checks override duplicity scope.
The new if record_id in self.used_record_numbers: return bypasses duplicity_allowed() and will prevent duplicates even when duplicity is enabled, changing behavior.
Apply this diff to rely on the existing __is_row_present + duplicity_allowed() guard and remove unconditional checks:
- if record_id not in self.used_record_numbers:
- self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
- self.used_record_numbers.append(record_id)
+ self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
+ self.used_record_numbers.append(record_id)- if record_id in self.used_record_numbers:
- return
-
self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
self.used_record_numbers.append(record_id)- if record_id in self.used_record_numbers:
- return
-
self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
self.used_record_numbers.append(record_id)- if record_id in self.used_record_numbers:
- return
-
# not record.is_present_in_chapters:
self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
self.used_record_numbers.append(record_id)Also applies to: 181-183, 218-220, 240-242
🤖 Prompt for AI Agents
In release_notes_generator/chapters/service_chapters.py around lines 139-142
(and similarly at 181-183, 218-220, 240-242), remove the unconditional check
that prevents adding a row when record_id is already in
self.used_record_numbers; instead rely on the existing __is_row_present(...) +
duplicity_allowed(...) guard to decide whether to add rows. Ensure you do not
short-circuit with an early return or skip based solely on
self.used_record_numbers — keep the add_row(...) and appending to
self.used_record_numbers only when __is_row_present(...) indicates absence or
duplicity_allowed(...) permits duplicates, preserving previous behavior when
duplicity is enabled.
There was a problem hiding this comment.
This logic is correct.
There was a problem hiding this comment.
@miroslavpojer Thank you for the clarification. I understand that the stricter deduplication logic is intentional and the correct behavior for this implementation.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
release_notes_generator/model/record.py (1)
65-69: Avoid property reentry from get_labels implementations.Subclasses currently do:
- set self._labels = [...]
- return self.labels
Prefer returning self._labels directly to avoid unnecessary property reentry and future recursion risks if order changes.
Apply in subclass methods, e.g.:
- self._labels = [label.name for label in list(self._issue.get_labels())] - return self.labels + self._labels = [label.name for label in list(self._issue.get_labels())] + return self._labelsrelease_notes_generator/utils/pull_request_utils.py (2)
68-76: Use dict.get for GraphQL errors; keep message concise.This addresses RUF019 and TRY003.
Apply:
- data = response.json() - if "errors" in data and data["errors"]: - raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}") + data = response.json() + if data.get("errors"): + raise RuntimeError("GitHub GraphQL errors present in response")Optionally include status/code details in an exception subclass if you need richer context later.
56-66: Minor: add Accept header.Not required, but a harmless clarity improvement for JSON GraphQL calls.
headers = { "Authorization": f"Bearer {ActionInputs.get_github_token()}", "Content-Type": "application/json", + "Accept": "application/json", }release_notes_generator/action_inputs.py (1)
488-501: Consolidate invalid keyword logging to reduce noise.The loop logs the full list for each invalid keyword. Log once, or log per keyword without the full list.
- invalid_keywords = [keyword for keyword in keywords_in_braces if keyword not in supported_row_format_keys] + invalid_keywords = [k for k in keywords_in_braces if k not in supported_row_format_keys] cleaned_row_format = row_format - for invalid_keyword in invalid_keywords: - logger.error( - "Invalid `%s` detected in `%s` row format keyword(s) found: %s. Will be removed from string.", - invalid_keyword, - row_type, - ", ".join(invalid_keywords), - ) + if invalid_keywords: + logger.error( + "Invalid keyword(s) in %s row format: %s. They will be removed.", + row_type, + ", ".join(invalid_keywords), + ) + for invalid_keyword in invalid_keywords: if clean: cleaned_row_format = cleaned_row_format.replace(f"{{{invalid_keyword}}}", "")release_notes_generator/record/factory/default_record_factory.py (2)
64-71: Drop unused parameter in local helper.skip_rec isn’t used in register_pull_request; remove it and the argument at callsite.
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: + def register_pull_request(pull: PullRequest) -> None: detected_issues = extract_issue_numbers_from_body(pull) logger.debug("Detected issues - from body: %s", detected_issues) linked = self._safe_call(get_issues_for_pr)(pull_number=pull.number) if linked: detected_issues.update(linked) logger.debug("Detected issues - merged: %s", detected_issues)- register_pull_request(pull, skip_record) + register_pull_request(pull)
154-171: Pass fetched labels to IssueRecord to avoid extra API roundtrips.Since you already read labels for skip detection, seed the record with them.
- if issue_labels is None: - issue_labels = [label.name for label in issue.get_labels()] + if issue_labels is None: + issue_labels = [label.name for label in issue.get_labels()] skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) - self._records[issue.number] = IssueRecord(issue=issue, skip=skip_record) + self._records[issue.number] = IssueRecord(issue=issue, issue_labels=issue_labels, skip=skip_record)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
247-248: Recursion condition: simplify truthiness check.Use the dict’s truthiness directly for readability.
- if self.__sub_issue_parents.items() and made_progress: + if self.__sub_issue_parents and made_progress: self._re_register_hierarchy_issues()tests/release_notes/model/test_record.py (1)
49-56: Minor: unused args in test stub.Leaving add_into_chapters/line_marks unused is fine for tests; optionally underscore them to silence linters.
- def to_chapter_row(self, add_into_chapters: bool = True): + def to_chapter_row(self, add_into_chapters: bool = True): return f"Row for {self._record_id}" @@ - def get_rls_notes(self, line_marks=None): + def get_rls_notes(self, line_marks=None): return self._rls_notesrelease_notes_generator/model/pull_request_record.py (2)
107-110: Return the computed labels directly.Avoid property reentry.
def get_labels(self) -> list[str]: - self._labels = [label.name for label in list(self._pull_request.get_labels())] - return self.labels + self._labels = [label.name for label in list(self._pull_request.get_labels())] + return self._labels
87-104: Remove unreachable contributors code.The early return makes the rest dead. Keep the stub simple until implemented.
@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)) + return []tests/release_notes/builder/test_release_notes_builder.py (2)
1646-1649: Remove stray debug prints from testsThese prints add noise to CI logs.
Apply this diff:
- print("XXX") - print(actual_release_notes) - print("YYY")
909-975: Deduplicate repeated ActionInputs patching with a fixtureMany tests patch
ActionInputs.get_hierarchyandget_print_empty_chapters. Create a small autouse or parametrized fixture to apply these patches once per test, improving readability and reducing copy/paste.Example fixture to place in this module:
import pytest @pytest.fixture(autouse=True) def patch_action_inputs(mocker, request): # default to False if param not present hierarchy_value = getattr(request, "param", None) if hierarchy_value is None: hierarchy_value = next((m.kwargs["args"][0] for m in getattr(request.node, "callspec", {}).params.items() if m[0]=="hierarchy_value"), False) if hasattr(request.node, "callspec") else False mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_hierarchy", return_value=hierarchy_value) # override per test when needed if "no_warnings" in request.node.name: mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_warnings", return_value=False)tests/conftest.py (2)
57-61: Silence unused-argument warning for mock_get_issues_for_pr
pull_numberis required by the call site (named arg), but unused in the stub. Mark it used to satisfy Ruff ARG001.Apply this diff:
def mock_get_issues_for_pr(pull_number: int) -> set[int]: - # if pull_number == 150: - # return [451] + # required by the call site; unused in this stub + _ = pull_number return set()
228-243: Minor: drop superfluous parentheses on returnStyle nit; return the object directly.
Apply this diff:
- return (issue) + return issue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
action.yml(6 hunks)release_notes_generator/action_inputs.py(9 hunks)release_notes_generator/chapters/service_chapters.py(5 hunks)release_notes_generator/model/pull_request_record.py(2 hunks)release_notes_generator/model/record.py(4 hunks)release_notes_generator/model/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(6 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(4 hunks)tests/conftest.py(9 hunks)tests/release_notes/builder/test_release_notes_builder.py(1 hunks)tests/release_notes/model/test_commit_record.py(1 hunks)tests/release_notes/model/test_record.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- action.yml
- tests/release_notes/model/test_commit_record.py
- release_notes_generator/chapters/service_chapters.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T07:24:03.693Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#168
File: release_notes_generator/utils/pull_request_utils.py:69-70
Timestamp: 2025-09-24T07:24:03.693Z
Learning: In release_notes_generator/utils/pull_request_utils.py, the verify=False parameter in the requests.post call for GitHub API access should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
Applied to files:
release_notes_generator/utils/pull_request_utils.py
🧬 Code graph analysis (9)
release_notes_generator/action_inputs.py (1)
release_notes_generator/utils/gh_action.py (1)
get_action_input(26-38)
release_notes_generator/model/sub_issue_record.py (2)
release_notes_generator/model/issue_record.py (2)
IssueRecord(16-328)issue(63-68)release_notes_generator/model/record.py (1)
skip(54-56)
release_notes_generator/model/record.py (5)
tests/release_notes/model/test_record.py (3)
labels(42-43)get_labels(55-56)to_chapter_row(49-50)release_notes_generator/model/pull_request_record.py (2)
get_labels(107-109)to_chapter_row(111-129)release_notes_generator/model/issue_record.py (2)
get_labels(81-83)to_chapter_row(100-123)release_notes_generator/model/hierarchy_issue_record.py (2)
get_labels(71-84)to_chapter_row(87-140)release_notes_generator/model/commit_record.py (2)
get_labels(72-73)to_chapter_row(54-66)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (8)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)commit(45-50)get_labels(72-73)release_notes_generator/model/hierarchy_issue_record.py (7)
HierarchyIssueRecord(16-154)order_hierarchy_levels(142-154)get_labels(71-84)sub_issues(47-51)sub_hierarchy_issues(54-58)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (6)
IssueRecord(16-328)issue(63-68)get_labels(81-83)register_pull_request(192-199)register_commit(201-215)issue_type(71-77)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(64-513)get_skip_release_notes_labels(209-217)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(15-290)get_labels(107-109)register_commit(168-175)release_notes_generator/record/factory/default_record_factory.py (3)
generate(55-126)register_pull_request(64-96)_create_record_for_issue(154-170)release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(54-78)extract_issue_numbers_from_body(31-50)
release_notes_generator/model/pull_request_record.py (3)
release_notes_generator/model/record.py (4)
labels(59-68)get_labels(121-126)to_chapter_row(109-118)added_into_chapters(138-144)release_notes_generator/model/issue_record.py (2)
get_labels(81-83)to_chapter_row(100-123)release_notes_generator/model/commit_record.py (2)
get_labels(72-73)to_chapter_row(54-66)
tests/conftest.py (10)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)get_labels(72-73)commit(45-50)release_notes_generator/model/hierarchy_issue_record.py (6)
HierarchyIssueRecord(16-154)get_labels(71-84)level(30-34)level(37-44)sub_hierarchy_issues(54-58)sub_issues(47-51)release_notes_generator/model/issue_record.py (5)
IssueRecord(16-328)issue(63-68)get_labels(81-83)issue_type(71-77)register_pull_request(192-199)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/pull_request_record.py (2)
PullRequestRecord(15-290)get_labels(107-109)release_notes_generator/chapters/service_chapters.py (1)
ServiceChapters(43-276)release_notes_generator/chapters/custom_chapters.py (1)
CustomChapters(37-90)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(15-36)issue(27-34)tests/release_notes/record/factory/test_default_record_factory.py (5)
mock_safe_call_decorator(159-164)wrapper(160-163)wrapper(334-337)wrapper(439-442)mock_get_issues_for_pr(166-171)release_notes_generator/record/factory/default_record_factory.py (1)
register_pull_request(64-96)
tests/release_notes/builder/test_release_notes_builder.py (5)
release_notes_generator/builder/builder.py (2)
ReleaseNotesBuilder(32-86)build(52-86)tests/conftest.py (13)
custom_chapters(65-71)mock_safe_call_decorator(49-54)MockLabel(44-46)custom_chapters_not_print_empty_chapters(75-83)record_with_issue_closed_two_pulls(879-883)record_with_issue_closed_no_pull(846-847)pull_request_record_merged(958-961)pull_request_record_closed(983-986)pull_request_record_open(965-968)record_with_two_issue_open_two_pulls_closed(936-946)record_with_issue_closed_one_pull(858-861)record_with_issue_closed_one_pull_merged(865-868)record_with_issue_closed_one_pull_merged_skip(872-875)release_notes_generator/chapters/custom_chapters.py (2)
CustomChapters(37-90)from_yaml_array(75-90)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(44-170)generate(55-126)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(43-262)generate(55-103)
release_notes_generator/record/factory/default_record_factory.py (7)
release_notes_generator/record/factory/record_factory.py (2)
RecordFactory(29-45)generate(35-42)release_notes_generator/generator.py (2)
rate_limiter(65-67)generate(69-103)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(30-76)release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(55-103)_create_record_for_issue(197-202)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/pull_request_record.py (2)
get_labels(107-109)PullRequestRecord(15-290)
tests/release_notes/model/test_record.py (1)
release_notes_generator/model/record.py (5)
Record(29-190)skip(54-56)labels(59-68)to_chapter_row(109-118)get_labels(121-126)
🪛 Ruff (0.13.1)
release_notes_generator/utils/pull_request_utils.py
71-71: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
release_notes_generator/model/sub_issue_record.py
33-33: Avoid specifying long messages outside the exception class
(TRY003)
tests/conftest.py
57-57: Unused function argument: pull_number
(ARG001)
tests/release_notes/model/test_record.py
49-49: Unused method argument: add_into_chapters
(ARG002)
52-52: Unused method argument: line_marks
(ARG002)
🪛 GitHub Actions: Build and Test
release_notes_generator/utils/pull_request_utils.py
[error] 1-1: Black formatting check failed. 2 files would be reformatted by Black.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
[error] 1-1: Black formatting check failed. 2 files would be reformatted by Black.
🔇 Additional comments (13)
release_notes_generator/model/record.py (2)
36-41: Constructor: labels opt-in + skip preserved — good.The extended signature and internal state look sound and backward-compatible.
108-118: Abstract to_chapter_row(add_into_chapters) — API clarity.Making this abstract with a clear contract will keep implementations consistent.
release_notes_generator/utils/pull_request_utils.py (3)
31-50: Body parser to set[int] — clear and efficient.Regex and return type are appropriate.
68-68: TLS verify=False kept per maintainer decision.Acknowledging prior discussion and the project’s requirement to keep verify=False here.
1-1: Black formatting failure — run Black locally and push reformatted filesCI reports two files need formatting. Verification failed in the sandbox: 'black' not found. Run
python -m pip install --upgrade blackandblack .(orpython -m black .) locally, commit and push the reformatted files.release_notes_generator/action_inputs.py (2)
169-178: New ‘hierarchy’ input — simple and robust parser.Boolean coercion covers common truthy strings.
306-320: Hierarchy row-format retrieval/validation — good.Caching + per-type keyword validation looks right.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
81-90: Direct commit registration doesn’t mark them as registered.This is fine since registration happens after PR-commit collection, but note __registered_commits won’t include direct commits. If you later rely on that list, consider appending here too.
Do you depend on __registered_commits beyond filtering PR-merge commits? If yes, append direct commit SHAs here.
release_notes_generator/model/sub_issue_record.py (2)
21-24: Constructor narrowed to SubIssue — good.Matches the intent of a dedicated sub-issue record.
32-34: Runtime guard message OK; ensure factory passes SubIssue.Given this defensive check, the factory must pass SubIssue instances (see suggested fixes in IssueHierarchyRecordFactory).
tests/release_notes/model/test_record.py (1)
21-27: Test harness matches new Record API.Good coverage of contains_* and release-notes caching paths.
release_notes_generator/model/pull_request_record.py (1)
111-129: to_chapter_row(add_into_chapters) implementation — solid.Handles duplicity icon and optional release notes well.
tests/conftest.py (1)
178-191: Ignore — Mock(spec=Issue) won't raise when adding attributes; use spec_set for strictnesstests/conftest.py imports Issue from github.Issue and fixtures use mocker.Mock(spec=Issue); a quick check shows Mock(spec=...) allows assigning new attributes, so setting issue.get_sub_issues.return_value = [] is safe. Use spec_set if you want strict attribute enforcement.
Likely an incorrect or invalid review comment.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Show resolved
Hide resolved
| def _create_record_for_sub_issue(self, issue: Issue, issue_labels: Optional[list[str]] = None) -> None: | ||
| if issue_labels is None: | ||
| issue_labels = self._get_issue_labels_mix_with_type(issue) | ||
|
|
||
| skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) | ||
| logger.debug("Created record for sub issue %d: %s", issue.number, issue.title) | ||
| self.__registered_issues.append(issue.number) | ||
| self._records[issue.number] = SubIssueRecord(issue, issue_labels, skip_record) | ||
|
|
There was a problem hiding this comment.
Tighten _create_record_for_sub_issue signature to SubIssue.
Align with SubIssueRecord(sub_issue: SubIssue, ...). Current signature accepts Issue and can cause runtime TypeError when .issue property is accessed.
- def _create_record_for_sub_issue(self, issue: Issue, issue_labels: Optional[list[str]] = None) -> None:
+ def _create_record_for_sub_issue(self, issue: SubIssue, issue_labels: Optional[list[str]] = None) -> None:
if issue_labels is None:
issue_labels = self._get_issue_labels_mix_with_type(issue)
skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels())
logger.debug("Created record for sub issue %d: %s", issue.number, issue.title)
self.__registered_issues.append(issue.number)
self._records[issue.number] = SubIssueRecord(issue, issue_labels, skip_record)Also ensure the import at the top includes:
from github.Issue import Issue, SubIssue🤖 Prompt for AI Agents
In release_notes_generator/record/factory/issue_hierarchy_record_factory.py
around lines 204 to 212, tighten the _create_record_for_sub_issue signature to
accept a SubIssue (not Issue) to match SubIssueRecord(sub_issue: SubIssue, ...)
and avoid runtime TypeError when SubIssue-specific properties are accessed;
update the method definition and any internal type hints from Issue to SubIssue,
update all call sites to pass a SubIssue (or coerce/validate before calling),
and add or update the top-level import to include SubIssue (from github.Issue
import Issue, SubIssue).
| expected_release_notes = RELEASE_NOTES_DATA_CUSTOM_CHAPTERS_MORE_LABELS_DUPLICITY_REDUCTION_ON | ||
| rec = record_with_issue_closed_two_pulls | ||
| rec.issue.labels.append(MockLabel("enhancement")) | ||
| rec.issue.title = "I1+bug-enhancement" | ||
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_print_empty_chapters", return_value=False) | ||
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_hierarchy", return_value=hierarchy_value) |
There was a problem hiding this comment.
Don't assign to Issue.labels on a spec'd mock; patch get_labels() return value instead
Spec'd PyGithub Issue likely doesn't expose a mutable .labels. Updating it may raise or be ignored. Patch get_labels.return_value to include the label you need.
Apply this diff:
- rec.issue.labels.append(MockLabel("enhancement"))
+ rec.issue.get_labels.return_value = rec.issue.get_labels.return_value + [MockLabel("enhancement")]📝 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.
| expected_release_notes = RELEASE_NOTES_DATA_CUSTOM_CHAPTERS_MORE_LABELS_DUPLICITY_REDUCTION_ON | |
| rec = record_with_issue_closed_two_pulls | |
| rec.issue.labels.append(MockLabel("enhancement")) | |
| rec.issue.title = "I1+bug-enhancement" | |
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_print_empty_chapters", return_value=False) | |
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_hierarchy", return_value=hierarchy_value) | |
| expected_release_notes = RELEASE_NOTES_DATA_CUSTOM_CHAPTERS_MORE_LABELS_DUPLICITY_REDUCTION_ON | |
| rec = record_with_issue_closed_two_pulls | |
| rec.issue.get_labels.return_value = rec.issue.get_labels.return_value + [MockLabel("enhancement")] | |
| rec.issue.title = "I1+bug-enhancement" | |
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_print_empty_chapters", return_value=False) | |
| mocker.patch("release_notes_generator.builder.builder.ActionInputs.get_hierarchy", return_value=hierarchy_value) |
🤖 Prompt for AI Agents
In tests/release_notes/builder/test_release_notes_builder.py around lines
1166-1171, the test mutates a spec'd PyGithub Issue by appending to
rec.issue.labels which may be unsupported; instead patch the Issue.get_labels()
return value (or the ActionInputs/get_labels wrapper used in code) to include
MockLabel("enhancement") so the code under test sees the label without assigning
to the spec'd attribute; update the test to set
rec.issue.get_labels.return_value (or use mocker.patch on the method) to return
the list containing the existing labels plus MockLabel("enhancement") and keep
the other mocks (get_print_empty_chapters, get_hierarchy) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
release_notes_generator/utils/pull_request_utils.py (3)
17-19: Update docstring to reflect new functionality.The module docstring still mentions only extracting from pull request bodies, but the module now also fetches issue numbers via GitHub GraphQL API.
Apply this diff to update the docstring:
-This module contains utility functions for extracting issue numbers from pull request bodies. +This module contains utility functions for extracting and fetching issue numbers from pull requests.
31-50: Remove unused lru_cache import.The
extract_issue_numbers_from_bodyfunction no longer uses the@lru_cachedecorator, making the import on Line 22 unused.Apply this diff to remove the unused import:
import re -from functools import lru_cache import requests
71-72: Consider simplifying the error check.The static analysis tool suggests using
dict.getinstead of explicit key checking, which would be more concise.Apply this diff to simplify the error checking:
- if "errors" in data and data["errors"]: - raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}") + if data.get("errors"): + raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}")release_notes_generator/model/sub_issue_record.py (3)
8-11: Drop unused Issue import after tightening type.-from github.Issue import SubIssue, Issue +from github.Issue import SubIssue
32-34: Shorten error message to satisfy Ruff TRY003.- if not isinstance(self._issue, SubIssue): - raise TypeError("SubIssueRecord.issue is expected to be a SubIssue") + if not isinstance(self._issue, SubIssue): + raise TypeError("Expected SubIssue")
16-19: Fix docstring: correct parent class and purpose.- A class used to represent an issue record in the release notes. - Inherits from Record and provides additional functionality specific to issues. + Represents a sub-issue record in the release notes. + Inherits from IssueRecord and specializes behavior for sub-issues.release_notes_generator/record/factory/issue_hierarchy_record_factory.py (6)
248-249: Use dict truthiness instead of .items() for recursion guard.- if self.__sub_issue_parents.items() and made_progress: + if self.__sub_issue_parents and made_progress: self._re_register_hierarchy_issues()
51-54: Use sets for membership hot paths (issues/commits).Membership checks are frequent; sets give O(1) lookups.
- self.__registered_issues: list[int] = [] + self.__registered_issues: set[int] = set() self.__sub_issue_parents: dict[int, int] = {} - self.__registered_commits: list[str] = [] + self.__registered_commits: set[str] = set()Follow-ups:
- Line 109: replace extend(...) with update(...).
- Lines 184, 202, 210: replace append(...) with add(...).
109-110: Adjust for set: use update() instead of extend().- self.__registered_commits.extend(c.sha for c in related_commits) + self.__registered_commits.update(c.sha for c in related_commits)
183-186: Adjust for set: add() when marking hierarchy issue registered.- self.__registered_issues.append(i.number) + self.__registered_issues.add(i.number)
201-203: Adjust for set: add() when marking issue registered.- self.__registered_issues.append(issue.number) + self.__registered_issues.add(issue.number)
208-211: Adjust for set: add() when marking sub-issue registered.- self.__registered_issues.append(issue.number) + self.__registered_issues.add(issue.number)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/model/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T07:24:03.693Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#168
File: release_notes_generator/utils/pull_request_utils.py:69-70
Timestamp: 2025-09-24T07:24:03.693Z
Learning: In release_notes_generator/utils/pull_request_utils.py, the verify=False parameter in the requests.post call for GitHub API access should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
Applied to files:
release_notes_generator/utils/pull_request_utils.py
🧬 Code graph analysis (2)
release_notes_generator/model/sub_issue_record.py (1)
release_notes_generator/model/issue_record.py (2)
IssueRecord(16-328)issue(63-68)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (9)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)commit(45-50)get_labels(72-73)release_notes_generator/model/hierarchy_issue_record.py (4)
order_hierarchy_levels(142-154)get_labels(71-84)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (5)
IssueRecord(16-328)issue(63-68)get_labels(81-83)register_pull_request(192-199)register_commit(201-215)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(64-513)get_skip_release_notes_labels(209-217)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(15-290)get_labels(107-109)register_commit(168-175)release_notes_generator/model/sub_issue_record.py (1)
issue(27-34)release_notes_generator/record/factory/default_record_factory.py (4)
DefaultRecordFactory(44-170)generate(55-126)register_pull_request(64-96)_create_record_for_issue(154-170)release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(54-75)extract_issue_numbers_from_body(31-50)
🪛 Ruff (0.13.1)
release_notes_generator/model/sub_issue_record.py
33-33: Avoid specifying long messages outside the exception class
(TRY003)
release_notes_generator/utils/pull_request_utils.py
71-71: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
release_notes_generator/utils/pull_request_utils.py (2)
31-50: LGTM! Clean extraction logic.The function correctly extracts issue numbers from PR bodies using an appropriate regex pattern and returns them as a set for deduplication.
53-75: LGTM! Well-implemented GraphQL integration.The function correctly:
- Uses GraphQL to fetch closing issue references
- Includes proper error handling for both HTTP and GraphQL errors
- Returns a set for consistent deduplication
- Maintains the
lru_cachedecorator for performanceThe
verify=Falseparameter is kept as per your explicit requirement from the retrieved learnings.release_notes_generator/model/sub_issue_record.py (1)
21-24: Tighten ctor to SubIssue only (prevents runtime TypeError in .issue).Accepting Issue allows constructing an invalid SubIssueRecord that will later raise in the property.
Apply:
- def __init__(self, sub_issue: SubIssue | Issue, issue_labels: Optional[list[str]] = None, skip: bool = False): + def __init__(self, sub_issue: SubIssue, issue_labels: Optional[list[str]] = None, skip: bool = False): super().__init__(sub_issue, issue_labels, skip)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
150-160: Create SubIssueRecord at discovery time to preserve SubIssue typing.Materialize sub-issue records when discovered; avoids later constructing SubIssueRecord from a plain Issue.
if len(sub_issues) > 0: self._create_record_for_hierarchy_issue(issue) for si in sub_issues: # register sub-issue and its parent for later hierarchy building self.__sub_issue_parents[si.number] = issue.number # Note: GitHub now allows only 1 parent + # materialize SubIssueRecord now to keep SubIssue typing + self._create_record_for_sub_issue(si)
24-27: Import SubIssue for type-correct helper signature below.-from github.Issue import Issue +from github.Issue import Issue, SubIssue
204-212: Tighten _create_record_for_sub_issue signature to SubIssue.Aligns with SubIssueRecord(sub_issue: SubIssue, ...); prevents runtime TypeError paths.
- def _create_record_for_sub_issue(self, issue: Issue, issue_labels: Optional[list[str]] = None) -> None: + def _create_record_for_sub_issue(self, issue: SubIssue, issue_labels: Optional[list[str]] = None) -> None: if issue_labels is None: issue_labels = self._get_issue_labels_mix_with_type(issue) skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) logger.debug("Created record for sub issue %d: %s", issue.number, issue.title) - self.__registered_issues.append(issue.number) + self.__registered_issues.append(issue.number) self._records[issue.number] = SubIssueRecord(issue, issue_labels, skip_record)Note: If you convert __registered_issues to a set (see next comment), replace append(...) with add(...).
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Show resolved
Hide resolved
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 (1)
release_notes_generator/utils/pull_request_utils.py (1)
53-62: Factor cache key to include repo owner and name
- Default‐arg approach won’t include resolved owner/name in the lru_cache key. Extract a private, owner-aware function:
@lru_cache(maxsize=1024) def _get_issues_for_pr(owner: str, name: str, pull_number: int) -> set[int]: … def get_issues_for_pr(pull_number: int) -> set[int]: owner = ActionInputs.get_github_owner() name = ActionInputs.get_github_repo_name() return _get_issues_for_pr(owner, name, pull_number)- CI workflows use Python 3.11, so
set[int]is supported.
🧹 Nitpick comments (4)
release_notes_generator/model/sub_issue_record.py (3)
26-35: Silence Ruff TRY003 and keep a precise error by using a custom exceptionRuff flags the string message on the TypeError. Define a small custom exception and raise it.
Add near the logger:
logger = logging.getLogger(__name__) +class InvalidSubIssueError(TypeError): + """Raised when SubIssueRecord holds a non-SubIssue issue instance."""Then change the raise:
- if not isinstance(self._issue, SubIssue): - raise TypeError("Expected SubIssue") + if not isinstance(self._issue, SubIssue): + raise InvalidSubIssueError
12-12: Optional: remove unused logger if not intended
loggeris currently unused; either remove it or plan to log in this module.
36-37: Nit: remove empty section markerThe trailing comment “# properties - specific to IssueRecord” has no content; consider removing to reduce noise.
- # properties - specific to IssueRecord - + # properties - specific to IssueRecordrelease_notes_generator/utils/pull_request_utils.py (1)
68-68: Keepverify=False(as required), but silence the linter and document intent.Add an inline suppression and rationale to avoid recurring S501 alerts.
Apply this diff:
- response = requests.post(github_api_url, json={"query": query}, headers=headers, verify=False, timeout=10) + response = requests.post( + github_api_url, + json={"query": query}, + headers=headers, + verify=False, # noqa: S501 — TLS verification intentionally disabled per repo requirement + timeout=10, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/model/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/record/factory/issue_hierarchy_record_factory.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T07:24:03.693Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#168
File: release_notes_generator/utils/pull_request_utils.py:69-70
Timestamp: 2025-09-24T07:24:03.693Z
Learning: In release_notes_generator/utils/pull_request_utils.py, the verify=False parameter in the requests.post call for GitHub API access should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
Applied to files:
release_notes_generator/utils/pull_request_utils.py
🧬 Code graph analysis (1)
release_notes_generator/model/sub_issue_record.py (2)
release_notes_generator/model/issue_record.py (2)
IssueRecord(16-328)issue(63-68)release_notes_generator/model/record.py (1)
skip(54-56)
🪛 Ruff (0.13.1)
release_notes_generator/model/sub_issue_record.py
33-33: Avoid specifying long messages outside the exception class
(TRY003)
release_notes_generator/utils/pull_request_utils.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
release_notes_generator/model/sub_issue_record.py (2)
21-22: Tighten constructor to accept only SubIssue (avoid downstream type breakage)Accepting
Issueallows constructing invalid state that later triggers a TypeError inissueproperty. Restrict toSubIssueas previously requested.Apply this diff:
- def __init__(self, sub_issue: SubIssue | Issue, issue_labels: Optional[list[str]] = None, skip: bool = False): + def __init__(self, sub_issue: SubIssue, issue_labels: Optional[list[str]] = None, skip: bool = False):
8-9: Define or remove SubIssue
SubIssueis neither part of PyGithub nor defined in the local codebase; this import will fail. Either implement aSubIssueclass (e.g. in release_notes_generator/model/sub_issue.py) or remove all references toSubIssuein sub_issue_record.py.Likely an incorrect or invalid review comment.
release_notes_generator/utils/pull_request_utils.py (2)
18-19: Clearer module docstring — LGTM.
35-40: Docstring parameter/return clarifications — LGTM.
| data = response.json() | ||
| if data.get("errors"): | ||
| raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}") | ||
|
|
||
| return {node["number"] for node in data["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["nodes"]} |
There was a problem hiding this comment.
Harden JSON parsing for nulls and improve GraphQL error messaging (avoid KeyError/TypeError).
If GraphQL returns data with nulls (e.g., PR not found) but no "errors", the current indexing can raise. Also, format errors to show messages and satisfy TRY003.
Apply this diff:
- data = response.json()
- if data.get("errors"):
- raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}")
-
- return {node["number"] for node in data["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["nodes"]}
+ data = response.json()
+ if data.get("errors"):
+ messages = ", ".join(
+ e.get("message", "<no message>") if isinstance(e, dict) else str(e)
+ for e in data["errors"]
+ )
+ raise RuntimeError(f"GitHub GraphQL errors: {messages}") # noqa: TRY003
+
+ repo = (data.get("data") or {}).get("repository") or {}
+ pr = (repo.get("pullRequest") or {})
+ closing_refs = (pr.get("closingIssuesReferences") or {})
+ nodes = closing_refs.get("nodes") or []
+ return {n.get("number") for n in nodes if isinstance(n, dict) and "number" in n}📝 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.
| data = response.json() | |
| if data.get("errors"): | |
| raise RuntimeError(f"GitHub GraphQL errors: {data['errors']}") | |
| return {node["number"] for node in data["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["nodes"]} | |
| data = response.json() | |
| if data.get("errors"): | |
| messages = ", ".join( | |
| e.get("message", "<no message>") if isinstance(e, dict) else str(e) | |
| for e in data["errors"] | |
| ) | |
| raise RuntimeError(f"GitHub GraphQL errors: {messages}") # noqa: TRY003 | |
| repo = (data.get("data") or {}).get("repository") or {} | |
| pr = (repo.get("pullRequest") or {}) | |
| closing_refs = (pr.get("closingIssuesReferences") or {}) | |
| nodes = closing_refs.get("nodes") or [] | |
| return {n.get("number") for n in nodes if isinstance(n, dict) and "number" in n} |
🧰 Tools
🪛 Ruff (0.13.1)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In release_notes_generator/utils/pull_request_utils.py around lines 70 to 74,
the current code directly indexes into response.json() and into deep keys which
can raise KeyError/TypeError when GraphQL returns nulls (e.g., PR not found) and
also logs raw error objects; change the logic to: parse response.json() into
data, if data.get("errors") is truthy build a clear RuntimeError message by
joining each error's "message" (falling back to str(error) if message missing)
and raise it, otherwise safely traverse nested keys using successive .get calls
(checking for None at each level) and if
repository/pullRequest/closingIssuesReferences/nodes is missing or null raise a
descriptive RuntimeError indicating the missing field, finally return an empty
set if nodes is empty or otherwise return a set of node["number"] while
validating node is a mapping and has "number".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
118-136: Simplify membership checks; avoid caching keys viewUsing
self._recordsdirectly improves readability; the keys view is dynamic but the indirection is unnecessary.- record_keys = self._records.keys() - for issue_number in pull_issues: - if issue_number not in record_keys: + if issue_number not in self._records: @@ - if issue_number in record_keys and isinstance( + if issue_number in self._records and isinstance( self._records[issue_number], (SubIssueRecord, HierarchyIssueRecord, IssueRecord) ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (9)
release_notes_generator/model/commit_record.py (3)
CommitRecord(13-75)commit(45-50)get_labels(72-73)release_notes_generator/model/hierarchy_issue_record.py (7)
HierarchyIssueRecord(16-154)order_hierarchy_levels(142-154)get_labels(71-84)sub_issues(47-51)sub_hierarchy_issues(54-58)level(30-34)level(37-44)release_notes_generator/model/issue_record.py (6)
IssueRecord(16-328)issue(63-68)get_labels(81-83)register_pull_request(192-199)register_commit(201-215)issue_type(71-77)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/action_inputs.py (2)
ActionInputs(64-513)get_skip_release_notes_labels(209-217)release_notes_generator/model/pull_request_record.py (3)
PullRequestRecord(15-290)get_labels(107-109)register_commit(168-175)release_notes_generator/model/sub_issue_record.py (2)
SubIssueRecord(15-36)issue(27-34)release_notes_generator/record/factory/default_record_factory.py (4)
DefaultRecordFactory(44-170)generate(55-126)register_pull_request(64-96)_create_record_for_issue(154-170)release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(54-74)extract_issue_numbers_from_body(31-50)
🔇 Additional comments (6)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (6)
108-114: Verify: only merge commit is associated to PRYou attach only the merge commit to the PR. Rebase merges or unmerged PRs may leave commits classified as “direct”. Confirm this is intended for hierarchy mode.
Would you like me to propose logic to associate all PR commits (when available via API) and keep dedupe for direct commits?
248-249: Good termination guard on re-registration recursionRecursing only when progress is made avoids infinite loops; dropping unresolved mappings is correct.
24-27: Import SubIssue to match intended typing
SubIssueRecordexpects aSubIssue. Import it so the factory can construct typed sub-issue records.-from github.Issue import Issue +from github.Issue import Issue, SubIssue
154-159: Materialize SubIssueRecord at discovery time (preserve SubIssue typing)Without creating
SubIssueRecordhere, later code constructs it fromIssue, which can trigger TypeError when accessingSubIssueRecord.issue. Instantiate it now.if len(sub_issues) > 0: self._create_record_for_hierarchy_issue(issue) for si in sub_issues: # register sub-issue and its parent for later hierarchy building self.__sub_issue_parents[si.number] = issue.number # Note: GitHub now allows only 1 parent + # preserve SubIssue typing by creating the record immediately + self._create_record_for_sub_issue(si)
160-166: Do not construct SubIssueRecord from IssueWhen the issue is known to be a sub-issue (present in the mapping), skip creating it from
Issue; it was already created fromSubIssueabove.def _create_issue_record_using_sub_issues_not_existence(self, issue: Issue) -> None: # Expected to run after all issue with sub-issues are registered - if issue.number in self.__sub_issue_parents.keys(): # pylint: disable=consider-iterating-dictionary - self._create_record_for_sub_issue(issue) - else: - self._create_record_for_issue(issue) + if issue.number in self.__sub_issue_parents: + return # already created as SubIssueRecord from a SubIssue + self._create_record_for_issue(issue)
204-212: Tighten _create_record_for_sub_issue signature to SubIssueAligns with
SubIssueRecordexpectations and prevents runtime TypeError when the.issueproperty is accessed.- def _create_record_for_sub_issue(self, issue: Issue, issue_labels: Optional[list[str]] = None) -> None: + def _create_record_for_sub_issue(self, issue: SubIssue, issue_labels: Optional[list[str]] = None) -> None: if issue_labels is None: issue_labels = self._get_issue_labels_mix_with_type(issue) skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) logger.debug("Created record for sub issue %d: %s", issue.number, issue.title) self.__registered_issues.add(issue.number) self._records[issue.number] = SubIssueRecord(issue, issue_labels, skip_record)
miroslavpojer
deleted the
feature/167-Introduce-regime-for-hierarchical-release-notes
branch
1 participant
Release Notes:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores
Tests
Fixes #167
Fixes #81