#203 - Refactor custom chapter using new principles #204
Conversation
- Unit test file refactored.
WalkthroughAdded OS/editor ignores to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/release_notes_generator/chapters/test_custom_chapters.py (7)
🪛 Ruff (0.14.0)tests/unit/release_notes_generator/chapters/test_custom_chapters.py65-65: Unused method argument: (ARG002) 🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.specify/memory/principles.md (1)
309-370: Great pytest conventions; add pytest.ini markers to avoid unknown‑marker warningsDocument defines slow/integration/internal markers; ensure pytest.ini declares them so pytest doesn’t warn and default runs can exclude them.
As a follow‑up, add:
# pytest.ini [pytest] markers = slow: long-running unit tests integration: integration tests excluded from default run smoke: high-signal subset internal: tests for non-public internalsAs per coding guidelines
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (1)
26-89: Tidy the stub: drop unused noqa and prefix unused paramsSilence Ruff warnings and keep signature compatibility by prefixing unused args; remove unused D401 noqa.
Apply:
- def to_chapter_row(self, add_into_chapters: bool = True) -> str: # noqa: D401 + def to_chapter_row(self, _add_into_chapters: bool = True) -> str: return f"{self._rid} row" - def get_rls_notes(self, line_marks: list[str] | None = None) -> str: # noqa: D401 + def get_rls_notes(self, _line_marks: list[str] | None = None) -> str: return ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks).specify/memory/principles.md(2 hunks)tests/unit/release_notes_generator/chapters/test_custom_chapters.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (6)
release_notes_generator/utils/enums.py (1)
DuplicityScopeEnum(24-32)release_notes_generator/action_inputs.py (1)
ActionInputs(64-518)release_notes_generator/model/record/record.py (2)
skip(76-78)is_present_in_chapters(49-55)tests/unit/conftest.py (1)
custom_chapters(74-80)release_notes_generator/chapters/custom_chapters.py (4)
populate(76-105)CustomChapters(71-176)from_yaml_array(107-176)_normalize_labels(33-68)release_notes_generator/model/record/commit_record.py (2)
commit(54-59)CommitRecord(13-87)
🪛 LanguageTool
.specify/memory/principles.md
[grammar] ~267-~267: There might be a mistake here.
Context: ...entence imperative summary. Parameters: - name: concise description - ... Returns...
(QB_NEW_EN)
[grammar] ~268-~268: There might be a mistake here.
Context: ... Parameters: - name: concise description - ... Returns: - Description of value sem...
(QB_NEW_EN)
[grammar] ~271-~271: There might be a mistake here.
Context: ...ame: concise description - ... Returns: - Description of value semantics. Raises:...
(QB_NEW_EN)
[grammar] ~274-~274: There might be a mistake here.
Context: ... of value semantics. Raises: (optional) - ExceptionType: condition """ Canonical ...
(QB_NEW_EN)
[grammar] ~311-~311: There might be a mistake here.
Context: ...mplement PID:A-1 through PID:A-4. Scope: - Applies to all files under tests/ for ...
(QB_NEW_EN)
[grammar] ~320-~320: There might be a mistake here.
Context: ...o, make_user`). Functions vs Classes: - Default style: free test functions. Clas...
(QB_NEW_EN)
[grammar] ~321-~321: There might be a mistake here.
Context: ...h / edge / error) improving readability. - Test classes MUST be named `Test<ThingOr...
(QB_NEW_EN)
[grammar] ~331-~331: There might be a mistake here.
Context: ...ions—no production logic or test bodies. - Fixture default scope MUST remain `funct...
(QB_NEW_EN)
[grammar] ~342-~342: There might be a mistake here.
Context: ...ation adds value. - Logging checks MUST set explicit level (`with caplog.at_level("...
(QB_NEW_EN)
[grammar] ~346-~346: There might be a mistake here.
Context: ...Y identify a minimal high-signal subset. - Known unfixed bugs MUST use `pytest.mark...
(QB_NEW_EN)
[grammar] ~348-~348: There might be a mistake here.
Context: ...on absence (e.g., OS-specific behavior). - Internal-only tests touching non-public ...
(QB_NEW_EN)
[grammar] ~351-~351: There might be a mistake here.
Context: ... @pytest.mark.internal. Test Doubles: - Fakes (simple dataclasses, lambdas, smal...
(QB_NEW_EN)
[grammar] ~352-~352: There might be a mistake here.
Context: ...ns) SHOULD be preferred over deep mocks. - Monkeypatches MUST patch where an object...
(QB_NEW_EN)
[grammar] ~363-~363: There might be a mistake here.
Context: ...on Checklist (Guidance / NON-NORMATIVE): - Is a class genuinely clearer than multip...
(QB_NEW_EN)
[grammar] ~367-~367: There might be a mistake here.
Context: ...ork/time/FS? If yes, isolate or mark. - Does failure message clearly show diff? If n...
(QB_NEW_EN)
[grammar] ~367-~367: There might be a mistake here.
Context: ...or mark. - Does failure message clearly show diff? If not, prefer equality or add `m...
(QB_NEW_EN)
🪛 Ruff (0.14.0)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py
67-67: Unused method argument: add_into_chapters
(ARG002)
67-67: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
76-76: Unused method argument: line_marks
(ARG002)
76-76: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (4)
.gitignore (1)
161-164: OS/editor artifacts ignore — LGTMGood adds; prevents accidental commits of local artifacts.
.specify/memory/principles.md (1)
265-276: Docstring template alignment — LGTMTemplate wording is consistent and clear.
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (2)
100-116: Populate happy-path coverage — LGTMGood AAA structure and clear assertions.
118-136: Duplicity scope invariance — LGTMPatch scope and assert dual population; concise and deterministic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (6)
67-77: Silence lint: mark unused parameters with underscoresBoth parameters are unused; rename to underscore to satisfy Ruff ARG002.
- def to_chapter_row(self, add_into_chapters: bool = True) -> str: + def to_chapter_row(self, _add_into_chapters: bool = True) -> str: return f"{self._rid} row" @@ - def get_rls_notes(self, line_marks: list[str] | None = None) -> str: + def get_rls_notes(self, _line_marks: list[str] | None = None) -> str: return ""Based on static analysis hints.
29-41: Remove unused constructor parameter pulls_countThe stub never uses pulls_count; dropping it reduces noise and avoids misleading callers.
- def __init__( - self, - rid: str, - labels: list[str] | None, - pulls_count: int, - skip: bool, - contains_change_increment: bool, - ): - super().__init__(labels=labels, skip=skip) - self.pulls_count = pulls_count + def __init__( + self, + rid: str, + labels: list[str] | None, + skip: bool, + contains_change_increment: bool, + ): + super().__init__(labels=labels, skip=skip) self._rid = rid self._contains = contains_change_increment
79-86: Align factory with constructor change (remove pulls_count)Update the factory to mirror the simplified stub signature.
- def _make( - rid: str = "org/repo#X", - labels: list[str] | None = None, - pulls_count: int = 1, - skip: bool = False, - contains_change_increment: bool = True, - ) -> Record: - return RecordStub(rid, labels or [], pulls_count, skip, contains_change_increment) + def _make( + rid: str = "org/repo#X", + labels: list[str] | None = None, + skip: bool = False, + contains_change_increment: bool = True, + ) -> Record: + return RecordStub(rid, labels or [], skip, contains_change_increment)
118-126: Add DuplicityScopeEnum.BOTH to param setFor completeness, include BOTH in scope coverage.
- [DuplicityScopeEnum.SERVICE, DuplicityScopeEnum.NONE], - ids=["duplicity-scope-service", "duplicity-scope-none"], + [DuplicityScopeEnum.SERVICE, DuplicityScopeEnum.NONE, DuplicityScopeEnum.BOTH], + ids=["duplicity-scope-service", "duplicity-scope-none", "duplicity-scope-both"],
171-186: Scope caplog to the module logger to avoid cross-test noiseTarget the logger used by custom_chapters for more deterministic assertions.
- caplog.set_level("WARNING") + caplog.set_level("WARNING", logger="release_notes_generator.chapters.custom_chapters")
244-259: Also scope caplog here to the module loggerKeeps skip message assertions precise per logger.
- caplog.set_level("WARNING") + caplog.set_level("WARNING", logger="release_notes_generator.chapters.custom_chapters")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (6)
release_notes_generator/action_inputs.py (1)
ActionInputs(64-518)release_notes_generator/model/record/record.py (1)
skip(76-78)tests/unit/conftest.py (1)
custom_chapters(74-80)release_notes_generator/chapters/custom_chapters.py (4)
populate(76-105)CustomChapters(71-176)from_yaml_array(107-176)_normalize_labels(33-68)release_notes_generator/model/record/issue_record.py (1)
IssueRecord(17-268)release_notes_generator/model/record/commit_record.py (2)
commit(54-59)CommitRecord(13-87)
🪛 Ruff (0.14.0)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py
67-67: Unused method argument: add_into_chapters
(ARG002)
76-76: Unused method argument: line_marks
(ARG002)
🔇 Additional comments (1)
tests/unit/release_notes_generator/chapters/test_custom_chapters.py (1)
16-21: The review comment is based on an incorrect assumption about Python 3.8 support.The project's Black configuration explicitly targets Python 3.11 (
target-version = ['py311']), and there is no evidence of Python 3.8 support in the codebase. Thelist[str]anddict[str, Record]type hints used throughout the test file are fully compatible with Python 3.11 and do not require thefrom __future__ import annotationsimport. The code is correct as-is.Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
Documentation
Tests
Chores
Fixes #203