Skip to content

#149 - Merge labels from PR and the connected issue #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −25
Open

#149 - Merge labels from PR and the connected issue #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
from release_notes_generator.action_inputs import ActionInputs
from release_notes_generator.utils.gh_action import set_action_output
from release_notes_generator.utils.logging_config import setup_logging
from release_notes_generator.filter import FilterByRelease

warnings.filterwarnings("ignore", category=InsecureRequestWarning)

Expand Down
4 changes: 0 additions & 4 deletions release_notes_generator/model/commit_record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ def is_closed(self) -> bool:
def is_open(self) -> bool:
return False

@property
def labels(self):
return []

@property
def authors(self) -> list[str]:
return [self._commit.author.login] if self._commit.author else []
Expand Down
7 changes: 3 additions & 4 deletions release_notes_generator/model/issue_record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def __init__(self, issue: Issue, skip: bool = False):
super().__init__(skip=skip)

self._issue: Issue = issue
self._labels = {label.name for label in self._issue.labels}

self._pull_requests: dict[int, PullRequest] = {}
self._commits: dict[int, dict[str, Commit]] = {}

Expand All @@ -44,10 +46,6 @@ def is_closed(self) -> bool:
def is_open(self) -> bool:
return self._issue.state == self.ISSUE_STATE_OPEN

@property
def labels(self):
return [label.name for label in self._issue.labels]

@property
def authors(self) -> list[str]:
if not self._issue or not self._issue.user:
Expand Down Expand Up @@ -161,6 +159,7 @@ def register_pull_request(self, pull: PullRequest) -> None:
Returns: None
"""
self._pull_requests[pull.number] = pull
self._labels.update({label.name for label in pull.labels})

def register_commit(self, pull: PullRequest, commit: Commit) -> None:
"""
Expand Down
6 changes: 2 additions & 4 deletions release_notes_generator/model/pull_request_record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ def __init__(self, pull: PullRequest, skip: bool = False):
super().__init__(skip=skip)

self._pull_request: PullRequest = pull
self._labels = {label.name for label in self._pull_request.labels}

Comment on lines +28 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

Suggested change
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.

self._commits: dict[str, Commit] = {}

# properties - override Record properties
Expand All @@ -45,10 +47,6 @@ def is_closed(self) -> bool:
def is_open(self) -> bool:
return self._pull_request.state == self.PR_STATE_OPEN

@property
def labels(self):
return [label.name for label in self._pull_request.labels]

@property
def authors(self) -> list[str]:
if not self._pull_request or not self._pull_request.user:
Expand Down
19 changes: 10 additions & 9 deletions release_notes_generator/model/record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def __init__(self, skip: bool = False):
self._present_in_chapters = 0
self._skip = skip
self._is_release_note_detected: Optional[bool] = None
self._labels: set[str] = set()
self._rls_notes: Optional[str] = None # single annotation here

# properties
Expand All @@ -55,6 +56,15 @@ def skip(self) -> bool:
"""Check if the record should be skipped during output generation process."""
return self._skip

@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)

Comment on lines +59 to +67
Copy link

@coderabbitai coderabbitai bot Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miroslavpojer, understood! I'll defer to your judgment on this. The current implementation works fine for your use case.

    🐰
╭─────╮
│ ◕   ◕ │
│   ‿   │
╰─────╯

@property
@abstractmethod
def record_id(self) -> int | str:
Expand Down Expand Up @@ -82,15 +92,6 @@ def is_open(self) -> bool:
bool: True if the record is open, False otherwise.
"""

@property
@abstractmethod
def labels(self) -> list[str]:
"""
Gets the labels of the record.
Returns:
list[str]: A list of labels associated with the record.
"""

@property
@abstractmethod
def authors(self) -> list[str]:
Expand Down
6 changes: 3 additions & 3 deletions tests/release_notes/test_release_notes_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ def test_build_closed_issue_with_user_labels_no_prs(
):
expected_release_notes = RELEASE_NOTES_DATA_CLOSED_ISSUE_NO_PR_WITH_USER_LABELS
rec = record_with_issue_closed_no_pull
rec.issue.labels = [MockLabel("bug"), MockLabel("breaking-changes")]
rec._labels = {"bug", "breaking-changes"}
mocker.patch("release_notes_generator.builder.ActionInputs.get_print_empty_chapters", return_value=False)

builder = ReleaseNotesBuilder(
Expand All @@ -730,7 +730,7 @@ def test_build_closed_issue_with_prs_without_user_label(
):
expected_release_notes = RELEASE_NOTES_DATA_CLOSED_ISSUE_WITH_PR_WITHOUT_USER_LABELS
rec = record_with_issue_closed_two_pulls
rec.issue.labels = [MockLabel("label1"), MockLabel("label2")]
rec._labels = {"label1", "label2"}
rec.issue.title = "I1"
mocker.patch("release_notes_generator.builder.ActionInputs.get_print_empty_chapters", return_value=False)

Expand Down Expand Up @@ -825,7 +825,7 @@ def test_merged_pr_without_issue_with_more_user_labels_duplicity_reduction_on(
):
expected_release_notes = RELEASE_NOTES_DATA_MERGED_PR_WITH_USER_LABELS_DUPLICITY_REDUCTION_ON
rec = pull_request_record_merged
rec.pull_request.labels = [MockLabel("bug"), MockLabel("enhancement")]
rec._labels = {"bug", "enhancement"}
mocker.patch("release_notes_generator.builder.ActionInputs.get_print_empty_chapters", return_value=False)

builder = ReleaseNotesBuilder(
Expand Down
Loading