Feature/150 refactor generation of rn#151
Conversation
WalkthroughThis update introduces a modular filtering system for release notes generation by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant FilterByRelease
participant ReleaseNotesGenerator
participant RecordFactory
Main->>FilterByRelease: Instantiate filterer
Main->>ReleaseNotesGenerator: generate(filterer)
ReleaseNotesGenerator->>FilterByRelease: filter(data)
FilterByRelease-->>ReleaseNotesGenerator: filtered data
ReleaseNotesGenerator->>RecordFactory: generate(data)
RecordFactory-->>ReleaseNotesGenerator: records
ReleaseNotesGenerator-->>Main: release notes output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
release_notes_generator/model/record.py (1)
1-1: Apply Black formatting.The pipeline indicates that Black formatting is required for this file.
black release_notes_generator/model/record.pyrelease_notes_generator/record/record_factory.py (1)
1-1: Apply Black formatting.The pipeline indicates that Black formatting is required for this file.
black release_notes_generator/record/record_factory.py
🧹 Nitpick comments (8)
.gitignore (1)
161-161: Qualify the path or use a pattern fordefault_output.txt.If the file is always produced in the repository root, prefix it with “/” to avoid accidentally ignoring identically-named files in nested directories.
Alternatively, if multiple output files can be generated, consider a pattern such as*_output.txt.tests/test_filter.py (2)
18-18: Remove unused import.The
pytestimport is not used in this file.-import pytest from unittest.mock import MagicMock
46-47: Fix Yoda conditions for better readability.The comparison order should follow natural language conventions.
-assert 0 == len(mock_log_info.call_args_list) -assert 0 == len(mock_log_debug.call_args_list) +assert len(mock_log_info.call_args_list) == 0 +assert len(mock_log_debug.call_args_list) == 0 -assert ('Starting issue, prs and commit reduction by the latest release since time.',) == mock_log_info.call_args_list[0][0] -assert ('Count of issues reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[0][0] -assert ('Count of pulls reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[1][0] -assert ('Count of commits reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[2][0] +assert mock_log_info.call_args_list[0][0] == ('Starting issue, prs and commit reduction by the latest release since time.',) +assert mock_log_debug.call_args_list[0][0] == ('Count of issues reduced from %d to %d', 2, 1) +assert mock_log_debug.call_args_list[1][0] == ('Count of pulls reduced from %d to %d', 2, 1) +assert mock_log_debug.call_args_list[2][0] == ('Count of commits reduced from %d to %d', 2, 1)Also applies to: 84-87
tests/release_notes/test_record_factory.py (3)
26-26: Fix import path consistency.The import should use the full path like other imports for consistency.
-from model.MinedData import MinedData +from release_notes_generator.model.MinedData import MinedData
187-187: Remove debug print statement.Debug print statements should not be left in production code.
-print(records[1].__class__ ) assert records[1].__class__ is IssueRecord
281-348: Complete the test migration.Two test functions are commented out with TODO notes, indicating incomplete migration. These tests should either be fixed or removed to avoid confusion.
The commented tests suggest there are issues with the new MinedData interface. Would you like me to help fix these tests or should they be removed if they're no longer needed?
release_notes_generator/model/record.py (2)
253-254: Address the TODO comment and verify function usage.The TODO comment suggests that both functions should be called and merged. However, based on the AI summary, the usage should be migrated to
get_issues_for_pr. Please clarify the intended approach.Do you want me to help implement the proper integration of both functions or migrate to the new
get_issues_for_prfunction?
443-475: LGTM! Well-structured inheritance with one naming concern.The new record subclasses properly inherit from the base
Recordclass and specialize for different GitHub entity types. The initialization logic correctly calls the parent constructor and sets the release notes detection flag.Consider using the conventional double underscore for the
___issuesattribute inPullRequestRecord:- self.___issues: Dict[int, List[Issue]] = {} + self.__issues: Dict[int, List[Issue]] = {}Triple underscores are unusual in Python and may cause confusion. Double underscores provide proper name mangling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)main.py(2 hunks)release_notes_generator/action_inputs.py(3 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(4 hunks)release_notes_generator/model/record.py(4 hunks)release_notes_generator/record/record_factory.py(4 hunks)release_notes_generator/utils/constants.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(2 hunks)tests/release_notes/test_record_factory.py(7 hunks)tests/test_filter.py(1 hunks)tests/test_release_notes_generator.py(7 hunks)tests/utils/test_pull_reuqest_utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/utils/test_pull_reuqest_utils.py (1)
release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(31-47)get_issues_for_pr(50-64)
main.py (2)
release_notes_generator/filter.py (1)
FilterByRelease(40-77)release_notes_generator/generator.py (1)
generate(68-98)
release_notes_generator/filter.py (2)
release_notes_generator/model/MinedData.py (1)
MinedData(38-50)release_notes_generator/model/record.py (2)
commits(75-77)issue(65-67)
release_notes_generator/model/record.py (2)
release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(31-47)release_notes_generator/record/record_factory.py (1)
register_pull_request(74-97)
release_notes_generator/action_inputs.py (1)
release_notes_generator/utils/gh_action.py (1)
get_action_input(26-38)
tests/release_notes/test_record_factory.py (5)
release_notes_generator/model/MinedData.py (1)
MinedData(38-50)release_notes_generator/model/record.py (4)
Record(43-441)IssueRecord(456-464)commits(75-77)labels(128-133)tests/conftest.py (1)
mock_repo(78-81)release_notes_generator/record/record_factory.py (2)
RecordFactory(41-138)generate(47-138)release_notes_generator/generator.py (1)
generate(68-98)
release_notes_generator/generator.py (2)
release_notes_generator/filter.py (3)
filter(30-37)filter(48-77)Filter(25-37)release_notes_generator/record/record_factory.py (1)
generate(47-138)
🪛 Ruff (0.11.9)
tests/utils/test_pull_reuqest_utils.py
19-19: release_notes_generator.utils.pull_request_utils.get_issues_for_pr imported but unused
Remove unused import: release_notes_generator.utils.pull_request_utils.get_issues_for_pr
(F401)
release_notes_generator/model/record.py
24-24: typing.AnyStr imported but unused
Remove unused import: typing.AnyStr
(F401)
tests/test_filter.py
18-18: pytest imported but unused
Remove unused import: pytest
(F401)
46-46: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 0
(SIM300)
47-47: Yoda condition detected
Rewrite as len(mock_log_debug.call_args_list) == 0
(SIM300)
84-84: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
86-86: Yoda condition detected
(SIM300)
87-87: Yoda condition detected
(SIM300)
🪛 GitHub Actions: Build and Test
release_notes_generator/utils/pull_request_utils.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
release_notes_generator/filter.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
release_notes_generator/model/record.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
release_notes_generator/action_inputs.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
release_notes_generator/record/record_factory.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
release_notes_generator/generator.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
🔇 Additional comments (12)
release_notes_generator/utils/constants.py (1)
71-84: LGTM! Well-structured constants for GitHub API integration.The new constants are properly designed:
LINKED_ISSUES_MAXprovides a reasonable limit for API callsISSUES_FOR_PRSGraphQL query correctly usesclosingIssuesReferencesto fetch linked issues- The placeholders align with the usage in
pull_request_utils.pytests/utils/test_pull_reuqest_utils.py (1)
19-19: LGTM! Import statement corrected and expanded.The import statement properly fixes the typo from
pull_reuqest_utilstopull_request_utilsand adds the newget_issues_for_prfunction. The unused import warning can be ignored as this is common in test files where imports may be prepared for future tests.main.py (2)
32-32: LGTM! Import added for new filtering mechanism.The import correctly adds
FilterByReleaseto support the new external filtering approach.
55-56: LGTM! Generator updated to use new filtering interface.The changes correctly implement the new filtering mechanism:
- Creates a
FilterByReleaseinstance- Passes it to the
generate()method as required by the new signatureThis aligns with the refactoring to externalize filtering logic from the generator.
tests/test_release_notes_generator.py (3)
26-26: LGTM! Import added for test integration.The import correctly adds
FilterByReleaseto support testing the new filtering mechanism.
42-42: LGTM! Tests updated for new filtering interface.All test cases have been consistently updated to pass
FilterByRelease()instances to thegenerate()method, ensuring compatibility with the new external filtering approach.Also applies to: 77-77, 127-127, 173-173
70-70: LGTM! Proper mocking for external dependencies.The addition of mocking for
get_issues_for_prensures test isolation and prevents external API calls during testing. This is a best practice for unit testing.Also applies to: 113-113, 164-164
release_notes_generator/generator.py (1)
68-98: Excellent architectural improvement.The refactoring to use external filtering via the
Filterabstraction is a great improvement in separation of concerns and modularity. This makes the code more testable and allows for different filtering strategies.release_notes_generator/model/record.py (1)
37-37: No import inconsistency detected in record.pyThe file consistently imports and uses
extract_issue_numbers_from_body—there is no reference toget_issues_for_pr, so no change is needed.• File: release_notes_generator/model/record.py
– Line 37: importsextract_issue_numbers_from_body
– Line 254: callsextract_issue_numbers_from_body(self.__pulls[0])Likely an incorrect or invalid review comment.
release_notes_generator/record/record_factory.py (3)
47-56: LGTM! Excellent refactoring to use MinedData.The method signature change to accept a single
MinedDataobject instead of separate parameters is a clean improvement that enhances data encapsulation and consistency across the codebase.
70-70: LGTM! Proper usage of specialized record classes.The migration from generic
Recordto specializedIssueRecordandPullRequestRecordclasses improves type safety and code clarity. The instantiation logic correctly maintains the existing functionality while leveraging the new class hierarchy.Also applies to: 91-91, 124-124
75-75: LGTM! Consistent migration to new utility function.The change from
extract_issue_numbers_from_bodytoget_issues_for_pris consistent with the refactoring goals and provides better integration with GitHub's GraphQL API for retrieving linked issues.Also applies to: 123-123
.gitignore
Outdated
| .idea/ | ||
|
|
||
| default_output.txt | ||
| run_localy.sh No newline at end of file |
There was a problem hiding this comment.
Typo in ignored script name – likely ineffective rule.
run_localy.sh is probably intended to be run_locally.sh.
Because of the misspelling, the actual helper script will still be tracked (and the stray typo will mask future mistakes).
Fix the entry and keep the incorrect form for backward compatibility if it was already committed before:
-run_localy.sh
+# Correct name
+run_locally.sh
+# Legacy misspelling kept to cover any existing accidental commits
+run_localy.sh📝 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.
| run_localy.sh | |
| # Correct name | |
| run_locally.sh | |
| # Legacy misspelling kept to cover any existing accidental commits | |
| run_localy.sh |
🤖 Prompt for AI Agents
In the .gitignore file at line 162, the script name "run_localy.sh" is
misspelled and should be corrected to "run_locally.sh" to properly ignore the
intended file. Update the entry to include the correct spelling "run_locally.sh"
and retain the incorrect "run_localy.sh" line if it was previously committed,
ensuring backward compatibility while fixing the ignore rule.
There was a problem hiding this comment.
Will be fixed in next commit.
There was a problem hiding this comment.
@miroslavpojer Thank you for confirming! The typo fix will ensure the gitignore rule works as intended.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| ActionInputs._owner, ActionInputs._repo_name = ActionInputs.get_github_repository().split("/") | ||
| else: | ||
| owner = repo_name = "" | ||
| ActionInputs._owner = ActionInputs._repo_name = "" | ||
|
|
||
| if not isinstance(owner, str) or not owner.strip() or not isinstance(repo_name, str) or not repo_name.strip(): | ||
| if not isinstance(ActionInputs._owner, str) or not ActionInputs._owner.strip() or not isinstance(ActionInputs._repo_name, str) or not ActionInputs._repo_name.strip(): | ||
| errors.append("Owner and Repo must be a non-empty string.") |
There was a problem hiding this comment.
Inconsistent repository parsing logic.
The parsing logic differs between the new caching methods and the validation logic. When there's no "/" in the repository ID, the new methods set both owner and repo name to the full repository ID, but validation sets both to empty string.
Apply this diff to make the logic consistent:
- if "/" in repository_id:
- ActionInputs._owner, ActionInputs._repo_name = ActionInputs.get_github_repository().split("/")
- else:
- ActionInputs._owner = ActionInputs._repo_name = ""
+ ActionInputs._owner = ActionInputs.get_github_owner()
+ ActionInputs._repo_name = ActionInputs.get_github_repo_name()📝 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.
| ActionInputs._owner, ActionInputs._repo_name = ActionInputs.get_github_repository().split("/") | |
| else: | |
| owner = repo_name = "" | |
| ActionInputs._owner = ActionInputs._repo_name = "" | |
| if not isinstance(owner, str) or not owner.strip() or not isinstance(repo_name, str) or not repo_name.strip(): | |
| if not isinstance(ActionInputs._owner, str) or not ActionInputs._owner.strip() or not isinstance(ActionInputs._repo_name, str) or not ActionInputs._repo_name.strip(): | |
| errors.append("Owner and Repo must be a non-empty string.") | |
| ActionInputs._owner = ActionInputs.get_github_owner() | |
| ActionInputs._repo_name = ActionInputs.get_github_repo_name() | |
| if not isinstance(ActionInputs._owner, str) or not ActionInputs._owner.strip() or not isinstance(ActionInputs._repo_name, str) or not ActionInputs._repo_name.strip(): | |
| errors.append("Owner and Repo must be a non-empty string.") |
🤖 Prompt for AI Agents
In release_notes_generator/action_inputs.py around lines 335 to 340, the
repository parsing logic is inconsistent: the new caching methods assign both
owner and repo name to the full repository ID if no "/" is present, but the
validation sets both to empty strings. To fix this, update the validation logic
to match the caching methods by assigning both owner and repo name to the full
repository ID string when no "/" is found, ensuring consistent handling across
the code.
There was a problem hiding this comment.
The current code is valid and provide same result in one operation.
There was a problem hiding this comment.
@miroslavpojer You're absolutely right! Looking at it more carefully, the current validation logic is intentionally designed to fail for repository IDs without a "/" separator, which is the correct behavior since GitHub repository identifiers should always be in the format "owner/repo-name".
The validation sets both _owner and _repo_name to empty strings when there's no "/" in the repository ID, which then correctly triggers the validation error "Owner and Repo must be a non-empty string." This is the intended behavior to catch malformed repository inputs.
My suggestion would have incorrectly allowed malformed repository IDs to pass validation. Thank you for the clarification!
🐰 ✨ learns something new
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#151
File: release_notes_generator/action_inputs.py:0-0
Timestamp: 2025-08-06T09:14:33.151Z
Learning: In the ActionInputs class validation logic, repository IDs without "/" separators are intentionally set to empty strings to trigger validation failures, as GitHub repository identifiers must be in "owner/repo-name" format. This is correct behavior and should not be changed to match the caching methods' logic.
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.
| _owner = "" | ||
| _repo_name = "" | ||
|
|
||
| @staticmethod | ||
| def get_github_owner() -> str: | ||
| """ | ||
| Get the GitHub owner from the action inputs. | ||
| """ | ||
| if ActionInputs._owner: | ||
| return ActionInputs._owner | ||
|
|
||
| repository_id = get_action_input(GITHUB_REPOSITORY) or "" | ||
| if "/" in repository_id: | ||
| ActionInputs._owner, _ = repository_id.split("/", 1) | ||
| else: | ||
| ActionInputs._owner = repository_id | ||
|
|
||
| return ActionInputs._owner | ||
|
|
||
| @staticmethod | ||
| def get_github_repo_name() -> str: | ||
| """ | ||
| Get the GitHub repository name from the action inputs. | ||
| """ | ||
| if ActionInputs._repo_name: | ||
| return ActionInputs._repo_name | ||
|
|
||
| repository_id = get_action_input(GITHUB_REPOSITORY) or "" | ||
| if "/" in repository_id: | ||
| _, ActionInputs._repo_name = repository_id.split("/", 1) | ||
| else: | ||
| ActionInputs._repo_name = repository_id | ||
|
|
||
| return ActionInputs._repo_name |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix formatting and address logic inconsistency.
The pipeline shows Black formatting issues that need to be resolved. Additionally, there's an inconsistency in parsing logic:
- In
get_github_owner()andget_github_repo_name(): when no "/" exists, both methods set their respective values to the fullrepository_id - In
validate_inputs()(line 337): when no "/" exists, both_ownerand_repo_nameare set to empty string
Apply this diff to fix the formatting and logic consistency:
- _owner = ""
- _repo_name = ""
+ _owner = ""
+ _repo_name = ""And run Black to fix formatting issues:
🏁 Script executed:
#!/bin/bash
# Fix Black formatting issues
black release_notes_generator/action_inputs.pyLength of output: 144
🏁 Script executed:
#!/bin/bash
# Locate and display the validate_inputs method to verify parsing logic
rg -n "def validate_inputs" -A 5 -B 5 release_notes_generator/action_inputs.pyLength of output: 522
#!/bin/bash
# Display the full validate_inputs method to verify owner/repo parsing logic
rg -n "def validate_inputs" -A 50 release_notes_generator/action_inputs.py🏁 Script executed:
#!/bin/bash
# Extract the complete validate_inputs method to inspect parsing logic
rg -n -C50 "def validate_inputs" release_notes_generator/action_inputs.pyLength of output: 4987
Ensure consistent GitHub owner/repo parsing in validate_inputs and fix formatting
The getters get_github_owner() and get_github_repo_name() fall back to the full repository string when no “/” is present, but validate_inputs() currently:
- Calls a non-existent
get_github_repository()at line 330 - Splits and assigns
_owner/_repo_nameonly when “/” exists, else resets both to"" - This diverges from the getters and will overwrite your cached values
• In release_notes_generator/action_inputs.py, update validate_inputs() (around lines 330–337):
- repository_id = ActionInputs.get_github_repository()
- if not isinstance(repository_id, str) or not repository_id.strip():
- errors.append("Repository ID must be a non-empty string.")
-
- if "/" in repository_id:
- ActionInputs._owner, ActionInputs._repo_name = ActionInputs.get_github_repository().split("/")
- else:
- ActionInputs._owner = ActionInputs._repo_name = ""
+ # Use the new getters for consistent parsing
+ owner = ActionInputs.get_github_owner()
+ repo_name = ActionInputs.get_github_repo_name()
+ repository_id = f"{owner}/{repo_name}" if owner and repo_name else owner or repo_name
+ if not repository_id:
+ errors.append("Repository ID must be a non-empty string.")
+ # Persist the values returned by the getters
+ ActionInputs._owner = owner
+ ActionInputs._repo_name = repo_name• Run Black on release_notes_generator/action_inputs.py (or install/configure Black in CI) to resolve formatting errors.
🤖 Prompt for AI Agents
In release_notes_generator/action_inputs.py around lines 330 to 337, update the
validate_inputs() function to use the existing get_github_owner() and
get_github_repo_name() methods instead of calling the non-existent
get_github_repository(). Ensure that the owner and repo name are assigned
consistently by relying on these getters rather than manually splitting the
repository string. Also, run Black code formatter on the entire file to fix any
formatting issues and maintain consistent style.
There was a problem hiding this comment.
Check again after next commit
There was a problem hiding this comment.
🧩 Analysis chain
I'll check the current state of the code to see what changes have been made.
🏁 Script executed:
#!/bin/bash
# Check recent commits to see if any changes were made
git log --oneline -5
# Check the current state of the validate_inputs method to see if the logic inconsistency was fixed
rg -n -A 20 "def validate_inputs" release_notes_generator/action_inputs.py
# Check if Black formatting issues still exist by running Black in check mode
black --check release_notes_generator/action_inputs.py || echo "Formatting issues detected"
Length of output: 1212
@miroslavpojer I can see that the issues I previously identified are still present in the current code. It looks like no commit has been made yet to address them.
The same problems remain in the validate_inputs() method:
- Logic inconsistency: Lines 330 and 335 still call
get_github_repository()instead of using the newget_github_owner()andget_github_repo_name()methods - Inconsistent parsing behavior: Line 337 sets both
_ownerand_repo_nameto empty string when no "/" is found, but the getter methods would set them to the full repository string - Black formatting issues: The formatting still needs to be addressed
The suggested fix from my previous comment would resolve these issues:
# Replace lines 330-337 in validate_inputs()
# Use the new getters for consistent parsing
owner = ActionInputs.get_github_owner()
repo_name = ActionInputs.get_github_repo_name()
repository_id = f"{owner}/{repo_name}" if owner and repo_name else owner or repo_name
if not repository_id:
errors.append("Repository ID must be a non-empty string.")
# Persist the values returned by the getters
ActionInputs._owner = owner
ActionInputs._repo_name = repo_nameOnce you make these changes and run Black for formatting, I'll be happy to review again! 🐰
/| /|
( :v: )
|(_)|
🧬 Code Graph Analysis Results
Relevant snippets for release_notes_generator/action_inputs.py
File: release_notes_generator/utils/enums.py
Lines: 24-32
class DuplicityScopeEnum(StrEnum):
"""
A class representing the scope where duplicates are allowed.
"""
NONE = "NONE"
CUSTOM = "CUSTOM"
SERVICE = "SERVICE"
BOTH = "BOTH"- Enum used in
ActionInputs.get_duplicity_scope()to validate and represent duplicity scope input.
File: release_notes_generator/utils/gh_action.py
Lines: 26-38
def get_action_input(name: str, default: Optional[str] = None) -> Optional[str]:
"""
Retrieve the value of a specified input parameter from environment variables.
@param name: The name of the input parameter.
@param default: The default value to return if the environment variable is not set.
@return: The value of the specified input parameter, or an empty string if the environment
"""
if default is None:
return os.getenv(f'INPUT_{name.replace("-", "_").upper()}')
return os.getenv(f'INPUT_{name.replace("-", "_").upper()}', default=default)- Function used extensively in
ActionInputsstatic methods to fetch GitHub Action inputs from environment variables.
No other snippets from the provided possibly relevant code snippets directly relate to the action_inputs.py file or the user comment. The base_chapters.py snippet about titles() is unrelated to input handling or validation.
[End of relevant snippets]
| from action_inputs import ActionInputs | ||
| from release_notes_generator.utils.constants import ISSUES_FOR_PRS, LINKED_ISSUES_MAX | ||
|
|
||
| ## todo romove |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the TODO comment and outdated function.
The TODO comment indicates the extract_issue_numbers_from_body function should be removed, but it's still present in the code. If this function is no longer needed, please remove it to avoid confusion.
🤖 Prompt for AI Agents
In release_notes_generator/utils/pull_request_utils.py at line 30, remove the
TODO comment and delete the entire extract_issue_numbers_from_body function if
it is no longer needed, to keep the code clean and avoid confusion.
| @lru_cache(maxsize=None) | ||
| def get_issues_for_pr(pull_number: int) -> list[int]: | ||
| """Update the placeholder values and formate the graphQL query""" | ||
| github_api_url = "https://api.github.com/graphql" | ||
| query = ISSUES_FOR_PRS.format(number=pull_number, | ||
| owner=ActionInputs.get_github_owner(), | ||
| name=ActionInputs.get_github_repo_name(), | ||
| first=LINKED_ISSUES_MAX) | ||
| headers = { | ||
| "Authorization": f"Bearer {ActionInputs.get_github_token()}", | ||
| "Content-Type": "application/json", | ||
| } | ||
| response = requests.post(github_api_url, json={"query": query}, headers=headers) | ||
| response.raise_for_status() # Raise an error for HTTP issues | ||
| numbers = [node['number'] for node in response.json()['data']['repository']['pullRequest']['closingIssuesReferences']['nodes']] | ||
| return numbers | ||
|
|
There was a problem hiding this comment.
Add proper error handling for GraphQL API calls.
The function lacks robust error handling for several potential failure scenarios:
- Invalid pull request number
- GraphQL API returning unexpected structure
- Network failures or API rate limits
Consider adding validation and error handling:
@lru_cache(maxsize=None)
def get_issues_for_pr(pull_number: int) -> list[int]:
"""Update the placeholder values and formate the graphQL query"""
+ if not isinstance(pull_number, int) or pull_number <= 0:
+ raise ValueError(f"Invalid pull request number: {pull_number}")
+
github_api_url = "https://api.github.com/graphql"
query = ISSUES_FOR_PRS.format(number=pull_number,
owner=ActionInputs.get_github_owner(),
name=ActionInputs.get_github_repo_name(),
first=LINKED_ISSUES_MAX)
headers = {
"Authorization": f"Bearer {ActionInputs.get_github_token()}",
"Content-Type": "application/json",
}
- response = requests.post(github_api_url, json={"query": query}, headers=headers)
- response.raise_for_status() # Raise an error for HTTP issues
- numbers = [node['number'] for node in response.json()['data']['repository']['pullRequest']['closingIssuesReferences']['nodes']]
- return numbers
+ try:
+ response = requests.post(github_api_url, json={"query": query}, headers=headers)
+ response.raise_for_status()
+ data = response.json()
+
+ if 'errors' in data:
+ logger.warning("GraphQL errors: %s", data['errors'])
+ return []
+
+ pr_data = data.get('data', {}).get('repository', {}).get('pullRequest')
+ if not pr_data:
+ logger.warning("Pull request %d not found", pull_number)
+ return []
+
+ closing_issues = pr_data.get('closingIssuesReferences', {}).get('nodes', [])
+ return [node['number'] for node in closing_issues if 'number' in node]
+ except requests.exceptions.RequestException as e:
+ logger.error("Failed to fetch issues for PR %d: %s", pull_number, e)
+ return []📝 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.
| @lru_cache(maxsize=None) | |
| def get_issues_for_pr(pull_number: int) -> list[int]: | |
| """Update the placeholder values and formate the graphQL query""" | |
| github_api_url = "https://api.github.com/graphql" | |
| query = ISSUES_FOR_PRS.format(number=pull_number, | |
| owner=ActionInputs.get_github_owner(), | |
| name=ActionInputs.get_github_repo_name(), | |
| first=LINKED_ISSUES_MAX) | |
| headers = { | |
| "Authorization": f"Bearer {ActionInputs.get_github_token()}", | |
| "Content-Type": "application/json", | |
| } | |
| response = requests.post(github_api_url, json={"query": query}, headers=headers) | |
| response.raise_for_status() # Raise an error for HTTP issues | |
| numbers = [node['number'] for node in response.json()['data']['repository']['pullRequest']['closingIssuesReferences']['nodes']] | |
| return numbers | |
| @lru_cache(maxsize=None) | |
| def get_issues_for_pr(pull_number: int) -> list[int]: | |
| """Update the placeholder values and formate the graphQL query""" | |
| if not isinstance(pull_number, int) or pull_number <= 0: | |
| raise ValueError(f"Invalid pull request number: {pull_number}") | |
| github_api_url = "https://api.github.com/graphql" | |
| query = ISSUES_FOR_PRS.format( | |
| number=pull_number, | |
| owner=ActionInputs.get_github_owner(), | |
| name=ActionInputs.get_github_repo_name(), | |
| first=LINKED_ISSUES_MAX, | |
| ) | |
| headers = { | |
| "Authorization": f"Bearer {ActionInputs.get_github_token()}", | |
| "Content-Type": "application/json", | |
| } | |
| try: | |
| response = requests.post(github_api_url, json={"query": query}, headers=headers) | |
| response.raise_for_status() | |
| data = response.json() | |
| if "errors" in data: | |
| logger.warning("GraphQL errors: %s", data["errors"]) | |
| return [] | |
| pr_data = data.get("data", {}).get("repository", {}).get("pullRequest") | |
| if not pr_data: | |
| logger.warning("Pull request %d not found", pull_number) | |
| return [] | |
| closing_issues = pr_data.get("closingIssuesReferences", {}).get("nodes", []) | |
| return [node["number"] for node in closing_issues if "number" in node] | |
| except requests.exceptions.RequestException as e: | |
| logger.error("Failed to fetch issues for PR %d: %s", pull_number, e) | |
| return [] |
🤖 Prompt for AI Agents
In release_notes_generator/utils/pull_request_utils.py around lines 49 to 65,
the function get_issues_for_pr lacks error handling for invalid pull request
numbers, unexpected GraphQL response structures, and network or API errors. Add
try-except blocks around the API call and JSON parsing to catch requests
exceptions and KeyError or TypeError for unexpected data. Validate the response
structure before accessing nested keys and handle cases where the pull request
or issues data is missing. Log or raise informative errors to make failures
clear and prevent unhandled exceptions.
| def __init__(self, release_version: Optional[str] = None): | ||
| self.release_version = release_version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused constructor parameter.
The release_version parameter is not used anywhere in the FilterByRelease class. Either implement its usage or remove it to avoid confusion.
-def __init__(self, release_version: Optional[str] = None):
- self.release_version = release_version
+def __init__(self):
+ pass📝 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 __init__(self, release_version: Optional[str] = None): | |
| self.release_version = release_version | |
| def __init__(self): | |
| pass |
🤖 Prompt for AI Agents
In release_notes_generator/filter.py around lines 45 to 46, the constructor of
the FilterByRelease class has an unused parameter release_version. To fix this,
remove the release_version parameter from the __init__ method and also remove
the assignment to self.release_version to avoid confusion and keep the code
clean.
There was a problem hiding this comment.
I will check later, added TODO
There was a problem hiding this comment.
@miroslavpojer That sounds good! Taking time to review the unused parameter and adding a TODO is a reasonable approach. The suggestion will be there when you're ready to address it in a future iteration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
release_notes_generator/model/record.py (1)
24-24: Remove unused import.The
AnyStrimport is not used in the file and should be removed.
🧹 Nitpick comments (3)
release_notes_generator/model/record.py (1)
453-453: Consider lazy evaluation for release notes detection.The constructors call
self.contains_release_noteswhich performs regex parsing of PR bodies. This could impact performance if many record objects are created. Consider lazy evaluation or caching this computation.Also applies to: 465-465, 477-477
tests/test_filter.py (2)
18-18: Remove unused import.The
pytestimport is not used in the file. Themockerfixture is used but doesn't require importingpytestdirectly.-import pytest from unittest.mock import MagicMock from datetime import datetime
46-47: Consider using conventional condition order.The assertions use "Yoda conditions" (literal on the left). While not incorrect, Python convention typically puts the variable on the left.
- assert 0 == len(mock_log_info.call_args_list) - assert 0 == len(mock_log_debug.call_args_list) + assert len(mock_log_info.call_args_list) == 0 + assert len(mock_log_debug.call_args_list) == 0Apply similar changes to lines 84-87.
Also applies to: 84-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)DEVELOPER.md(1 hunks)release_notes_generator/action_inputs.py(3 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(2 hunks)release_notes_generator/model/record.py(4 hunks)release_notes_generator/record/record_factory.py(4 hunks)release_notes_generator/utils/pull_request_utils.py(2 hunks)tests/test_filter.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- DEVELOPER.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .gitignore
- release_notes_generator/utils/pull_request_utils.py
- release_notes_generator/action_inputs.py
- release_notes_generator/filter.py
- release_notes_generator/record/record_factory.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
release_notes_generator/model/record.py (2)
release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-48)release_notes_generator/record/record_factory.py (1)
register_pull_request(72-95)
tests/test_filter.py (2)
release_notes_generator/filter.py (3)
filter(31-38)filter(51-85)FilterByRelease(41-85)release_notes_generator/model/MinedData.py (1)
MinedData(38-50)
🪛 Ruff (0.12.2)
release_notes_generator/model/record.py
24-24: typing.AnyStr imported but unused
Remove unused import: typing.AnyStr
(F401)
tests/test_filter.py
18-18: pytest imported but unused
Remove unused import: pytest
(F401)
46-46: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 0
(SIM300)
47-47: Yoda condition detected
Rewrite as len(mock_log_debug.call_args_list) == 0
(SIM300)
84-84: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
86-86: Yoda condition detected
(SIM300)
87-87: Yoda condition detected
(SIM300)
🔇 Additional comments (6)
release_notes_generator/model/record.py (2)
37-37: LGTM!The import is correctly used in line 254 within the
pr_contains_issue_mentionsproperty.
474-477: Review the CommitRecord initialization logic.The
CommitRecordcallsself.register_commit(commit)in its constructor, but theregister_commitmethod expects the commit to match an existing pull request'smerge_commit_sha. For a standalone commit record, this will likely fail and log an error since no pull requests are registered yet.Consider whether this is the intended behavior or if
CommitRecordneeds a different initialization approach for handling direct commits.release_notes_generator/generator.py (3)
68-68: LGTM!The dependency injection approach for the filterer parameter improves testability and flexibility.
82-82: LGTM!The code correctly uses the return value from the filter method, addressing the previous review feedback about properly using the filtered data.
84-90: LGTM!The consistent use of
filtered_datathroughout the method and the simplifiedRecordFactory.generatecall align well with the refactoring objectives.tests/test_filter.py (1)
27-88: LGTM!The test cases provide good coverage of the filtering functionality:
- Testing the no-release scenario where no filtering should occur
- Testing the with-release scenario with proper date-based filtering
- Verifying logging behavior in both cases
The mock setup and assertions are appropriate and align with the FilterByRelease implementation.
Fixed unit tests. Fixed mypy.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
release_notes_generator/model/record.py (2)
24-24: Remove unused importAnyStr.The
AnyStrimport is not used in the file and should be removed.-from typing import Optional, AnyStr, Any, Dict, List +from typing import Optional, Any, Dict, List
458-458: Fix unconventional triple underscore naming.The attribute
___issuesuses triple underscores, which is unconventional. Use single or double underscore following Python conventions.- self.___issues: Dict[int, List[Issue]] = {} + self._issues: Dict[int, List[Issue]] = {}
🧹 Nitpick comments (8)
release_notes_generator/model/custom_chapters.py (1)
41-62: Approve the refactoring but simplify nested conditions.The refactoring to iterate over items and use local variables is excellent - it's more efficient and readable. The CommitRecord skip logic is appropriate since commits don't have labels.
However, consider combining the nested if statements on lines 58-59 as suggested by static analysis:
- if record_label in ch.labels and records[id].pulls_count > 0: - if not records[id].is_present_in_chapters: + if record_label in ch.labels and record.pulls_count > 0 and not record.is_present_in_chapters:Note: Also use the local
recordvariable instead ofrecords[id]for consistency.tests/test_filter.py (3)
18-18: Remove unused import.The
pytestimport is not used in this test file and should be removed.-import pytest
27-50: LGTM! Comprehensive test for no-release scenario.The test correctly validates that no filtering occurs when release is None and verifies that no logging calls are made.
Consider fixing the Yoda conditions for better readability:
- assert 0 == len(mock_log_info.call_args_list) - assert 0 == len(mock_log_debug.call_args_list) + assert len(mock_log_info.call_args_list) == 0 + assert len(mock_log_debug.call_args_list) == 0
52-90: LGTM! Thorough test for release filtering scenario.The test properly validates the filtering logic and logging behavior with appropriate mock data and assertions.
Consider fixing the Yoda conditions for better readability:
- assert ('Starting issue, prs and commit reduction by the latest release since time.',) == mock_log_info.call_args_list[0][0] - assert ('Count of issues reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[0][0] - assert ('Count of pulls reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[1][0] - assert ('Count of commits reduced from %d to %d', 2, 1) == mock_log_debug.call_args_list[2][0] + assert mock_log_info.call_args_list[0][0] == ('Starting issue, prs and commit reduction by the latest release since time.',) + assert mock_log_debug.call_args_list[0][0] == ('Count of issues reduced from %d to %d', 2, 1) + assert mock_log_debug.call_args_list[1][0] == ('Count of pulls reduced from %d to %d', 2, 1) + assert mock_log_debug.call_args_list[2][0] == ('Count of commits reduced from %d to %d', 2, 1)tests/release_notes/test_record_factory.py (3)
200-204: Fix Yoda conditions for consistency.Rewrite the assertions to follow standard comparison order.
- assert 1 == records[1].pulls_count + assert records[1].pulls_count == 1 assert pr1 == records[1].pull_request(0) # Verify that commits are registered - assert 1 == records[1].pull_request_commit_count(101) + assert records[1].pull_request_commit_count(101) == 1
239-247: Fix Yoda conditions for consistency.Rewrite the assertions to follow standard comparison order.
# Verify that PRs are registered - assert 1 == records[1].pulls_count - assert 1 == records[2].pulls_count + assert records[1].pulls_count == 1 + assert records[2].pulls_count == 1 assert pr1 == records[1].pull_request(0) assert pr2 == records[2].pull_request(0) # Verify that commits are registered - assert 1 == records[1].pull_request_commit_count(101) - assert 1 == records[2].pull_request_commit_count(102) + assert records[1].pull_request_commit_count(101) == 1 + assert records[2].pull_request_commit_count(102) == 1
274-281: Fix Yoda conditions for consistency.Rewrite the assertions to follow standard comparison order.
# Verify that PRs are registered - assert 0 == records[1].pulls_count - assert 1 == records[2].pulls_count + assert records[1].pulls_count == 0 + assert records[2].pulls_count == 1 assert pr1 == records[2].pull_request(0) # Verify that commits not present - assert 0 == records[1].pull_request_commit_count(1) - assert 0 == records[2].pull_request_commit_count(2) + assert records[1].pull_request_commit_count(1) == 0 + assert records[2].pull_request_commit_count(2) == 0release_notes_generator/model/record.py (1)
431-435: Simplify with built-inany()function.Use Python's built-in
any()for more concise and idiomatic code.- for pull in self._pulls: - if pull.merge_commit_sha == sha: - return True - - return False + return any(pull.merge_commit_sha == sha for pull in self._pulls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
main.py(3 hunks)release_notes_generator/builder.py(1 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(2 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/base_chapters.py(2 hunks)release_notes_generator/model/chapter.py(1 hunks)release_notes_generator/model/custom_chapters.py(2 hunks)release_notes_generator/model/record.py(14 hunks)release_notes_generator/model/service_chapters.py(6 hunks)release_notes_generator/record/record_factory.py(4 hunks)release_notes_generator/utils/constants.py(1 hunks)release_notes_generator/utils/github_rate_limiter.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(2 hunks)tests/release_notes/test_record_factory.py(6 hunks)tests/release_notes/test_release_notes_builder.py(1 hunks)tests/test_filter.py(1 hunks)tests/test_miner.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/release_notes/test_release_notes_builder.py
- tests/test_miner.py
- release_notes_generator/model/base_chapters.py
- release_notes_generator/utils/github_rate_limiter.py
🚧 Files skipped from review as they are similar to previous changes (6)
- release_notes_generator/utils/pull_request_utils.py
- release_notes_generator/generator.py
- release_notes_generator/record/record_factory.py
- release_notes_generator/utils/constants.py
- main.py
- release_notes_generator/filter.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
release_notes_generator/miner.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-50)
release_notes_generator/model/chapter.py (1)
release_notes_generator/model/base_chapters.py (1)
add_row(46-57)
tests/release_notes/test_record_factory.py (5)
release_notes_generator/model/mined_data.py (1)
MinedData(38-50)release_notes_generator/model/record.py (7)
Record(43-445)IssueRecord(461-469)PullRequestRecord(448-458)commits(76-78)skip(86-88)pull_request(307-316)labels(129-134)tests/conftest.py (2)
mock_repo(78-81)MockLabel(43-45)release_notes_generator/record/record_factory.py (2)
RecordFactory(40-147)generate(46-147)tests/release_notes/test_release_notes_builder.py (1)
MockLabel(75-77)
release_notes_generator/builder.py (2)
release_notes_generator/model/record.py (1)
Record(43-445)tests/release_notes/model/test_base_chapters.py (1)
Record(22-23)
tests/test_filter.py (2)
release_notes_generator/filter.py (3)
filter(32-39)filter(50-102)FilterByRelease(42-102)release_notes_generator/model/mined_data.py (1)
MinedData(38-50)
🪛 Ruff (0.12.2)
release_notes_generator/model/custom_chapters.py
58-59: Use a single if statement instead of nested if statements
(SIM102)
tests/release_notes/test_record_factory.py
200-200: Yoda condition detected
Rewrite as records[1].pulls_count == 1
(SIM300)
204-204: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(101) == 1
(SIM300)
239-239: Yoda condition detected
Rewrite as records[1].pulls_count == 1
(SIM300)
240-240: Yoda condition detected
Rewrite as records[2].pulls_count == 1
(SIM300)
246-246: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(101) == 1
(SIM300)
247-247: Yoda condition detected
Rewrite as records[2].pull_request_commit_count(102) == 1
(SIM300)
274-274: Yoda condition detected
Rewrite as records[1].pulls_count == 0
(SIM300)
275-275: Yoda condition detected
Rewrite as records[2].pulls_count == 1
(SIM300)
280-280: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(1) == 0
(SIM300)
281-281: Yoda condition detected
Rewrite as records[2].pull_request_commit_count(2) == 0
(SIM300)
release_notes_generator/model/record.py
24-24: typing.AnyStr imported but unused
Remove unused import: typing.AnyStr
(F401)
431-435: Use return any(pull.merge_commit_sha == sha for pull in self._pulls) instead of for loop
Replace with return any(pull.merge_commit_sha == sha for pull in self._pulls)
(SIM110)
tests/test_filter.py
18-18: pytest imported but unused
Remove unused import: pytest
(F401)
48-48: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 0
(SIM300)
49-49: Yoda condition detected
Rewrite as len(mock_log_debug.call_args_list) == 0
(SIM300)
87-87: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
90-90: Yoda condition detected
(SIM300)
🪛 GitHub Actions: Build and Test
release_notes_generator/model/record.py
[error] 356-356: SyntaxError: f-string expression part cannot include a backslash. The f-string at line 356 contains a backslash in the expression which is invalid syntax.
🔇 Additional comments (18)
release_notes_generator/miner.py (1)
32-32: LGTM! Import path updated to follow Python naming conventions.The change from
release_notes_generator.model.MinedDatatorelease_notes_generator.model.mined_dataaligns with PEP 8 guidelines for module naming (lowercase with underscores) and maintains consistency across the codebase.release_notes_generator/builder.py (1)
42-42: LGTM! Type annotation change supports expanded record key types.The change from
dict[int, Record]todict[int | str, Record]aligns with the broader refactoring to support both integer keys (for issues/PRs) and string keys (for commit SHAs). This maintains type safety while enabling the new functionality.release_notes_generator/model/chapter.py (2)
33-33: LGTM! Type annotation updated for flexible key types.The change from
dict[int, str]todict[int | str, str]properly supports both integer and string keys in the rows dictionary.
36-44: LGTM! Method signature updated consistently.The parameter rename from
numbertoidwith typeint | stris more semantically appropriate, and the docstring correctly reflects that it can be either a number or SHA. The implementation correctly uses the new parameter name.release_notes_generator/model/custom_chapters.py (2)
25-25: LGTM! Import added for new record type handling.The
CommitRecordimport is necessary for the instanceof check in the populate method.
34-34: LGTM! Type annotation updated for flexible record keys.The change to
dict[int | str, Record]maintains consistency with the broader refactoring to support both integer and string record identifiers.release_notes_generator/model/service_chapters.py (6)
26-26: LGTM! Import added for commit record handling.The
CommitRecordimport is necessary for the new direct commit functionality.
34-34: LGTM! Type annotations updated consistently.All type annotations have been properly updated to support
int | strfor record identifiers, maintaining consistency with the broader refactoring.Also applies to: 50-50, 56-56
78-81: LGTM! New direct commits chapter added.The new
DIRECT_COMMITSchapter with appropriate empty message provides visibility into commits not linked to pull requests.
93-127: LGTM! Population logic updated with new commit handling.The refactoring to iterate over items is efficient, and the new
CommitRecordhandling correctly routes direct commits to the appropriate chapter. The type signature updates are consistent throughout.
216-224: LGTM! Clean implementation of direct commit population.The new
__populate_direct_commitmethod is simple, focused, and follows the same pattern as other population methods.
128-234: LGTM! All method signatures updated consistently.All private methods have been properly updated to accept
int | stridentifiers, maintaining type consistency throughout the class.tests/release_notes/test_record_factory.py (4)
26-27: LGTM!The imports correctly reflect the refactored architecture using
MinedDataand specific record subclasses.
157-169: LGTM!The mock helper functions correctly simulate the
get_issues_for_prbehavior for testing purposes.
283-292: LGTM!The mock helper functions correctly simulate the scenario where no issues are linked to pull requests.
379-417: LGTM!The mock helper functions and test implementation correctly validate the edge case where pull requests reference non-existent issue numbers. The tests comprehensively cover the refactored
RecordFactory.generatemethod.release_notes_generator/model/record.py (2)
49-56: Good refactoring of attribute visibility.Changing from name-mangled (
__) to protected (_) attributes is appropriate since these are accessed by subclasses. This improves code maintainability.
448-482: Well-structured record subclasses.The introduction of
PullRequestRecord,IssueRecord, andCommitRecordsubclasses provides clear separation of concerns and type safety. This is a good architectural improvement.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
release_notes_generator/model/record.py (2)
24-24: Remove unused import.The
AnyStrimport is not used in the file and should be removed.-from typing import Optional, AnyStr, Any, Dict, List +from typing import Optional, Any, Dict, List
459-459: Fix the attribute naming convention.The attribute
___issuesuses triple underscores, which is unconventional. Python typically uses single underscore for protected attributes or double underscore for name mangling.- self.___issues: Dict[int, List[Issue]] = {} + self._issues: Dict[int, List[Issue]] = {}
🧹 Nitpick comments (1)
release_notes_generator/model/record.py (1)
432-436: Optimize with generator expression andany()function.The current for loop can be simplified using a more Pythonic approach.
- def is_commit_sha_present(self, sha: str) -> bool: - """ - Checks if the specified commit SHA is present in the record. - - @param sha: The commit SHA to check for. - @return: A boolean indicating whether the specified commit SHA is present in the record. - """ - for pull in self._pulls: - if pull.merge_commit_sha == sha: - return True - - return False + def is_commit_sha_present(self, sha: str) -> bool: + """ + Checks if the specified commit SHA is present in the record. + + @param sha: The commit SHA to check for. + @return: A boolean indicating whether the specified commit SHA is present in the record. + """ + return any(pull.merge_commit_sha == sha for pull in self._pulls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
release_notes_generator/model/record.py(14 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
release_notes_generator/model/record.py
24-24: typing.AnyStr imported but unused
Remove unused import: typing.AnyStr
(F401)
432-436: Use return any(pull.merge_commit_sha == sha for pull in self._pulls) instead of for loop
Replace with return any(pull.merge_commit_sha == sha for pull in self._pulls)
(SIM110)
🔇 Additional comments (6)
release_notes_generator/model/record.py (6)
37-37: Good fix for the import path.The correction from
pull_reuqest_utilstopull_request_utilsfixes the typo in the module name.
49-56: Excellent refactoring from name-mangled to protected attributes.The change from double-underscore (name-mangled private) to single-underscore (protected) attributes is a good improvement. This follows Python conventions better and makes the code more maintainable while still indicating these are internal attributes.
349-349: Good guidance for future development.The TODO comment encouraging child classes to override this method is excellent guidance for improving the design pattern.
355-357: Correct handling of CommitRecord and f-string syntax.The special case for
CommitRecordis well implemented, and the commit message processing is correctly moved outside the f-string to avoid the syntax error flagged in previous reviews.
462-471: Well-designed IssueRecord subclass.Clean and focused subclass for issue records with appropriate initialization and release note detection.
473-482: Good design for CommitRecord subclass.The CommitRecord subclass appropriately handles direct commits by using key 0 as a placeholder and correctly sets the release note detection to False, which makes sense for direct commits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
release_notes_generator/model/custom_chapters.py (1)
57-61: Consider simplifying nested if conditions.The static analysis tool suggests combining the nested if statements for better readability.
- for record_label in records[record_id].labels: # iterate all labels of the record (issue, or 1st PR) - if record_label in ch.labels and records[record_id].pulls_count > 0: - if not records[record_id].is_present_in_chapters: - ch.add_row(record_id, records[record_id].to_chapter_row()) - self.populated_record_numbers_list.append(record_id) + for record_label in record.labels: # iterate all labels of the record (issue, or 1st PR) + if record_label in ch.labels and record.pulls_count > 0 and not record.is_present_in_chapters: + ch.add_row(record_id, record.to_chapter_row()) + self.populated_record_numbers_list.append(record_id)tests/release_notes/test_record_factory.py (1)
199-280: Consider fixing Yoda conditions for better readability.The static analysis tool correctly identifies multiple Yoda conditions that could be rewritten for better readability.
For example, lines 199 and 203 could be rewritten as:
- assert 1 == records[1].pulls_count + assert records[1].pulls_count == 1 - assert 1 == records[1].pull_request_commit_count(101) + assert records[1].pull_request_commit_count(101) == 1Similar changes should be applied to other Yoda conditions throughout the test file for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.pylintrc(1 hunks)main.py(3 hunks)release_notes_generator/action_inputs.py(4 hunks)release_notes_generator/builder.py(1 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/miner.py(2 hunks)release_notes_generator/model/chapter.py(1 hunks)release_notes_generator/model/custom_chapters.py(2 hunks)release_notes_generator/model/mined_data.py(1 hunks)release_notes_generator/model/record.py(14 hunks)release_notes_generator/model/service_chapters.py(6 hunks)release_notes_generator/record/record_factory.py(3 hunks)release_notes_generator/utils/github_rate_limiter.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(2 hunks)tests/conftest.py(0 hunks)tests/release_notes/test_record_factory.py(6 hunks)
💤 Files with no reviewable changes (1)
- tests/conftest.py
✅ Files skipped from review due to trivial changes (1)
- release_notes_generator/model/mined_data.py
🚧 Files skipped from review as they are similar to previous changes (10)
- release_notes_generator/miner.py
- release_notes_generator/builder.py
- release_notes_generator/model/chapter.py
- release_notes_generator/action_inputs.py
- release_notes_generator/filter.py
- release_notes_generator/utils/github_rate_limiter.py
- main.py
- release_notes_generator/model/service_chapters.py
- release_notes_generator/model/record.py
- release_notes_generator/utils/pull_request_utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/release_notes/test_record_factory.py (5)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/record.py (9)
Record(43-442)IssueRecord(458-466)PullRequestRecord(445-455)commits(76-78)skip(86-88)pulls_count(248-250)pull_request(308-317)pull_request_commit_count(291-306)labels(129-134)tests/conftest.py (2)
mock_repo(77-80)MockLabel(42-44)release_notes_generator/record/record_factory.py (2)
RecordFactory(40-146)generate(46-146)tests/release_notes/test_release_notes_builder.py (1)
MockLabel(75-77)
🪛 Ruff (0.12.2)
tests/release_notes/test_record_factory.py
199-199: Yoda condition detected
Rewrite as records[1].pulls_count == 1
(SIM300)
203-203: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(101) == 1
(SIM300)
238-238: Yoda condition detected
Rewrite as records[1].pulls_count == 1
(SIM300)
239-239: Yoda condition detected
Rewrite as records[2].pulls_count == 1
(SIM300)
245-245: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(101) == 1
(SIM300)
246-246: Yoda condition detected
Rewrite as records[2].pull_request_commit_count(102) == 1
(SIM300)
273-273: Yoda condition detected
Rewrite as records[1].pulls_count == 0
(SIM300)
274-274: Yoda condition detected
Rewrite as records[2].pulls_count == 1
(SIM300)
279-279: Yoda condition detected
Rewrite as records[1].pull_request_commit_count(1) == 0
(SIM300)
280-280: Yoda condition detected
Rewrite as records[2].pull_request_commit_count(2) == 0
(SIM300)
release_notes_generator/model/custom_chapters.py
58-59: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (16)
.pylintrc (1)
325-325: LGTM! Configuration change aligns with architectural refactoring.Lowering the minimum public methods threshold from 2 to 1 is appropriate given the introduction of new focused classes like
Filter,FilterByRelease, and record subclasses that may legitimately have single public methods. This prevents unnecessary linter warnings for well-designed single-purpose classes.release_notes_generator/model/custom_chapters.py (3)
25-25: LGTM: Import updated for new record subclass.The import correctly includes the new
CommitRecordclass that's being handled in the populate method.
34-34: LGTM: Method signature updated for broader key types.The type annotation correctly reflects that records can now be keyed by both integers (issue/PR numbers) and strings (commit SHAs).
41-48: LGTM: Improved iteration and filtering logic.The changes properly handle the new data structure by iterating over items instead of keys and correctly skip CommitRecord instances since they don't have labels for chapter assignment.
release_notes_generator/record/record_factory.py (7)
28-34: LGTM: Imports updated for refactored dependencies.The imports correctly reflect the new MinedData structure and specific record subclasses being used in the refactored generate method.
46-57: LGTM: Method signature refactored for consolidated data input.The method now accepts a single
MinedDataobject instead of separate parameters, which is a cleaner API design. The documentation and return type annotation are properly updated to reflect the newint | strkeys.
69-69: LGTM: Using specific record subclass.The change from generic
RecordtoIssueRecordprovides better type safety and clearer intent.
74-84: LGTM: Improved issue retrieval using GraphQL API.The switch from parsing PR body text to using the
get_issues_for_prfunction that queries the GitHub GraphQL API is a more reliable approach for finding linked issues.
92-92: LGTM: Using specific record subclass for pull requests.Consistent with the issue record changes, using
PullRequestRecordprovides better type safety.
112-114: LGTM: Enhanced commit handling with dedicated record type.The creation of
CommitRecordfor direct commits (not linked to issues/PRs) is a good addition that provides better tracking and categorization of all commits.
118-145: LGTM: Well-structured data processing with improved logging.The refactored logic properly processes the MinedData structure and provides clear logging at each stage. The commit detection and counting logic is well-implemented.
tests/release_notes/test_record_factory.py (5)
26-27: LGTM: Imports updated for refactored dependencies.The imports correctly include the new
MinedDataclass and specific record subclasses needed for the updated tests.
157-169: LGTM: Well-structured mock functions for new API.The mock functions properly simulate the behavior of the new
get_issues_for_prfunction and safe_call_decorator integration.
171-189: LGTM: Test updated for MinedData structure.The test properly constructs a MinedData object and includes appropriate rate limiting mocks for the GitHub API interactions.
196-196: LGTM: Assertion updated for specific record type.The assertion correctly checks for the specific
IssueRecordsubclass instead of the genericRecordclass.
282-391: LGTM: Comprehensive mock scenarios.The additional mock functions cover different scenarios (no issues, wrong issue numbers) which provide good test coverage for edge cases in the refactored code.
miroslavpojer
left a comment
There was a problem hiding this comment.
As no one can review. I am approving myself after being overtaken by Olivie.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Documentation
Checklist:
Additional Context:
Fixes or Closes #(issue)