-
Notifications
You must be signed in to change notification settings - Fork 0
Add granular control for individual service chapter visibility #243
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
base: master
Are you sure you want to change the base?
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]>
miroslavpojer
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.
- pulled
- code read
- unit tests passing
…d-only args syntax Co-authored-by: miroslavpojer <[email protected]>
miroslavpojer
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.
Review comments fixed.
|
@copilot add pr related test among failure_cases in test_action_inputs. |
Co-authored-by: miroslavpojer <[email protected]>
miroslavpojer
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.
Last requirement implemented.
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 adds granular control for hiding individual service chapters in release notes, replacing the all-or-nothing warnings toggle with selective filtering via a new hidden-service-chapters input parameter.
Key Changes:
- Added
hidden-service-chaptersinput accepting comma or newline-separated chapter titles - Implemented filtering logic in
ServiceChapters.to_string()to skip hidden chapters - Added input validation and comprehensive test coverage for the new parameter
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| action.yml | Added hidden-service-chapters input definition with documentation of available chapter titles |
| release_notes_generator/action_inputs.py | Added get_hidden_service_chapters() parser with whitespace trimming and validation logic |
| release_notes_generator/builder/builder.py | Passed parsed hidden chapters list to ServiceChapters constructor |
| release_notes_generator/chapters/service_chapters.py | Added hidden_chapters parameter and overrode to_string() to filter chapters |
| release_notes_generator/utils/constants.py | Added HIDDEN_SERVICE_CHAPTERS constant |
| tests/unit/conftest.py | Updated service_chapters fixture formatting for consistency |
| tests/unit/release_notes_generator/test_action_inputs.py | Added validation tests and input parsing tests for hidden service chapters |
| tests/unit/release_notes_generator/chapters/test_service_chapters.py | Added comprehensive tests for chapter filtering behavior |
| docs/features/service_chapters.md | Documented granular chapter control with usage examples |
| docs/configuration_reference.md | Added hidden-service-chapters to configuration reference table |
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Co-authored-by: Copilot <[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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Co-authored-by: miroslavpojer <[email protected]>
| for chapter in hidden_service_chapters: | ||
| if not isinstance(chapter, str) or not chapter: | ||
| errors.append("Hidden service chapters must be a non-empty string and have non-zero length.") | ||
| break |
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.
This whole block of code is not needed since, I think the same logic you are doing by if title.strip() in
hidden_chapters = [title.strip() for title in titles.split(separator) if title.strip()]
If I am correct and this logic is duplication, you need to update the following logging logic, since the break on line 485 would not happen.
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.
Yes, I know about this. I was thinking about for some time. Then I kept it here waiting of another opinions.
I will remove it it here.
| """ | ||
| return ActionInputs.get_duplicity_scope() in (DuplicityScopeEnum.SERVICE, DuplicityScopeEnum.BOTH) | ||
|
|
||
| def to_string(self) -> str: |
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.
This method has much more logic than just to_string. I do suggest something like serialize_chapters.
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.
This method override parent method to_string to keep the expected contract.
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 react to the comments:)
Implementation Summary: Granular Service Chapter Visibility Control
This PR implements the ability to selectively hide individual service chapters while keeping others visible, as requested in the issue.
Release Notes
What Changed
Core Implementation:
hidden-service-chaptersinput toaction.ymlthat accepts comma or newline-separated list of service chapter titlesHIDDEN_SERVICE_CHAPTERSinutils/constants.pyget_hidden_service_chapters()method inActionInputsto parse the inputServiceChaptersclass to accept and usehidden_chaptersparameterto_string()method inServiceChaptersto filter out hidden chaptersReleaseNotesBuilderto pass hidden chapters toServiceChaptersCode Review Feedback Addressed:
validate_inputs()for hidden service chapters (similar to coderabbit_summary_ignore_groups)validate_inputs()*) fromServiceChapters.__init__to match project conventionsrow-format-link-prin failure_casesget_row_format_link_pr()to verify boolean validationTesting:
ActionInputs.get_hidden_service_chapters()covering:get_row_format_link_pr():ServiceChaptersfiltering covering:Documentation:
docs/features/service_chapters.mdwith configuration examplesdocs/configuration_reference.mdwith the new input parameterQuality Checks:
Usage Example
Behavior
warnings: false, all service chapters are hidden (master override, existing behavior preserved)warnings: trueandhidden-service-chaptersis empty, all service chapters are shown (existing behavior preserved)warnings: trueandhidden-service-chaptershas values, only the specified chapters are hiddenAvailable Service Chapters
Original prompt
This section details on the original issue you should resolve
<issue_title>Add granular control for individual service chapter visibility</issue_title>
<issue_description>## Background
Creating draft closer to desired final state.
Feature
Make it possible to switch on/off not just all service chapters as a group, but one by one.
Proposed Solution
Solution Ideas:
show-service-chapters-> boolean, defaulttrueshow-service-chapter1-> boolean, defaultshow-service-chaptersshow-service-chapter2-> boolean, default `show-service-chapters<agent_instructions>Do review of all documentation files and keep them up to date.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@miroslavpojer ## Background Service chapters help identify quality gaps in release metadata (missing PRs, unlabeled issues, direct commits, etc.). Currently, the `warnings` input controls all service chapters as a single group—either all are shown or all are hidden.For cleaner release notes drafts, users need the ability to selectively show/hide individual service chapters while keeping others visible.
Problem Statement
The current
warnings: true/falsesetting is too coarse-grained. Users cannot:Proposed Solution
Add a new input
hidden-service-chaptersthat accepts a list of service chapter titles to hide:Logic
warnings: false, no service chapters are shown (master override, existing behavior)warnings: true, show all service chapters except those listed inhidden-service-chaptershidden-service-chaptersis empty/unset, all service chapters are visible (existing behavior)Example Usage
*Example 1: Hide specific diagnostic chapters...</comment_new>
<comment_new>@miroslavpojer
...</comment_new>
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.