-
Notifications
You must be signed in to change notification settings - Fork 0
Fix release note row rendering to skip missing fields (type/assignee/developers/PRs) #251
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]>
…sts and docs Co-authored-by: miroslavpojer <[email protected]>
Co-authored-by: miroslavpojer <[email protected]>
…ssion 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 review - comments created
- local test run is passing
docs/features/custom_row_formats.md
Outdated
| ``` | ||
| (Formatting reflects the provided custom templates.) | ||
| (Formatting reflects the provided custom templates. Note how issues without a type omit the type prefix, and issues without PRs omit the "in {pull-requests}" suffix.) |
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.
| (Formatting reflects the provided custom templates. Note how issues without a type omit the type prefix, and issues without PRs omit the "in {pull-requests}" suffix.) | |
| > Formatting reflects the provided custom templates. Note how issues without a type omit the type prefix, and issues without PRs omit the "in {pull-requests}" suffix. |
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.
Applied the suggested formatting change. Updated in commit a9df629.
| # require escaping in regex patterns (hyphens are only special inside character classes). | ||
| # The triple-brace syntax {{{var}}} in f-strings produces {{var}} which becomes | ||
| # \{var\} in the final regex pattern, correctly matching literal braces. | ||
| FIELD_TYPE = "type" |
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.
@copilot here are defined supported placeholders, but their string form is already defined in constants.py
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.
Changed to reuse field name constants from SUPPORTED_ROW_FORMAT_KEYS_ISSUE in constants.py. Updated in commit a9df629.
docs/features/custom_row_formats.md
Outdated
| - `row-format-pr` | ||
| - `row-format-link-pr` (boolean controlling prefix `PR:` presence for standalone PR links) | ||
| - Placeholders are case-insensitive; unknown placeholders are removed. | ||
| - **Empty field suppression**: When a placeholder value is empty, the generator intelligently omits the surrounding text fragment: |
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.
@copilot here you should mention that suppression is implemented only for a sub group of all supported placeholders.
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.
Added clarification that suppression is only implemented for specific placeholders (type, assignees, developers, pull-requests) with a note that other placeholders do not trigger suppression. Updated in commit a9df629.
| return user | ||
|
|
||
|
|
||
| def make_mock_issue(mocker, number: int, title: str, type_name=None, assignees=None, author=None): |
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.
@copilot try to reuse already existing mocks in conftest.py.
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.
Refactored tests to reuse existing mocks from conftest.py (mock_issue_closed, mock_user, mock_pull_closed) instead of creating custom helper functions. Updated in commit a9df629.
| return pr | ||
|
|
||
|
|
||
| class TestIssueRecordRowFormattingMissingFields: |
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.
@copilot follow style in other test files. There are no Test classes. There are just test methods.
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.
Removed the Test class wrapper and converted to plain test functions following the project's existing test style. Updated in commit a9df629.
…ests Co-authored-by: miroslavpojer <[email protected]>
Overview
Release notes rendered dangling text fragments when issue metadata was missing, producing output like:
This PR implements intelligent suppression of empty field fragments so missing metadata results in clean output:
Release Notes
Changes
Core Implementation
Added
format_row_with_suppression()inrelease_notes_generator/utils/record_utils.py:{type}:prefix when type is missing (no "N/A" substitution)assigned to {assignees}phrase when assignees list is emptydeveloped by {developers} in {pull-requests}phrase when both are emptyin {pull-requests}suffix when only PRs are missingconstants.pyfor maintainabilityUpdated
IssueRecord.to_chapter_row(): Changed type fallback from'N/A'to'', delegates formatting to suppression helperUpdated
HierarchyIssueRecord.to_chapter_row(): Changed type fallback from'None'to'', delegates formatting to suppression helperTesting & Documentation
test_issue_record_row_formatting.py, following project test style conventions and reusing existing fixtures fromconftest.pydocs/features/custom_row_formats.mddocumenting empty field suppression behavior with examples, clarifying that suppression only applies to specific placeholders ({type},{assignees},{developers},{pull-requests})Template Coupling Note: Suppression rules target default row format patterns. Custom templates with different phrase structures may not benefit from all suppression rules.
Original prompt
This section details on the original issue you should resolve
<issue_title>Fix release note row rendering to skip missing fields (type/assignee/developers/PRs)</issue_title>
<issue_description>### Feature Description
Update release-note row rendering so that when issue metadata is missing, the generator omits the entire related text fragment (prefix/label phrase), instead of printing placeholders as empty strings.
Concrete example:
Actual (today):
- N/A: AbsaOSS/generate-release-notes#231 _Dependency Dashboard_ author is @renovate[bot] assigned to developed by inExpected (after fix):
- AbsaOSS/generate-release-notes#231 _Dependency Dashboard_ author is @renovate[bot]This should apply to issue rows (and, if applicable, hierarchy issue rows) generated by the Action’s row-format templates.
Problem / Opportunity
The generated release notes can contain redundant, confusing, or broken text segments when certain GitHub fields are missing:
N/A:(noise; looks like a bug).assigned tofollowed by nothing (dangling phrase).developed by in(dangling phrase).This reduces readability and professionalism of release notes and forces manual cleanup for otherwise valid issues (e.g., Renovate bot dashboard issues).
Who benefits:
Acceptance Criteria
N/Aand must not start withN/A:whenissue.typeis absent.{type}:prefix (or equivalent prefix fragment) must be omitted entirely.assigned toat all.developed bynorinfragments (nodeveloped by in).Proposed Solution
High-level approach:
.format()into strings that include constant phrases around empty placeholders.{type}is empty/missing → omit the entire “type prefix” fragment (not substituteN/A).{assignees}is empty → omit the entire “assigned to …” fragment.{developers}or{pull-requests}is empty → omit the entire “developed by … in …” fragment.row-format-issue)row-format-hierarchy-issue) where the same placeholders/fragments exist.Expected code touchpoints (permalinks):
N/Aand always emits potentially-empty fields:IssueRecord.to_chapter_row()generate-release-notes/release_notes_generator/model/record/issue_record.py
Lines 120 to 154 in cfb1544
type = "None") and empty-string risks:HierarchyIssueRecord.to_chapter_row()generate-release-notes/release_notes_generator/model/record/hierarchy_issue_record.py
Lines 108 to 139 in cfb1544
action.ymlrow-format defaultsgenerate-release-notes/action.yml
Lines 91 to 109 in cfb1544
Expected docs touchpoints:
docs/features/custom_row_formats.mdgenerate-release-notes/docs/features/custom_row_formats.md
Lines 1 to 24 in cfb1544
Expected tests:
tests/unit/release_notes_generator/builder/test_release_notes_builder.pygenerate-release-notes/tests/unit/release_notes_generator/builder/test_release_notes_builder.py
Lines 1491 to 1545 in cfb1544
Dependencies / Related
No response
Additional Context
Relevant reference docs / constants:
generate-release-notes/docs/features/custom_row_formats.md
Lines 1 to 24 in cfb1544
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.