-
Notifications
You must be signed in to change notification settings - Fork 0
Fix chapter tracking to count unique chapters instead of invocations #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: miroslavpojer <[email protected]>
Co-authored-by: miroslavpojer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- code review done
@copilot Fix failing integration tests - snapshot ones. It is failing with error: AttributeError: 'R' object has no attribute 'added_into_chapters'
Co-authored-by: miroslavpojer <[email protected]>
Fixed in commit af8ffb4. Added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the chapter tracking mechanism in the Record model to correctly count unique chapters instead of counting method invocations. Previously, added_into_chapters() incremented a counter each time it was called, leading to inflated chapter counts when records were added multiple times. The fix changes the internal tracking from an integer counter to a set of chapter IDs, ensuring that present_in_chapters() returns the true count of unique chapters containing a record.
Key Changes:
- Changed internal tracking from integer counter to set-based tracking of unique chapter identifiers
- Updated method signature to accept chapter_id parameter:
added_into_chapters(chapter_id: str) - Moved chapter tracking calls from
to_chapter_row()implementations to chapter population logic
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| release_notes_generator/model/record/record.py | Core fix: changed _present_in_chapters from int to set and updated tracking logic |
| release_notes_generator/model/record/pull_request_record.py | Removed internal added_into_chapters() call from to_chapter_row() |
| release_notes_generator/model/record/issue_record.py | Removed internal added_into_chapters() call from to_chapter_row() |
| release_notes_generator/model/record/hierarchy_issue_record.py | Removed internal added_into_chapters() call from to_chapter_row() |
| release_notes_generator/model/record/commit_record.py | Removed internal added_into_chapters() call from to_chapter_row() |
| release_notes_generator/chapters/service_chapters.py | Added explicit added_into_chapters(chapter_id) calls before row generation at 11 call sites |
| release_notes_generator/chapters/custom_chapters.py | Added explicit added_into_chapters(chapter_id) call before row generation |
| tests/unit/release_notes_generator/model/test_record.py | Updated tests to use new signature and added test for unique chapter tracking |
| tests/unit/release_notes_generator/model/test_commit_record.py | Updated test to explicitly call added_into_chapters() to simulate multiple chapter additions |
| tests/integration/test_release_notes_snapshot.py | Added added_into_chapters() method to mock record class |
|
@copilot do double check review using senior developer agent |
Completed comprehensive review of the implementation. All checks passed: ✅ Code Quality
✅ Testing
✅ Implementation Review
✅ Security
The implementation correctly solves issue #191 by tracking unique chapters instead of invocations. |
tmikula-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see and react to my comments. Thanks! :)
tmikula-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, much better with this updated naming. Approved.
Release Notes
Fix
added_into_chaptersto track unique chapters ✅Implementation Complete
_present_in_chaptersfrominttoset[str]added_into_chapters(chapter_id: str)signaturepresent_in_chapters()to returnlen(set)is_present_in_chapterspropertyadded_into_chapters()calls fromto_chapter_row()implementationsadded_into_chapters(chapter_id)BEFOREto_chapter_row()added_into_chaptersmethod to mockSummary
Successfully changed chapter tracking from simple counter to set-based tracking of unique chapter IDs. The fix ensures
present_in_chapters()returns the true count of unique chapters containing a record, resolving the overcounting issue described in #191. Integration tests updated to support the newadded_into_chapters(chapter_id)method signature.Original prompt
added_into_chaptersto correctly track chapter presences #191💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.