Feature/55 chapter line formatting authors final #188
Conversation
- Added support for placeholders: author, asignees, developers.
WalkthroughAdds author/assignees/developers placeholders and defaults across docs and action inputs; extends supported row-format keys; refactors Record API from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Inputs as ActionInputs
participant Builder as ReleaseNotesBuilder
participant Record as RecordImpl
rect rgb(240,248,255)
Inputs->>Builder: provide row-format-* (may include {author},{assignees},{developers})
end
Builder->>Record: to_chapter_row(format)
rect rgb(245,255,240)
Note right of Record: RecordImpl builds format values\nincluding author, assignees, developers
Record-->>Builder: formatted chapter row string
end
Builder-->>Inputs: validate placeholders against SUPPORTED_ROW_FORMAT_KEYS (updated)
Builder->>Builder: assemble full release notes output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/conftest.py (2)
150-220: Update fixtures to use shared mock_user for consistency.The fixtures
mock_issue_open,mock_issue_open_2, andmock_issue_closedcreate their ownmock_named_userinstances instead of accepting themock_userfixture parameter. This is inconsistent with other updated fixtures (e.g., lines 224+) that properly use the shared fixture.Apply this pattern to update each fixture:
@pytest.fixture -def mock_issue_open(mocker): +def mock_issue_open(mocker, mock_user): issue = mocker.Mock(spec=Issue) ... - mock_named_user = mocker.Mock(spec=NamedUser) - mock_named_user.login = "user" - issue.assignees = [mock_named_user] + mock_author = deepcopy(mock_user) + mock_author.login = "author" + issue.user = mock_author + + issue.assignees = [mock_user]Repeat for
mock_issue_open_2andmock_issue_closed.
605-643: Add missing user and assignees setup to merged pull request fixtures.The fixtures
mock_pull_merged_with_rls_notes_101andmock_pull_merged_with_rls_notes_102don't set uppull.userorpull.assignees, unlike their "closed" counterparts at lines 549-601. This inconsistency could cause test failures or mask bugs in code that relies on author/assignee data.Update both fixtures to accept
mock_userand set up user relationships:@pytest.fixture -def mock_pull_merged_with_rls_notes_101(mocker): +def mock_pull_merged_with_rls_notes_101(mocker, mock_user): pull = mocker.Mock(spec=PullRequest) pull.state = PullRequestRecord.PR_STATE_CLOSED ... + mock_author = deepcopy(mock_user) + mock_author.login = "pr_author_101" + pull.user = mock_author + + pull.assignees = [mock_user] + label1 = mocker.Mock(spec=MockLabel)Apply the same pattern to
mock_pull_merged_with_rls_notes_102withpr_author_102.
🧹 Nitpick comments (2)
release_notes_generator/action_inputs.py (1)
343-345: Consider handling empty developers list more gracefully.The new default PR format includes
{developers}, which is a helpful addition. However, when the developers list is empty, the output will include "developed by " with no names following it.Consider one of these approaches for better handling of empty developers:
- Use conditional formatting in the template (e.g., only include "developed by" when developers is non-empty)
- Post-process the formatted string to clean up trailing "developed by " phrases
- Document this edge case for users who might encounter it
Example conditional approach:
- ROW_FORMAT_PR, "{number} _{title}_ developed by {developers}" + ROW_FORMAT_PR, "{number} _{title}_" # with dynamic suffix when developers existNote: This is a minor cosmetic issue and can be deferred, as users can always override the format.
action.yml (1)
80-90: Verify placeholder substitution behavior and handle empty lists
{developers},{assignees}, and{pull-requests}use", ".join(...), so multi-item lists render as"@user1, @user2"and single items render correctly.- Empty lists produce
"", leaving surrounding phrases (e.g., "developed by ") blank. Consider conditionally omitting those phrases when the list is empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
README.md(1 hunks)action.yml(1 hunks)release_notes_generator/action_inputs.py(2 hunks)release_notes_generator/builder/builder.py(1 hunks)release_notes_generator/chapters/base_chapters.py(1 hunks)release_notes_generator/chapters/custom_chapters.py(1 hunks)release_notes_generator/chapters/service_chapters.py(1 hunks)release_notes_generator/data/miner.py(1 hunks)release_notes_generator/generator.py(1 hunks)release_notes_generator/model/record/commit_record.py(2 hunks)release_notes_generator/model/record/hierarchy_issue_record.py(3 hunks)release_notes_generator/model/record/issue_record.py(4 hunks)release_notes_generator/model/record/pull_request_record.py(3 hunks)release_notes_generator/model/record/record.py(1 hunks)release_notes_generator/model/record/sub_issue_record.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(1 hunks)release_notes_generator/record/factory/record_factory.py(1 hunks)release_notes_generator/utils/constants.py(1 hunks)tests/conftest.py(31 hunks)tests/release_notes/builder/test_release_notes_builder.py(33 hunks)tests/release_notes/chapters/test_custom_chapters.py(1 hunks)tests/release_notes/model/test_commit_record.py(3 hunks)tests/release_notes/model/test_issue_record.py(1 hunks)tests/release_notes/model/test_pull_request_record.py(1 hunks)tests/release_notes/model/test_record.py(2 hunks)tests/release_notes/record/factory/test_default_record_factory.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (22)
tests/release_notes/chapters/test_custom_chapters.py (3)
release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)release_notes_generator/model/record/record.py (1)
Record(29-240)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
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 (2)
release_notes_generator/model/record/commit_record.py (1)
CommitRecord(13-87)release_notes_generator/model/record/record.py (1)
Record(29-240)
release_notes_generator/builder/builder.py (2)
release_notes_generator/model/record/record.py (1)
Record(29-240)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
release_notes_generator/chapters/service_chapters.py (4)
release_notes_generator/model/record/commit_record.py (1)
CommitRecord(13-87)release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(15-321)release_notes_generator/model/record/record.py (1)
Record(29-240)
tests/release_notes/model/test_record.py (5)
release_notes_generator/model/record/record.py (3)
author(118-124)assignees(128-134)developers(138-145)release_notes_generator/model/record/commit_record.py (3)
author(39-40)assignees(43-45)developers(48-49)release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
release_notes_generator/data/miner.py (2)
release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(15-321)
release_notes_generator/model/record/hierarchy_issue_record.py (2)
release_notes_generator/model/record/issue_record.py (5)
IssueRecord(16-368)developers(70-87)issue(92-97)assignees(61-67)author(55-58)release_notes_generator/model/record/sub_issue_record.py (1)
SubIssueRecord(15-26)
tests/release_notes/model/test_issue_record.py (1)
release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)
release_notes_generator/model/record/record.py (5)
release_notes_generator/model/record/commit_record.py (3)
author(39-40)assignees(43-45)developers(48-49)release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)tests/release_notes/model/test_record.py (3)
author(47-48)assignees(51-52)developers(55-56)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
release_notes_generator/generator.py (2)
release_notes_generator/model/record/record.py (1)
Record(29-240)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
release_notes_generator/chapters/base_chapters.py (2)
release_notes_generator/model/record/record.py (1)
Record(29-240)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
tests/conftest.py (3)
release_notes_generator/model/record/issue_record.py (2)
issue(92-97)assignees(61-67)release_notes_generator/model/record/pull_request_record.py (1)
assignees(55-61)release_notes_generator/model/record/record.py (1)
assignees(128-134)
release_notes_generator/record/factory/default_record_factory.py (8)
release_notes_generator/model/record/commit_record.py (1)
CommitRecord(13-87)release_notes_generator/model/record/hierarchy_issue_record.py (1)
HierarchyIssueRecord(16-196)release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)release_notes_generator/model/mined_data.py (1)
MinedData(38-81)release_notes_generator/action_inputs.py (1)
ActionInputs(64-517)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(15-321)release_notes_generator/model/record/record.py (1)
Record(29-240)release_notes_generator/model/record/sub_issue_record.py (1)
SubIssueRecord(15-26)
release_notes_generator/record/factory/record_factory.py (2)
release_notes_generator/model/record/record.py (1)
Record(29-240)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
release_notes_generator/model/record/pull_request_record.py (4)
release_notes_generator/model/record/record.py (4)
Record(29-240)author(118-124)assignees(128-134)developers(138-145)release_notes_generator/model/record/commit_record.py (4)
author(39-40)assignees(43-45)developers(48-49)commit(54-59)release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
release_notes_generator/model/record/issue_record.py (4)
release_notes_generator/model/record/record.py (4)
Record(29-240)author(118-124)assignees(128-134)developers(138-145)release_notes_generator/model/record/commit_record.py (4)
author(39-40)assignees(43-45)developers(48-49)commit(54-59)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
tests/release_notes/model/test_commit_record.py (2)
release_notes_generator/model/record/commit_record.py (2)
CommitRecord(13-87)author(39-40)tests/conftest.py (1)
mock_commit(717-723)
tests/release_notes/record/factory/test_default_record_factory.py (4)
release_notes_generator/model/record/commit_record.py (1)
CommitRecord(13-87)release_notes_generator/model/record/hierarchy_issue_record.py (1)
HierarchyIssueRecord(16-196)release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(15-321)
tests/release_notes/model/test_pull_request_record.py (1)
release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(15-321)
release_notes_generator/model/record/sub_issue_record.py (1)
release_notes_generator/model/record/issue_record.py (1)
IssueRecord(16-368)
release_notes_generator/model/record/commit_record.py (3)
release_notes_generator/model/record/record.py (4)
Record(29-240)author(118-124)assignees(128-134)developers(138-145)release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)
🪛 Ruff (0.13.3)
release_notes_generator/model/record/issue_record.py
83-83: Loop control variable cid not used within loop body
Rename unused cid to _cid
(B007)
🔇 Additional comments (19)
release_notes_generator/builder/builder.py (1)
27-27: LGTM! Import path correctly updated to nested module structure.The import path refactor aligns with the broader reorganization of record classes under
release_notes_generator.model.record.*.release_notes_generator/data/miner.py (1)
36-38: LGTM! Import paths correctly updated to nested module structure.Both
IssueRecordandPullRequestRecordimports now reference the new nested module locations underrelease_notes_generator.model.record.*, consistent with the broader refactor.tests/release_notes/chapters/test_custom_chapters.py (1)
19-20: LGTM! Test imports correctly updated to match the nested module structure.The import paths for
IssueRecordandRecordnow reference the new module locations underrelease_notes_generator.model.record.*.release_notes_generator/action_inputs.py (1)
329-329: LGTM! Default issue format enhanced with developers information.The new default format includes the
{developers}placeholder, providing useful information about who worked on issues. Users can override this with custom formats if needed.release_notes_generator/chapters/custom_chapters.py (1)
26-27: LGTM! Import paths correctly updated to nested module structure.Both
CommitRecordandRecordimports now reference the new module locations underrelease_notes_generator.model.record.*.tests/release_notes/model/test_pull_request_record.py (1)
19-19: LGTM! Test import correctly updated to nested module structure.The
PullRequestRecordimport now references the new module location underrelease_notes_generator.model.record.*.tests/release_notes/model/test_issue_record.py (1)
22-22: LGTM! Test import correctly updated to nested module structure.The
IssueRecordimport now references the new module location underrelease_notes_generator.model.record.*.release_notes_generator/generator.py (1)
33-33: LGTM! Import path correctly updated to nested module structure.The
Recordimport now references the new module location underrelease_notes_generator.model.record.*, consistent with the broader refactor.tests/release_notes/model/test_commit_record.py (1)
1-57: LGTM!The import path has been correctly updated to the nested module structure, and all test assertions have been properly updated to use the new
authorproperty (string) instead of the oldauthorsproperty (list).README.md (1)
44-63: LGTM!The documentation has been properly updated to reflect the new placeholders (
author,assignees,developers) in the input descriptions and default values. The table formatting improvements enhance readability.release_notes_generator/utils/constants.py (1)
41-43: LGTM!The supported row format keys have been correctly expanded to include
author,assignees, anddevelopersfor all three record types (hierarchy issue, issue, and pull request), aligning with the new API surface.release_notes_generator/model/record/hierarchy_issue_record.py (3)
10-11: LGTM!The import paths have been correctly updated to reference the nested record modules.
60-82: LGTM!The
developersproperty correctly aggregates unique developers from assignees, sub-hierarchy issues, and sub-issues, returning a sorted list. This implementation properly overrides the base class behavior for hierarchical issues.
127-129: LGTM!The format values have been correctly extended to include
author,assignees, anddevelopers, properly joining list values into comma-separated strings for display.release_notes_generator/model/record/issue_record.py (2)
13-13: LGTM!The import path has been correctly updated to reference the nested record module.
239-239: LGTM!The initialization of
self._commits[pull.number] = {}inregister_pull_requestis necessary to ensure that the commits dictionary is properly structured before being accessed in thedevelopersproperty.release_notes_generator/model/record/commit_record.py (2)
10-10: LGTM!The import path has been correctly updated to reference the nested record module.
39-49: LGTM!The new properties correctly implement the base class contract:
authorreturns a string (login or empty string)assigneesreturns an empty list (not applicable for commits)developersreturns a list containing the single authorThis implementation is appropriate for direct commit records.
tests/conftest.py (1)
65-69: LGTM! Centralized mock user fixture improves test maintainability.The new
mock_userfixture provides a reusable foundation for user-related mocks, reducing duplication and making tests more consistent.
| def developers(self) -> list[str]: | ||
| devs = set() | ||
|
|
||
| # Assignees (main implementers) | ||
| for assignee in self.assignees: | ||
| devs.add(f"{assignee}") | ||
|
|
||
| # Linked PR authors (people who created PRs closing this issue) | ||
| for _cid, commit in self._commits.items(): | ||
| if commit.author and getattr(commit.author, "login", None): | ||
| devs.add(f"@{commit.author.login}") | ||
|
|
||
| return sorted(devs) |
There was a problem hiding this comment.
Include the PR author in the developers list.
When a PR has no assignees or registered commits, {developers} renders empty, so the new default templates emit developed by with no names. Adding the PR author avoids that regression.
Apply this diff to ensure the author is captured:
def developers(self) -> list[str]:
devs = set()
+ if self.author:
+ devs.add(self.author)
+
# Assignees (main implementers)
for assignee in self.assignees:
devs.add(f"{assignee}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def developers(self) -> list[str]: | |
| devs = set() | |
| # Assignees (main implementers) | |
| for assignee in self.assignees: | |
| devs.add(f"{assignee}") | |
| # Linked PR authors (people who created PRs closing this issue) | |
| for _cid, commit in self._commits.items(): | |
| if commit.author and getattr(commit.author, "login", None): | |
| devs.add(f"@{commit.author.login}") | |
| return sorted(devs) | |
| def developers(self) -> list[str]: | |
| devs = set() | |
| if self.author: | |
| devs.add(self.author) | |
| # Assignees (main implementers) | |
| for assignee in self.assignees: | |
| devs.add(f"{assignee}") | |
| # Linked PR authors (people who created PRs closing this issue) | |
| for _cid, commit in self._commits.items(): | |
| if commit.author and getattr(commit.author, "login", None): | |
| devs.add(f"@{commit.author.login}") | |
| return sorted(devs) |
🤖 Prompt for AI Agents
In release_notes_generator/model/record/pull_request_record.py around lines 64
to 76, the developers() method currently gathers assignees and commit authors
but omits the PR author, which can leave the developers list empty; update the
method to check self.author (and its login) and add f"@{self.author.login}" to
the devs set (same deduping behavior), ensuring the PR author is included before
returning sorted(devs).
There was a problem hiding this comment.
No. Author does not need to be developer.
There was a problem hiding this comment.
@miroslavpojer, understood. I've noted that the author and developers serve different purposes in your model—the PR author isn't automatically considered a developer.
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#188
File: release_notes_generator/model/record/pull_request_record.py:64-76
Timestamp: 2025-10-07T06:58:23.344Z
Learning: In the release notes generator project (AbsaOSS/generate-release-notes), the `developers` field for pull requests should include only assignees and commit authors, but NOT the PR author. The author and developers are distinct concepts: the author is who opened the PR, while developers are those who implemented the code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
release_notes_generator/model/record/issue_record.py (1)
61-68: Harden against None assignees.Guard iteration in case issue.assignees is None.
Apply this diff:
- for assignee in self.issue.assignees: + for assignee in (self.issue.assignees or []): assignees.add(f"@{assignee.login}")release_notes_generator/model/record/record.py (1)
138-145: Optional doc enhancement: mention commit authors.Developers can include commit authors (e.g., for PRs). Clarify in the docstring.
Apply this diff:
- Getter for the developers of the record. - - assignees - - linked PR authors (if any) + Getter for the developers of the record. + - assignees + - linked PR authors (if any, e.g. for issues) + - commit authors (e.g. for pull requests)Based on learnings
tests/conftest.py (1)
16-16: Optional: avoid duplicate copy imports.Use either
from copy import deepcopyorimport copyconsistently to reduce redundancy.Also applies to: 19-19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
release_notes_generator/model/record/issue_record.py(4 hunks)release_notes_generator/model/record/record.py(1 hunks)tests/conftest.py(33 hunks)tests/release_notes/model/test_record.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T06:58:23.344Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#188
File: release_notes_generator/model/record/pull_request_record.py:64-76
Timestamp: 2025-10-07T06:58:23.344Z
Learning: In the release notes generator project (AbsaOSS/generate-release-notes), the `developers` field for pull requests should include only assignees and commit authors, but NOT the PR author. The author and developers are distinct concepts: the author is who opened the PR, while developers are those who implemented the code.
Applied to files:
release_notes_generator/model/record/record.py
🧬 Code graph analysis (4)
tests/release_notes/model/test_record.py (5)
release_notes_generator/model/record/record.py (4)
Record(29-240)author(118-124)assignees(128-134)developers(138-145)release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)release_notes_generator/model/record/commit_record.py (3)
author(39-40)assignees(43-45)developers(48-49)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
release_notes_generator/model/record/issue_record.py (5)
release_notes_generator/model/record/record.py (4)
Record(29-240)author(118-124)assignees(128-134)developers(138-145)tests/release_notes/model/test_record.py (3)
author(47-48)assignees(51-52)developers(55-56)release_notes_generator/model/record/commit_record.py (4)
author(39-40)assignees(43-45)developers(48-49)commit(54-59)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
tests/conftest.py (4)
release_notes_generator/model/record/commit_record.py (2)
assignees(43-45)get_labels(84-85)release_notes_generator/model/record/issue_record.py (3)
issue(92-97)assignees(61-67)get_labels(110-112)release_notes_generator/model/record/pull_request_record.py (2)
assignees(55-61)get_labels(131-133)release_notes_generator/model/record/record.py (2)
assignees(128-134)get_labels(171-176)
release_notes_generator/model/record/record.py (5)
release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)tests/release_notes/model/test_record.py (3)
author(47-48)assignees(51-52)developers(55-56)release_notes_generator/model/record/commit_record.py (3)
author(39-40)assignees(43-45)developers(48-49)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
🔇 Additional comments (7)
tests/release_notes/model/test_record.py (2)
18-19: Import path update looks good.Consistent with new module layout.
47-56: Properties align with the new Record API.author returns a string; assignees and developers return lists. Good.
release_notes_generator/model/record/issue_record.py (3)
55-59: LGTM: author property.Returns "@login" or empty string as expected.
69-87: LGTM: developers aggregation.Union of assignees + PR authors + commit authors, sorted. Matches intended semantics for issues.
231-240: Good initialization of commit map per PR.Prevents KeyError when iterating commits for registered PRs.
release_notes_generator/model/record/record.py (1)
118-145: Abstract interface update looks solid.Clear separation: author vs assignees vs developers. Good.
tests/conftest.py (1)
65-70: LGTM: mock_user fixture.Typed NamedUser mock with login; used consistently across fixtures.
| format_values["author"] = self.author | ||
| format_values["assignees"] = ", ".join(self.assignees) | ||
| format_values["developers"] = ", ".join(self.developers) |
There was a problem hiding this comment.
Remove duplicate assignment to author in format values.
format_values["author"] is set twice; the second overwrites redundantly.
Apply this diff:
- format_values["author"] = self.author if self.author is not None else ""Also applies to: 147-147
🤖 Prompt for AI Agents
In release_notes_generator/model/record/issue_record.py around lines 139-141 and
also at line 147, there are duplicate assignments to format_values["author"];
remove the redundant assignment(s) so "author" is only set once and leave the
assignments for "assignees" and "developers" intact, ensuring no other logic
depends on the overwritten value.
There was a problem hiding this comment.
@miroslavpojer, thank you for removing the duplicate assignment! The code is now cleaner with author being set only once. 🎉
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#188
File: release_notes_generator/model/record/pull_request_record.py:64-76
Timestamp: 2025-10-07T06:58:23.344Z
Learning: In the release notes generator project (AbsaOSS/generate-release-notes), the `developers` field for pull requests should include only assignees and commit authors, but NOT the PR author. The author and developers are distinct concepts: the author is who opened the PR, while developers are those who implemented the code.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/conftest.py (1)
149-693: Consider adding user/assignees to remaining fixtures for consistency.Most fixtures have been updated to use
mock_userand set author/assignees, which is great. However, a few fixtures still lack these fields:
mock_pull_closed_with_skip_label(line 527)mock_pull_merged_with_rls_notes_101(line 606)mock_pull_merged_with_rls_notes_102(line 627)mock_pull_no_rls_notes(line 702)While the Record implementations are defensive and won't crash if these fields are missing, adding them would improve test consistency and prevent potential issues if these fixtures are used in future tests that check author/assignees/developers properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/model/record/issue_record.py(4 hunks)release_notes_generator/model/record/record.py(1 hunks)tests/conftest.py(36 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T06:58:23.344Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#188
File: release_notes_generator/model/record/pull_request_record.py:64-76
Timestamp: 2025-10-07T06:58:23.344Z
Learning: In the release notes generator project (AbsaOSS/generate-release-notes), the `developers` field for pull requests should include only assignees and commit authors, but NOT the PR author. The author and developers are distinct concepts: the author is who opened the PR, while developers are those who implemented the code.
Applied to files:
release_notes_generator/model/record/record.pyrelease_notes_generator/model/record/issue_record.py
🧬 Code graph analysis (3)
release_notes_generator/model/record/record.py (5)
release_notes_generator/model/record/issue_record.py (3)
author(55-58)assignees(61-67)developers(70-87)tests/release_notes/model/test_record.py (3)
author(47-48)assignees(51-52)developers(55-56)release_notes_generator/model/record/commit_record.py (3)
author(39-40)assignees(43-45)developers(48-49)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
release_notes_generator/model/record/issue_record.py (5)
release_notes_generator/model/record/record.py (4)
Record(29-241)author(118-124)assignees(128-134)developers(138-146)tests/release_notes/model/test_record.py (3)
author(47-48)assignees(51-52)developers(55-56)release_notes_generator/model/record/commit_record.py (4)
author(39-40)assignees(43-45)developers(48-49)commit(54-59)release_notes_generator/model/record/pull_request_record.py (3)
author(49-52)assignees(55-61)developers(64-76)release_notes_generator/model/record/hierarchy_issue_record.py (1)
developers(61-82)
tests/conftest.py (4)
release_notes_generator/model/record/commit_record.py (2)
assignees(43-45)commit(54-59)release_notes_generator/model/record/issue_record.py (2)
issue(92-97)assignees(61-67)release_notes_generator/model/record/pull_request_record.py (1)
assignees(55-61)release_notes_generator/utils/record_utils.py (1)
get_id(24-45)
🔇 Additional comments (3)
release_notes_generator/model/record/issue_record.py (1)
13-13: LGTM! Well-structured implementation of the new author/assignees/developers API.The changes correctly implement the new interface:
- Import path updated to the more specific module location
authorreturns a single string (empty if missing or@loginformat)assigneesaggregates unique assignees with proper sortingdeveloperscorrectly combines assignees, PR authors, and commit authors while excluding the issue author (as per learnings)to_chapter_rowproperly formats the new fields- Commit tracking initialization ensures proper data structure setup
All implementations are defensive against None/missing values, preventing potential AttributeErrors.
Also applies to: 55-87, 139-141, 238-238
release_notes_generator/model/record/record.py (1)
118-146: LGTM! Clear and well-documented abstract interface.The refactoring from
authors(list) toauthor(string) plus the newassigneesanddevelopersproperties provides a cleaner separation of concerns:
author: Who opened the issue/PR (single creator)assignees: Who is assigned to work on itdevelopers: Comprehensive list of implementers (assignees + PR authors + commit authors)The docstrings clearly document the semantics and sources for each field.
tests/conftest.py (1)
64-68: LGTM! Well-designed reusable fixture.The
mock_userfixture provides a clean base for creating user objects across tests, and the pattern of usingdeepcopyto create distinct authors with customized logins is excellent.
miroslavpojer
deleted the
feature/55-Chapter-line-formatting---authors-final
branch
1 participant
Release Notes:
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Fixes #55