#149 - Merge labels from PR and the connected issue #159
Conversation
- Labels are now holded on record itself as set of strings.
WalkthroughConsolidated label handling into the base Record class using an internal set, removed per-record public labels properties, and updated IssueRecord to merge PR labels. Tests now seed labels via internal _labels. Removed an unused import in main.py. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Builder as ReleaseNotesBuilder
participant IssueRec as IssueRecord
participant PRRec as PullRequestRecord
participant Base as Record (base)
User->>Builder: build()
Builder->>IssueRec: load issue
Note over IssueRec: Initialize Base._labels from issue label names
Builder->>PRRec: load PR (optional)
Note over PRRec: Initialize Base._labels from PR label names (internal)
Builder->>IssueRec: register(PRRec)
Note over IssueRec: _labels ← _labels ∪ PRRec label names
Builder->>IssueRec: labels
IssueRec->>Builder: list(_labels)
Builder-->>User: notes categorized by merged labels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release_notes/test_release_notes_builder.py (1)
552-553: Replace stale.issue.labelsmutation with_labels
In tests/release_notes/test_release_notes_builder.py line 552, update:- rec.issue.labels.append(MockLabel("enhancement")) + rec._labels.add("enhancement")No other
.issue.labelsor.pull_request.labelsmutations remain in the tests.
🧹 Nitpick comments (2)
release_notes_generator/model/issue_record.py (1)
30-31: Good: Issue labels cached and PR labels merged into the same set (meets #149). Consider using get_labels() and optional normalization.This implements the objective to merge PR and Issue labels. For SDK compatibility, mirror the
get_labels()approach here as well.Proposed adjustments:
- self._labels = {label.name for label in self._issue.labels} + if hasattr(self._issue, "get_labels"): + self._labels = {label.name for label in self._issue.get_labels()} + else: + self._labels = {label.name for label in getattr(self._issue, "labels", [])}- self._labels.update({label.name for label in pull.labels}) + if hasattr(pull, "get_labels"): + self._labels.update({label.name for label in pull.get_labels()}) + else: + self._labels.update({label.name for label in getattr(pull, "labels", [])})Optionally, normalize names (e.g.,
.lower()) to avoid case-sensitivity mismatches with chapter label keys. I can wire that if desired.Also applies to: 162-162
tests/release_notes/test_release_notes_builder.py (1)
714-714: Tests poke private_labels; prefer updating the existing set rather than rebinding itDirectly assigning a new set replaces the object; using
update()avoids surprises and reads clearer. Longer term, consider a small helper (e.g.,rec.add_labels([...])) to avoid touching a private field from tests.Apply:
- rec._labels = {"bug", "breaking-changes"} + rec._labels.update({"bug", "breaking-changes"})- rec._labels = {"label1", "label2"} + rec._labels.update({"label1", "label2"})- rec._labels = {"bug", "enhancement"} + rec._labels.update({"bug", "enhancement"})I can add a tiny test helper or a public method to seed labels if you want to avoid accessing
_labelsdirectly.Also applies to: 733-733, 828-828
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
main.py(0 hunks)release_notes_generator/model/commit_record.py(0 hunks)release_notes_generator/model/issue_record.py(2 hunks)release_notes_generator/model/pull_request_record.py(1 hunks)release_notes_generator/model/record.py(2 hunks)tests/release_notes/test_release_notes_builder.py(3 hunks)
💤 Files with no reviewable changes (2)
- release_notes_generator/model/commit_record.py
- main.py
🧰 Additional context used
🧬 Code graph analysis (2)
release_notes_generator/model/pull_request_record.py (1)
tests/release_notes/model/test_record.py (1)
labels(41-42)
release_notes_generator/model/issue_record.py (1)
tests/release_notes/model/test_record.py (1)
labels(41-42)
🔇 Additional comments (1)
release_notes_generator/model/record.py (1)
41-41: Good: Centralized label storage on the base classCaching labels as a set here simplifies merging and deduplication.
| self._labels = {label.name for label in self._pull_request.labels} | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initialize labels via PyGithub API method for compatibility and determinism
Prefer get_labels() over the .labels attribute to avoid SDK differences and lazy-loading surprises. This also aligns with how Issue labels are typically fetched.
Proposed change:
- self._labels = {label.name for label in self._pull_request.labels}
+ # Prefer the API method when available; fall back to attribute for mocks/tests
+ if hasattr(self._pull_request, "get_labels"):
+ self._labels = {label.name for label in self._pull_request.get_labels()}
+ else:
+ self._labels = {label.name for label in getattr(self._pull_request, "labels", [])}If you want me to normalize casing (e.g., .lower()) for safer chapter matching, I can add that as well.
To confirm SDK behavior, please check:
🌐 Web query:
Does PyGithub PullRequest expose a `.labels` attribute or should labels be fetched via `get_labels()`?
💡 Result:
Use get_labels(). The official PyGithub PullRequest object exposes a get_labels() method (returns a PaginatedList of Label) and label-modifying methods (add_to_labels, remove_from_labels, set_labels, etc.). While some REPL/examples may show a .labels attribute in certain contexts, the documented/official way is pr.get_labels(). [1][2]
Sources:
- PyGithub PullRequest docs (get_labels, add_to_labels, etc.). [1]
- GitHub issue/example showing .labels in REPL but confirming get_labels() usage. [2]
Use get_labels() for deterministic label retrieval
Replace the .labels attribute with the documented get_labels() API, falling back to the attribute for mocks/tests:
- self._labels = {label.name for label in self._pull_request.labels}
+ # Prefer the API method; fallback for mocks/tests
+ if hasattr(self._pull_request, "get_labels"):
+ self._labels = {label.name for label in self._pull_request.get_labels()}
+ else:
+ self._labels = {label.name for label in getattr(self._pull_request, "labels", [])}📝 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.
| self._labels = {label.name for label in self._pull_request.labels} | |
| # Prefer the API method; fallback for mocks/tests | |
| if hasattr(self._pull_request, "get_labels"): | |
| self._labels = {label.name for label in self._pull_request.get_labels()} | |
| else: | |
| self._labels = {label.name for label in getattr(self._pull_request, "labels", [])} |
🤖 Prompt for AI Agents
In release_notes_generator/model/pull_request_record.py around lines 28-29,
replace direct access to self._pull_request.labels with the documented API: call
self._pull_request.get_labels() to retrieve labels, and fall back to the .labels
attribute for mocks/tests if get_labels is not present; then build the set with
something like {label.name for label in labels_iterable} (ensuring you treat the
result as an iterable) so label retrieval remains deterministic and compatible
with both real objects and test doubles.
| @property | ||
| def labels(self) -> list[str]: | ||
| """ | ||
| Gets the labels of the record. | ||
| Returns: | ||
| list[str]: A list of labels associated with the record. | ||
| """ | ||
| return list(self._labels) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make label exposure deterministic to prevent flaky ordering
Returning a list from a set yields non-deterministic order; sorting avoids flaky tests and unstable output.
Apply:
- return list(self._labels)
+ return sorted(self._labels)Optionally, if you’d like a public helper to simplify tests and avoid touching _labels, consider:
# outside diff – add to Record
def add_labels(self, labels: list[str]) -> None:
self._labels.update(labels)🤖 Prompt for AI Agents
In release_notes_generator/model/record.py around lines 59 to 67, the labels
property returns list(self._labels) which yields non-deterministic ordering;
change the property to return a consistently ordered list by converting the set
to a sorted list (e.g., return sorted(self._labels)); optionally add a public
helper method add_labels(self, labels: list[str]) that updates the internal set
via self._labels.update(labels) to simplify tests without touching the private
attribute.
There was a problem hiding this comment.
This is not needed.
There was a problem hiding this comment.
@miroslavpojer, understood! I'll defer to your judgment on this. The current implementation works fine for your use case.
🐰
╭─────╮
│ ◕ ◕ │
│ ‿ │
╰─────╯
Release Notes:
Fixes #149
Summary by CodeRabbit