Feature/147 split mining logic #148
Conversation
OlivieFranklova
requested review from
miroslavpojer and
Zejnilovic
as code owners
|
Caution Review failedThe pull request is closed. """ WalkthroughThe mining logic previously embedded in the release notes generator was extracted into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Generator as ReleaseNotesGenerator
participant Miner as DataMiner
participant GitHub as GitHub API
Generator->>Miner: mine_data()
Miner->>GitHub: Fetch repository, releases, issues, PRs, commits
Miner-->>Generator: MinedData
Generator->>Generator: _filter_by_release(MinedData)
Generator->>Generator: Generate release notes from filtered data
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 3
🔭 Outside diff range comments (1)
tests/tests_miner.py (1)
36-193: 🛠️ Refactor suggestionTests are directly accessing private methods.
All tests in this file are testing the private method
_get_latest_releasedirectly, which breaks encapsulation. Consider either:
- Making the method public if it's intended to be part of the public API
- Testing it indirectly through the public
mine_datamethodWould you like me to help refactor these tests to use the public interface instead?
🧰 Tools
🪛 Ruff (0.11.9)
60-60: Yoda condition detected
(SIM300)
61-61: Yoda condition detected
(SIM300)
62-62: Yoda condition detected
(SIM300)
63-63: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
90-90: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
92-92: Yoda condition detected
(SIM300)
114-114: Yoda condition detected
(SIM300)
136-136: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 2(SIM300)
137-137: Yoda condition detected
(SIM300)
138-138: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 1(SIM300)
165-165: Yoda condition detected
(SIM300)
190-190: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 2(SIM300)
191-191: Yoda condition detected
(SIM300)
192-192: Yoda condition detected
(SIM300)
🧹 Nitpick comments (4)
release_notes_generator/model/MinedData.py (2)
54-57: Simplify the return statement.The method can be simplified by directly returning the condition.
def is_empty(self): - if self.repository is None: - return True - return False + return self.repository is None🧰 Tools
🪛 Ruff (0.11.9)
55-57: Return the condition
self.repository is NonedirectlyReplace with
return self.repository is None(SIM103)
62-62: Use idiomatic None comparison.Use
is notfor None comparison as per Python conventions.- self.release = rls_mock if rls_mock != None else mocker.Mock(spec=GitRelease) + self.release = rls_mock if rls_mock is not None else mocker.Mock(spec=GitRelease)🧰 Tools
🪛 Ruff (0.11.9)
62-62: Comparison to
Noneshould becond is not NoneReplace with
cond is not None(E711)
tests/tests_miner.py (2)
17-28: Remove unused imports.Static analysis identified several unused imports that should be removed.
-import time -from datetime import datetime, timedelta -from unittest.mock import MagicMock - from github import Github from github.GitRelease import GitRelease from release_notes_generator.miner import DataMiner from release_notes_generator.model.MinedData import MinedData -from release_notes_generator.generator import ReleaseNotesGenerator -from release_notes_generator.model.custom_chapters import CustomChapters -from release_notes_generator.utils.constants import ROW_FORMAT_ISSUE🧰 Tools
🪛 Ruff (0.11.9)
17-17:
timeimported but unusedRemove unused import:
time(F401)
18-18:
datetime.datetimeimported but unusedRemove unused import
(F401)
18-18:
datetime.timedeltaimported but unusedRemove unused import
(F401)
19-19:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
26-26:
release_notes_generator.generator.ReleaseNotesGeneratorimported but unusedRemove unused import:
release_notes_generator.generator.ReleaseNotesGenerator(F401)
27-27:
release_notes_generator.model.custom_chapters.CustomChaptersimported but unusedRemove unused import:
release_notes_generator.model.custom_chapters.CustomChapters(F401)
28-28:
release_notes_generator.utils.constants.ROW_FORMAT_ISSUEimported but unusedRemove unused import:
release_notes_generator.utils.constants.ROW_FORMAT_ISSUE(F401)
60-63: Consider using standard assertion order for better readability.The Yoda-style assertions (expected == actual) are less readable than the standard order (actual == expected).
For example:
- assert 2 == len(mock_log_info.call_args_list) + assert len(mock_log_info.call_args_list) == 2This makes the assertions more natural to read: "assert that the length of call args list equals 2" rather than "assert that 2 equals the length of call args list".
Also applies to: 89-92, 114-114, 136-138, 164-165, 190-192
🧰 Tools
🪛 Ruff (0.11.9)
60-60: Yoda condition detected
(SIM300)
61-61: Yoda condition detected
(SIM300)
62-62: Yoda condition detected
(SIM300)
63-63: Yoda condition detected
(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/MinedData.py(1 hunks)tests/test_release_notes_generator.py(3 hunks)tests/tests_miner.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_release_notes_generator.py (1)
tests/conftest.py (1)
mock_git_release(86-89)
release_notes_generator/miner.py (4)
release_notes_generator/action_inputs.py (5)
ActionInputs(60-411)get_github_repository(70-74)is_from_tag_name_defined(98-103)get_from_tag_name(91-95)get_published_at(148-153)release_notes_generator/model/MinedData.py (1)
MinedData(37-76)release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(31-75)
release_notes_generator/model/MinedData.py (1)
tests/conftest.py (1)
mock_repo(78-81)
🪛 Pylint (3.3.7)
release_notes_generator/miner.py
[refactor] 37-37: Too few public methods (1/2)
(R0903)
🪛 Ruff (0.11.9)
release_notes_generator/model/MinedData.py
55-57: Return the condition self.repository is None directly
Replace with return self.repository is None
(SIM103)
62-62: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
tests/tests_miner.py
17-17: time imported but unused
Remove unused import: time
(F401)
18-18: datetime.datetime imported but unused
Remove unused import
(F401)
18-18: datetime.timedelta imported but unused
Remove unused import
(F401)
19-19: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
26-26: release_notes_generator.generator.ReleaseNotesGenerator imported but unused
Remove unused import: release_notes_generator.generator.ReleaseNotesGenerator
(F401)
27-27: release_notes_generator.model.custom_chapters.CustomChapters imported but unused
Remove unused import: release_notes_generator.model.custom_chapters.CustomChapters
(F401)
28-28: release_notes_generator.utils.constants.ROW_FORMAT_ISSUE imported but unused
Remove unused import: release_notes_generator.utils.constants.ROW_FORMAT_ISSUE
(F401)
60-60: Yoda condition detected
(SIM300)
61-61: Yoda condition detected
(SIM300)
62-62: Yoda condition detected
(SIM300)
63-63: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
90-90: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
92-92: Yoda condition detected
(SIM300)
114-114: Yoda condition detected
(SIM300)
136-136: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
137-137: Yoda condition detected
(SIM300)
138-138: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 1
(SIM300)
165-165: Yoda condition detected
(SIM300)
190-190: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
191-191: Yoda condition detected
(SIM300)
192-192: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mypy Type Check
- GitHub Check: Black Format Check
🔇 Additional comments (2)
tests/test_release_notes_generator.py (1)
68-68: LGTM! Test mocks correctly updated.The mock patches have been properly updated to reflect the architectural change where release retrieval logic moved from
ReleaseNotesGeneratortoDataMiner.Also applies to: 111-111, 161-161
release_notes_generator/generator.py (1)
28-28: Excellent refactoring - separation of concerns well implemented.The generator now properly delegates data mining responsibilities to
DataMinerand focuses solely on release notes generation. The use of theMinedDataobject provides a clean interface between components.Also applies to: 49-49, 74-103
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
release_notes_generator/miner.py (1)
125-125: Add None check for safer logging.This matches a previous review comment about adding a None check before accessing length to prevent potential AttributeError.
🧹 Nitpick comments (4)
release_notes_generator/miner.py (1)
67-73: Update docstring parameter reference.The docstring references
@param repobut the actual parameter isdata: MinedData.- @param repo: The repository to get the latest release from. + @param data: The MinedData object containing the repository to get the latest release from.tests/tests_miner.py (3)
44-44: Fix None comparison to use identity check.Use
is not Noneinstead of!= Nonefor None comparisons.- self.release = rls_mock if rls_mock != None else mocker.Mock(spec=GitRelease) + self.release = rls_mock if rls_mock is not None else mocker.Mock(spec=GitRelease)🧰 Tools
🪛 Ruff (0.11.9)
44-44: Comparison to
Noneshould becond is not NoneReplace with
cond is not None(E711)
86-89: Fix Yoda conditions for better readability.The assertions use Yoda conditions (literal == variable) which are less readable than normal conditions.
- assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] - assert ('Skipping invalid type of version tag: %s', 'v1.0.0') == mock_log_debug.call_args_list[0][0] - assert ('Skipping invalid type of version tag: %s', 'v2.0.0') == mock_log_debug.call_args_list[1][0] + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo') + assert mock_log_debug.call_args_list[0][0] == ('Skipping invalid type of version tag: %s', 'v1.0.0') + assert mock_log_debug.call_args_list[1][0] == ('Skipping invalid type of version tag: %s', 'v2.0.0')Also applies to: 115-118
🧰 Tools
🪛 Ruff (0.11.9)
86-86: Yoda condition detected
(SIM300)
87-87: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
162-164: Fix remaining Yoda conditions for consistency.Apply the same Yoda condition fixes to the remaining assertions for consistency.
- assert 2 == len(mock_log_info.call_args_list) - assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] + assert len(mock_log_info.call_args_list) == 2 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo')- assert 1 == len(mock_log_info.call_args_list) - assert ('Getting latest release by from-tag name %s', "") == mock_log_info.call_args_list[0][0] + assert len(mock_log_info.call_args_list) == 1 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by from-tag name %s', "")- assert 2 == len(mock_log_info.call_args_list) - assert ('Getting latest release by from-tag name %s', "") == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for received from-tag %s. Ending!', '') == mock_log_info.call_args_list[1][0] + assert len(mock_log_info.call_args_list) == 2 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by from-tag name %s', "") + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for received from-tag %s. Ending!', '')Also applies to: 190-191, 216-218
🧰 Tools
🪛 Ruff (0.11.9)
162-162: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 2(SIM300)
163-163: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/MinedData.py(1 hunks)tests/test_release_notes_generator.py(3 hunks)tests/tests_miner.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/model/MinedData.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_release_notes_generator.py (1)
tests/conftest.py (1)
mock_git_release(86-89)
release_notes_generator/generator.py (5)
release_notes_generator/miner.py (2)
DataMiner(38-148)mine_data(47-65)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(31-75)release_notes_generator/model/MinedData.py (1)
is_empty(49-50)release_notes_generator/utils/utils.py (1)
get_change_url(31-56)release_notes_generator/record/record_factory.py (2)
RecordFactory(40-142)generate(46-142)
🪛 Ruff (0.11.9)
tests/tests_miner.py
44-44: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
86-86: Yoda condition detected
(SIM300)
87-87: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
116-116: Yoda condition detected
(SIM300)
117-117: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
140-140: Yoda condition detected
(SIM300)
162-162: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
163-163: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
190-190: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 1
(SIM300)
191-191: Yoda condition detected
(SIM300)
216-216: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
217-217: Yoda condition detected
(SIM300)
218-218: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Black Format Check
- GitHub Check: Mypy Type Check
🔇 Additional comments (9)
tests/test_release_notes_generator.py (1)
68-68: LGTM! Mock targets correctly updated for DataMiner refactoring.The patch targets have been properly updated to reflect the migration of
get_latest_releasemethod fromReleaseNotesGeneratortoDataMiner. This aligns perfectly with the architectural changes.Also applies to: 111-111, 161-161
release_notes_generator/generator.py (5)
28-28: LGTM! Clean import of the new DataMiner component.
49-49: LGTM! Rate limiter properly instantiated in constructor.Good practice to create the rate limiter once in the constructor rather than on each method call.
73-76: LGTM! Clean delegation to DataMiner with proper error handling.The data mining is now properly delegated to DataMiner, and the empty data check ensures graceful handling when repository is not found.
77-97: LGTM! Well-preserved filtering logic with improved data access.The filtering logic has been correctly adapted to work with the pre-mined data structure. The logic for filtering issues, PRs, and commits based on release dates remains intact while benefiting from the cleaner data structure.
98-110: LGTM! Clean integration with the new data structure.The changelog URL generation and record factory calls have been properly updated to use the mined data, maintaining the same functionality with improved organization.
release_notes_generator/miner.py (2)
82-87: LGTM! Appropriate error handling for missing required tag.Using
sys.exit(1)is appropriate for a CLI tool when a required tag is not found, as this represents a user configuration error that should terminate the process.
127-148: LGTM! Robust semantic version parsing with proper error handling.The semantic version parsing correctly:
- Filters out drafts and pre-releases
- Handles ValueError and TypeError exceptions gracefully
- Strips 'v' prefix from tag names
- Uses proper version comparison
- Includes helpful debug logging for skipped tags
tests/tests_miner.py (1)
62-219: LGTM! Comprehensive test coverage for DataMiner functionality.The test suite provides excellent coverage of the
get_latest_releasemethod, including:
- Tag-defined release scenarios (found/not found)
- Semantic version sorting scenarios
- Error handling for invalid version tags (TypeError/ValueError)
- Proper logging verification
- Appropriate mocking of dependencies
The tests will help ensure the reliability of the extracted mining logic.
🧰 Tools
🪛 Ruff (0.11.9)
86-86: Yoda condition detected
(SIM300)
87-87: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
89-89: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
116-116: Yoda condition detected
(SIM300)
117-117: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
140-140: Yoda condition detected
(SIM300)
162-162: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 2(SIM300)
163-163: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
190-190: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 1(SIM300)
191-191: Yoda condition detected
(SIM300)
216-216: Yoda condition detected
Rewrite as
len(mock_log_info.call_args_list) == 2(SIM300)
217-217: Yoda condition detected
(SIM300)
218-218: Yoda condition detected
(SIM300)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
release_notes_generator/miner.py (1)
127-127:⚠️ Potential issueAdd None check before accessing length.
If
self._safe_callreturns None due to an error, this line will raise an AttributeError.-logger.info("Fetched %d issues", len(data.issues)) +logger.info("Fetched %d issues", len(data.issues) if data.issues else 0)
🧹 Nitpick comments (2)
release_notes_generator/miner.py (1)
49-67: Consider consistent data handling for all collections.Commits are converted to a list immediately (line 64) while issues and pull_requests remain as PaginatedList objects. This inconsistency could lead to unexpected behavior.
Consider either converting all to lists immediately or keeping all as PaginatedList objects until needed:
# pulls and commits, and then reduce them by the latest release since time data.pull_requests = self._safe_call(data.repository.get_pulls)(state=PR_STATE_CLOSED) -data.commits = list(self._safe_call(data.repository.get_commits)()) +data.commits = self._safe_call(data.repository.get_commits)()tests/test_miner.py (1)
81-84: Fix Yoda conditions for better readability.Replace Yoda conditions with standard Python comparison style for improved readability.
-assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] -assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] -assert ('Skipping invalid type of version tag: %s | Error: %s', 'v1.0.0', '') == mock_log_error.call_args_list[0][0] -assert ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '') == mock_log_error.call_args_list[2][0] +assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) +assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo') +assert mock_log_error.call_args_list[0][0] == ('Skipping invalid type of version tag: %s | Error: %s', 'v1.0.0', '') +assert mock_log_error.call_args_list[2][0] == ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '')Apply the same pattern to all other Yoda conditions in the file:
- Line 157:
assert len(mock_log_info.call_args_list) == 2- Line 184:
assert len(mock_log_info.call_args_list) == 1- Line 210:
assert len(mock_log_info.call_args_list) == 2Also applies to: 110-113, 135-135, 157-159, 184-185, 210-212
🧰 Tools
🪛 Ruff (0.11.9)
81-81: Yoda condition detected
(SIM300)
82-82: Yoda condition detected
(SIM300)
83-83: Yoda condition detected
(SIM300)
84-84: Yoda condition detected
(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)tests/test_miner.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
release_notes_generator/generator.py (5)
release_notes_generator/miner.py (2)
DataMiner(40-151)mine_data(49-67)release_notes_generator/utils/github_rate_limiter.py (1)
GithubRateLimiter(31-75)release_notes_generator/model/MinedData.py (1)
is_empty(49-50)release_notes_generator/utils/utils.py (1)
get_change_url(31-56)release_notes_generator/record/record_factory.py (2)
RecordFactory(40-142)generate(46-142)
🪛 Ruff (0.11.9)
tests/test_miner.py
81-81: Yoda condition detected
(SIM300)
82-82: Yoda condition detected
(SIM300)
83-83: Yoda condition detected
(SIM300)
84-84: Yoda condition detected
(SIM300)
110-110: Yoda condition detected
(SIM300)
111-111: Yoda condition detected
(SIM300)
112-112: Yoda condition detected
(SIM300)
113-113: Yoda condition detected
(SIM300)
135-135: Yoda condition detected
(SIM300)
157-157: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
158-158: Yoda condition detected
(SIM300)
159-159: Yoda condition detected
(SIM300)
184-184: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 1
(SIM300)
185-185: Yoda condition detected
(SIM300)
210-210: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
211-211: Yoda condition detected
(SIM300)
212-212: Yoda condition detected
(SIM300)
🔇 Additional comments (7)
release_notes_generator/generator.py (4)
28-28: Import statement looks good.The import follows the project's structure and naming conventions.
49-49: Good refactoring to instantiate rate limiter once.Creating the rate limiter in the constructor and reusing it is more efficient than creating it multiple times.
73-77: DataMiner integration is well implemented.The error handling using
data.is_empty()properly checks if the repository was found before proceeding.
105-111: RecordFactory call is properly implemented.The explicit conversion to lists ensures RecordFactory receives proper list objects instead of PaginatedList objects.
release_notes_generator/miner.py (2)
40-47: Well-structured class initialization.The DataMiner class is properly initialized with dependency injection for both GitHub instance and rate limiter.
129-151: Robust semantic version handling.The method properly filters releases and handles version parsing errors with appropriate logging.
tests/test_miner.py (1)
30-57: Well-designed test helpers.The decorator mock and MinedDataMock provide good test isolation and make tests more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
release_notes_generator/miner.py (1)
126-126: Add None check before accessing length.This issue was previously raised and is still valid. If
self._safe_callreturns None due to an error, this line will raise an AttributeError.Apply this fix:
- logger.info("Fetched %d issues", len(data.issues)) + logger.info("Fetched %d issues", len(data.issues) if data.issues else 0)
🧹 Nitpick comments (2)
tests/test_miner.py (2)
81-84: Fix Yoda conditions for better readability.Replace the Yoda conditions with standard comparisons for better readability.
For lines 81-84:
- assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] - assert ('Skipping invalid type of version tag: %s | Error: %s', 'v1.0.0', '') == mock_log_error.call_args_list[0][0] - assert ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '') == mock_log_error.call_args_list[2][0] + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo') + assert mock_log_error.call_args_list[0][0] == ('Skipping invalid type of version tag: %s | Error: %s', 'v1.0.0', '') + assert mock_log_error.call_args_list[2][0] == ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '')For lines 110-113:
- assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] - assert ('Skipping invalid value of version tag: %s', 'v1.0.0') == mock_log_error.call_args_list[0][0] - assert ('Skipping invalid value of version tag: %s', 'v2.0.0') == mock_log_error.call_args_list[1][0] + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo') + assert mock_log_error.call_args_list[0][0] == ('Skipping invalid value of version tag: %s', 'v1.0.0') + assert mock_log_error.call_args_list[1][0] == ('Skipping invalid value of version tag: %s', 'v2.0.0')Also applies to: 110-113
🧰 Tools
🪛 Ruff (0.11.9)
81-81: Yoda condition detected
(SIM300)
82-82: Yoda condition detected
(SIM300)
83-83: Yoda condition detected
(SIM300)
84-84: Yoda condition detected
(SIM300)
135-135: Fix remaining Yoda conditions throughout the test file.For consistency and readability, fix all remaining Yoda conditions.
Line 135:
- assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',)Lines 157-159:
- assert 2 == len(mock_log_info.call_args_list) - assert ('Getting latest release by semantic ordering (could not be the last one by time).',) == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for %s. 1st release for repository!', 'org/repo') == mock_log_info.call_args_list[1][0] + assert len(mock_log_info.call_args_list) == 2 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by semantic ordering (could not be the last one by time).',) + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for %s. 1st release for repository!', 'org/repo')Lines 184-185:
- assert 1 == len(mock_log_info.call_args_list) - assert ('Getting latest release by from-tag name %s', "") == mock_log_info.call_args_list[0][0] + assert len(mock_log_info.call_args_list) == 1 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by from-tag name %s', "")Lines 210-212:
- assert 2 == len(mock_log_info.call_args_list) - assert ('Getting latest release by from-tag name %s', "") == mock_log_info.call_args_list[0][0] - assert ('Latest release not found for received from-tag %s. Ending!', '') == mock_log_info.call_args_list[1][0] + assert len(mock_log_info.call_args_list) == 2 + assert mock_log_info.call_args_list[0][0] == ('Getting latest release by from-tag name %s', "") + assert mock_log_info.call_args_list[1][0] == ('Latest release not found for received from-tag %s. Ending!', '')Also applies to: 157-159, 184-185, 210-212
🧰 Tools
🪛 Ruff (0.11.9)
135-135: Yoda condition detected
(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)tests/test_miner.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/generator.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_miner.py (3)
release_notes_generator/miner.py (2)
DataMiner(40-150)get_latest_release(69-107)release_notes_generator/model/MinedData.py (1)
MinedData(38-50)tests/conftest.py (2)
mock_repo(78-81)mock_git_releases(93-102)
🪛 Ruff (0.11.9)
tests/test_miner.py
81-81: Yoda condition detected
(SIM300)
82-82: Yoda condition detected
(SIM300)
83-83: Yoda condition detected
(SIM300)
84-84: Yoda condition detected
(SIM300)
110-110: Yoda condition detected
(SIM300)
111-111: Yoda condition detected
(SIM300)
112-112: Yoda condition detected
(SIM300)
113-113: Yoda condition detected
(SIM300)
135-135: Yoda condition detected
(SIM300)
157-157: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
158-158: Yoda condition detected
(SIM300)
159-159: Yoda condition detected
(SIM300)
184-184: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 1
(SIM300)
185-185: Yoda condition detected
(SIM300)
210-210: Yoda condition detected
Rewrite as len(mock_log_info.call_args_list) == 2
(SIM300)
211-211: Yoda condition detected
(SIM300)
212-212: Yoda condition detected
(SIM300)
🔇 Additional comments (2)
release_notes_generator/miner.py (2)
69-107: Well-structured release retrieval logic.The method properly handles both tag-based and semantic version-based release retrieval with appropriate error handling and logging.
128-150: Robust semantic version handling.The implementation correctly filters releases and handles version parsing errors with detailed logging including tracebacks.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
release_notes_generator/generator.py (1)
76-78: Consider consistent handling of data conversions.The code converts
pull_requestsandcommitsto lists but notissues. While this works becauseissuesis already converted to a list in the miner, it creates inconsistency that could lead to confusion.Either convert all to lists for consistency:
- issues_list = data.issues + issues_list = list(data.issues)Or add a comment explaining why issues doesn't need conversion:
+ # Note: data.issues is already a list from DataMiner issues_list = data.issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/action_inputs.py(1 hunks)release_notes_generator/generator.py(3 hunks)release_notes_generator/miner.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
release_notes_generator/action_inputs.py (1)
release_notes_generator/utils/gh_action.py (1)
get_action_input(26-38)
release_notes_generator/miner.py
Outdated
| if rls is None: | ||
| logger.info( | ||
| "Latest release not found for received from-tag %s. Ending!", ActionInputs.get_from_tag_name() | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sys.exit with exception for better error handling.
Using sys.exit(1) in a class method makes the code difficult to test and prevents proper error handling by callers. Consider raising an exception instead.
if rls is None:
logger.info(
"Latest release not found for received from-tag %s. Ending!", ActionInputs.get_from_tag_name()
)
- sys.exit(1)
+ raise ValueError(f"Release with tag '{ActionInputs.get_from_tag_name()}' not found")📝 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.
| if rls is None: | |
| logger.info( | |
| "Latest release not found for received from-tag %s. Ending!", ActionInputs.get_from_tag_name() | |
| ) | |
| sys.exit(1) | |
| if rls is None: | |
| logger.info( | |
| "Latest release not found for received from-tag %s. Ending!", | |
| ActionInputs.get_from_tag_name() | |
| ) | |
| raise ValueError( | |
| f"Release with tag '{ActionInputs.get_from_tag_name()}' not found" | |
| ) |
🤖 Prompt for AI Agents
In release_notes_generator/miner.py around lines 84 to 88, replace the
sys.exit(1) call with raising a specific exception to indicate the error
condition. This change will improve error handling by allowing callers to catch
and manage the exception instead of abruptly terminating the program, making the
code more testable and maintainable.
Release Notes:
Closes #147
Summary by CodeRabbit
New Features
Refactor
Tests