Draft
Conversation
WalkthroughAdds two optional action inputs ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for super chapters to the release notes generator, allowing users to group existing custom chapters under higher-level ## headings based on labels, while preserving the original flat ### rendering when the feature is not configured.
Changes:
- Introduces a new optional
super-chaptersaction input (YAML) and wires it throughActionInputs. - Extends
CustomChaptersrendering to support super-chapter grouping plus an## Uncategorizedfallback. - Adds unit tests and updates documentation/action metadata for the new input.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
action.yml |
Adds the super-chapters input and exports it as INPUT_SUPER_CHAPTERS for runtime consumption. |
release_notes_generator/action_inputs.py |
Implements ActionInputs.get_super_chapters() and logs parsed super-chapter config during validation. |
release_notes_generator/chapters/custom_chapters.py |
Adds super-chapter parsing and grouped rendering logic with an Uncategorized fallback. |
release_notes_generator/utils/constants.py |
Adds SUPER_CHAPTERS input key constant and extracts UNCATEGORIZED_CHAPTER_TITLE. |
docs/features/custom_chapters.md |
Documents configuration, rendering, behavior, and validation rules for super chapters. |
docs/configuration_reference.md |
Adds super-chapters to the inputs reference table. |
tests/unit/release_notes_generator/test_action_inputs.py |
Adds unit tests covering parsing/validation behavior for get_super_chapters(). |
tests/unit/release_notes_generator/chapters/test_custom_chapters.py |
Adds unit tests for grouped super-chapter output behavior and fallback behavior. |
tests/unit/conftest.py |
Adds a helper to construct CustomChapters instances with super-chapter behavior patched in tests. |
![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
🧹 Nitpick comments (2)
action.yml (1)
33-38: Consider documenting uncategorized fallback in the input description.You already document matching and multi-membership; adding one line about the automatic
Uncategorizedsection would make behavior complete directly inaction.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 33 - 38, Update the description for the super-chapters input in action.yml to mention the automatic fallback: add a sentence to the existing description block (the `description` key for the super-chapter array) stating that any records not matching any super-chapter label will be placed into an automatic "Uncategorized" section; keep it brief and match the tone of the surrounding bullets.release_notes_generator/chapters/custom_chapters.py (1)
235-244: Temporary mutation ofchapter.rowsis a code smell.While the
try/finallyensures restoration, directly mutating shared state can be fragile and isn't thread-safe. Consider either passing filter criteria toChapter.to_stringor creating a lightweight wrapper that yields filtered rows without mutation.Also, the dual check
str(rid) in matching_ids or rid in matching_idssuggests type inconsistency between row IDs (possibly int) andmatching_ids(always str). Consider normalizing ID types upstream for cleaner comparisons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release_notes_generator/chapters/custom_chapters.py` around lines 235 - 244, The helper _render_chapter_for_ids currently mutates chapter.rows; instead avoid shared-state mutation by building a filtered_rows dict and invoking Chapter.to_string on a non-mutated object—either (A) add an optional parameter to Chapter.to_string like rows/filter_ids and pass filtered_rows, or (B) create a lightweight shallow copy of the Chapter (e.g., new_chapter = Chapter(...same metadata..., rows=filtered_rows) or add a Chapter.with_rows(rows) factory) and call new_chapter.to_string; also normalize ID comparisons by converting matching_ids to a consistent type (e.g., set[str] = {str(x) for x in matching_ids}) or casting rid once when creating filtered_rows so you can remove the dual check (str(rid) in matching_ids or rid in matching_ids).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/custom_chapters.md`:
- Line 258: Update the sentence "Within each super chapter, records are routed
to regular chapters by the normal label matching rules." to use a hyphenated
compound adjective for clarity — change "label matching rules" to
"label-matching rules" so the line reads: "Within each super chapter, records
are routed to regular chapters by the normal label-matching rules."
In `@release_notes_generator/action_inputs.py`:
- Around line 175-195: get_super_chapters currently only YAML-parses the input
and leaves semantic validation to CustomChapters._parse_super_chapters; move
that full validation into the ActionInputs boundary by having get_super_chapters
perform the same semantic checks and return only validated structures
(list[dict[str,str]]). Specifically, extend get_super_chapters to validate each
entry's type and required keys/fields (the same contract enforced in
CustomChapters._parse_super_chapters), log and skip or error on invalid entries
consistently, and ensure the return value is strictly the validated list so
CustomChapters no longer needs to re-validate; update/remove the redundant
checks in CustomChapters._parse_super_chapters accordingly.
In `@release_notes_generator/chapters/custom_chapters.py`:
- Around line 367-399: Change the _parse_super_chapters signature to accept
mixed YAML types (use list[dict[str, Any]] for raw_super_chapters) and validate
the title before using it: after extracting entry["title"] in
_parse_super_chapters, ensure it's a str (if not, logger.warning and continue),
and keep using SuperChapter(title=title, labels=...) and _normalize_labels as
before; also adjust the local typing for labels_input to reflect str | list[str]
if you keep annotations.
---
Nitpick comments:
In `@action.yml`:
- Around line 33-38: Update the description for the super-chapters input in
action.yml to mention the automatic fallback: add a sentence to the existing
description block (the `description` key for the super-chapter array) stating
that any records not matching any super-chapter label will be placed into an
automatic "Uncategorized" section; keep it brief and match the tone of the
surrounding bullets.
In `@release_notes_generator/chapters/custom_chapters.py`:
- Around line 235-244: The helper _render_chapter_for_ids currently mutates
chapter.rows; instead avoid shared-state mutation by building a filtered_rows
dict and invoking Chapter.to_string on a non-mutated object—either (A) add an
optional parameter to Chapter.to_string like rows/filter_ids and pass
filtered_rows, or (B) create a lightweight shallow copy of the Chapter (e.g.,
new_chapter = Chapter(...same metadata..., rows=filtered_rows) or add a
Chapter.with_rows(rows) factory) and call new_chapter.to_string; also normalize
ID comparisons by converting matching_ids to a consistent type (e.g., set[str] =
{str(x) for x in matching_ids}) or casting rid once when creating filtered_rows
so you can remove the dual check (str(rid) in matching_ids or rid in
matching_ids).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b625f1e8-a06c-426d-9639-b30f9cd8a5dc
📒 Files selected for processing (9)
action.ymldocs/configuration_reference.mddocs/features/custom_chapters.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/chapters/custom_chapters.pyrelease_notes_generator/utils/constants.pytests/unit/conftest.pytests/unit/release_notes_generator/chapters/test_custom_chapters.pytests/unit/release_notes_generator/test_action_inputs.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@release_notes_generator/action_inputs.py`:
- Around line 208-223: The code currently accepts any non-missing "title" value
which may be non-string or blank; update the validation around entry["title"] in
the super-chapter parsing block so that you only accept a title that is a
non-empty string (e.g., check isinstance(title, str) and title.strip() != ""),
log a warning and continue if the title is invalid, and only pass the
cleaned/stripped title into result.append({"title": title, "labels":
normalized}) (keep using normalize_labels and the existing logging patterns).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f83123e6-7884-4ada-81f2-3e9f9139277a
📒 Files selected for processing (7)
docs/features/custom_chapters.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/chapters/custom_chapters.pyrelease_notes_generator/utils/utils.pytests/unit/conftest.pytests/unit/release_notes_generator/chapters/test_custom_chapters.pytests/unit/release_notes_generator/test_action_inputs.py
✅ Files skipped from review due to trivial changes (1)
- docs/features/custom_chapters.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/release_notes_generator/test_action_inputs.py
- tests/unit/conftest.py
- tests/unit/release_notes_generator/chapters/test_custom_chapters.py
- release_notes_generator/chapters/custom_chapters.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
release_notes_generator/action_inputs.py (1)
211-226:⚠️ Potential issue | 🟡 MinorStrip the validated title before appending.
Line 212 validates that
title.strip()is non-empty, but Line 226 appends the originaltitlevalue with potential leading/trailing whitespace. This could produce headings like" Module A "in the release notes.Proposed fix
title = entry["title"] if not isinstance(title, str) or not title.strip(): logger.warning("Skipping super-chapter with invalid title value: %r", title) continue + title = title.strip() raw_labels = entry.get("labels", entry.get("label"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release_notes_generator/action_inputs.py` around lines 211 - 226, The validated title may contain leading/trailing whitespace—after confirming title.strip() is non-empty, replace or assign a stripped version (e.g., title = title.strip() or use title_stripped) before appending; update the code that builds the output (the result.append({"title": title, "labels": normalized}) call) to use the stripped title so headings are normalized, leaving label normalization via normalize_labels unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@release_notes_generator/action_inputs.py`:
- Around line 211-226: The validated title may contain leading/trailing
whitespace—after confirming title.strip() is non-empty, replace or assign a
stripped version (e.g., title = title.strip() or use title_stripped) before
appending; update the code that builds the output (the result.append({"title":
title, "labels": normalized}) call) to use the stripped title so headings are
normalized, leaving label normalization via normalize_labels unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4be0854c-caad-4b3c-8aab-3e5bef3e0bb2
📒 Files selected for processing (2)
release_notes_generator/action_inputs.pytests/unit/release_notes_generator/test_action_inputs.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/release_notes_generator/test_action_inputs.py
…ents (#280) * feat: enhance hierarchy issue rendering logic for open and closed parents * feat: add support for highlighting open sub-issues under closed hierarchy parents
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unit/release_notes_generator/utils/test_gh_action.py (1)
22-36: Cover the explicitdefault=path too.These assertions lock in the implicit
default=""call, but the contract change also depends on forwarding a caller-supplied default unchanged. A smallget_action_input("explicit-default", "fallback")case would keep that branch covered.🧪 Suggested test
+def test_get_input_with_explicit_default(mocker): + mock_getenv = mocker.patch("os.getenv", side_effect=lambda name, default=None: default) + + result = get_action_input("explicit-default", "fallback") + + mock_getenv.assert_called_with("INPUT_EXPLICIT_DEFAULT", default="fallback") + assert result == "fallback"As per coding guidelines, "Test return values, exceptions, log messages, and exit codes in Python tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/release_notes_generator/utils/test_gh_action.py` around lines 22 - 36, Add a test that exercises the explicit default path for get_action_input: call get_action_input("explicit-default", "fallback") in tests/unit/release_notes_generator/utils/test_gh_action.py, mock os.getenv and assert it was called with the environment key "INPUT_EXPLICIT_DEFAULT" and default="fallback", and assert the function returns the mocked value; reference get_action_input to locate the implementation and the existing tests for pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/custom_chapters.md`:
- Around line 218-315: The docs duplicate the entire "## Super Chapters" section
back-to-back; remove the second copy so there's a single consolidated "## Super
Chapters" section (look for the repeated "## Super Chapters" heading). In that
remaining section add a short sentence to Rendering or Behavior stating the
fallback: "Records that do not match any super-chapter labels are grouped under
## Uncategorized." Also ensure the Validation paragraph still appears once and
mentions skipped entries as before.
In `@release_notes_generator/chapters/custom_chapters.py`:
- Around line 421-425: The constructor is re-parsing already-validated
super-chapter entries causing duplicated validation; replace the duplicate calls
so self._super_chapters is assigned the validated, normalized output from
ActionInputs.get_super_chapters() only (do not call _parse_super_chapters on
that value), remove the redundant parsing calls around the constructor and the
block referenced (including the repeated section covering the 429-474 region),
and leave the standalone _parse_super_chapters method only for the original
raw-YAML parsing path if still needed; target symbols: self._super_chapters,
_parse_super_chapters, and ActionInputs.get_super_chapters.
- Around line 91-99: The class contains unresolved duplicate members—remove the
later duplicates and keep only the first occurrence of each: delete the second
__init__ duplicate and the later definitions of _to_string_with_super_chapters
and _parse_super_chapters; ensure the kept _to_string_with_super_chapters is the
first version that preserves the "## Uncategorized" fallback logic so unmatched
records are collected, and ensure the kept _parse_super_chapters is the first
version that does not re-validate/normalize input (leaving validation to
ActionInputs.get_super_chapters()); update imports/types only if needed after
removing the duplicates.
In `@release_notes_generator/model/record/hierarchy_issue_record.py`:
- Around line 225-254: The code that prefixes open-child icons currently
prepends the icon before the child's existing "- " list marker (see
sub_hierarchy_issue.to_chapter_row() handling and the open_icon_prefix /
sub_issue_block construction), which breaks nested Markdown and leaves stray
spaces when the icon is empty; change the logic to detect and preserve an
existing leading list marker (the initial "-" and following space) and insert
the icon immediately after that marker (e.g., "- " -> "- {icon} " only when icon
non-empty, otherwise leave as "- "), update both the sub_hierarchy_issue branch
and the sub_issue block where open_icon_prefix is used
(ActionInputs.get_open_hierarchy_sub_issue_icon()), and add regression
assertions that verify the rendered output begins with "- 🟡" when icon set and
remains "- " when icon is empty.
---
Nitpick comments:
In `@tests/unit/release_notes_generator/utils/test_gh_action.py`:
- Around line 22-36: Add a test that exercises the explicit default path for
get_action_input: call get_action_input("explicit-default", "fallback") in
tests/unit/release_notes_generator/utils/test_gh_action.py, mock os.getenv and
assert it was called with the environment key "INPUT_EXPLICIT_DEFAULT" and
default="fallback", and assert the function returns the mocked value; reference
get_action_input to locate the implementation and the existing tests for
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4d7d817-034a-47e2-933f-b6b4ec8142c4
📒 Files selected for processing (11)
action.ymldocs/features/custom_chapters.mddocs/features/issue_hierarchy_support.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/chapters/custom_chapters.pyrelease_notes_generator/model/record/hierarchy_issue_record.pyrelease_notes_generator/utils/constants.pyrelease_notes_generator/utils/gh_action.pytests/unit/conftest.pytests/unit/release_notes_generator/model/test_hierarchy_issue_record.pytests/unit/release_notes_generator/utils/test_gh_action.py
🚧 Files skipped from review as they are similar to previous changes (4)
- action.yml
- release_notes_generator/action_inputs.py
- release_notes_generator/utils/constants.py
- tests/unit/conftest.py
Comment on lines
+218
to
+315
| ## Super Chapters | ||
|
|
||
| **Super chapters** group regular chapters under higher-level headings based on a separate label. This is useful in monorepo or multi-module projects where the same chapter structure (Features, Bugfixes, …) should appear once per component. | ||
|
|
||
| ### Configuration | ||
|
|
||
| Define super chapters via the `super-chapters` input — a YAML array with `title` and `label`/`labels`: | ||
|
|
||
| ```yaml | ||
| with: | ||
| super-chapters: | | ||
| - title: "Atum Server" | ||
| label: "atum-server" | ||
| - title: "Atum Agent" | ||
| labels: "atum-agent, atum-agent-spark" | ||
| chapters: | | ||
| - {"title": "Enhancements", "label": "enhancement"} | ||
| - {"title": "Bugfixes", "label": "bug"} | ||
| ``` | ||
|
|
||
| ### Rendering | ||
|
|
||
| When super chapters are configured the output uses `##` headings for super chapters and `###` for regular chapters nested inside: | ||
|
|
||
| ```markdown | ||
| ## Atum Server | ||
| ### Enhancements | ||
| - #10 _Streaming API_ developed by @alice in #12 | ||
|
|
||
| ### Bugfixes | ||
| - #15 _Fix timeout_ developed by @bob in #16 | ||
|
|
||
| ## Atum Agent | ||
| ### Enhancements | ||
| - #20 _Checkpointing_ developed by @carol in #22 | ||
| ``` | ||
|
|
||
| ### Behavior | ||
| - A record is placed under a super-chapter if it carries at least one label matching the super-chapter's labels. | ||
| - A record can appear in **multiple** super-chapters if its labels match more than one. | ||
| - Within each super-chapter, records are routed to regular chapters by the normal label-matching rules. | ||
| - Empty super chapters (no matching records) respect the `print-empty-chapters` setting. | ||
| - When no super chapters are configured, output is flat (unchanged from previous behavior). | ||
|
|
||
| ### Validation | ||
| - Entries missing `title` or `label`/`labels` are skipped with a warning. | ||
| - Non-dict entries are skipped with a warning. | ||
| - Empty labels after normalization cause the entry to be skipped with a warning. | ||
|
|
||
| ## Super Chapters | ||
|
|
||
| **Super chapters** group regular chapters under higher-level headings based on a separate label. This is useful in monorepo or multi-module projects where the same chapter structure (Features, Bugfixes, …) should appear once per component. | ||
|
|
||
| ### Configuration | ||
|
|
||
| Define super chapters via the `super-chapters` input — a YAML array with `title` and `label`/`labels`: | ||
|
|
||
| ```yaml | ||
| with: | ||
| super-chapters: | | ||
| - title: "Atum Server" | ||
| label: "atum-server" | ||
| - title: "Atum Agent" | ||
| labels: "atum-agent, atum-agent-spark" | ||
| chapters: | | ||
| - {"title": "Enhancements", "label": "enhancement"} | ||
| - {"title": "Bugfixes", "label": "bug"} | ||
| ``` | ||
|
|
||
| ### Rendering | ||
|
|
||
| When super chapters are configured the output uses `##` headings for super chapters and `###` for regular chapters nested inside: | ||
|
|
||
| ```markdown | ||
| ## Atum Server | ||
| ### Enhancements | ||
| - #10 _Streaming API_ developed by @alice in #12 | ||
|
|
||
| ### Bugfixes | ||
| - #15 _Fix timeout_ developed by @bob in #16 | ||
|
|
||
| ## Atum Agent | ||
| ### Enhancements | ||
| - #20 _Checkpointing_ developed by @carol in #22 | ||
| ``` | ||
|
|
||
| ### Behavior | ||
| - A record is placed under a super chapter if it carries at least one label matching the super chapter's labels. | ||
| - A record can appear in **multiple** super chapters if its labels match more than one. | ||
| - Within each super chapter, records are routed to regular chapters by the normal label matching rules. | ||
| - Empty super chapters (no matching records) respect the `print-empty-chapters` setting. | ||
| - When no super chapters are configured, output is flat (unchanged from previous behavior). | ||
|
|
||
| ### Validation | ||
| - Entries missing `title` or `label`/`labels` are skipped with a warning. | ||
| - Non-dict entries are skipped with a warning. | ||
| - Empty labels after normalization cause the entry to be skipped with a warning. | ||
|
|
There was a problem hiding this comment.
Consolidate the Super Chapters docs and mention the fallback.
This section is duplicated back-to-back, and neither copy tells users that records missing every super-chapter label are grouped under ## Uncategorized. That fallback is part of the new visible behavior, so it should be documented once in the surviving section.
🧰 Tools
🪛 LanguageTool
[grammar] ~307-~307: Use a hyphen to join words.
Context: ... to regular chapters by the normal label matching rules. - Empty super chapters (...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features/custom_chapters.md` around lines 218 - 315, The docs duplicate
the entire "## Super Chapters" section back-to-back; remove the second copy so
there's a single consolidated "## Super Chapters" section (look for the repeated
"## Super Chapters" heading). In that remaining section add a short sentence to
Rendering or Behavior stating the fallback: "Records that do not match any
super-chapter labels are grouped under ## Uncategorized." Also ensure the
Validation paragraph still appears once and mentions skipped entries as before.
Comment on lines
+91
to
+99
| def __init__(self, sort_ascending: bool = True, print_empty_chapters: bool = True): | ||
| super().__init__(sort_ascending, print_empty_chapters) | ||
| self._super_chapters: list[SuperChapter] = [] | ||
| self._record_labels: dict[str, list[str]] = {} | ||
|
|
||
| def __init__(self, sort_ascending: bool = True, print_empty_chapters: bool = True): | ||
| super().__init__(sort_ascending, print_empty_chapters) | ||
| self._super_chapters: list[SuperChapter] = [] | ||
| self._record_labels: dict[str, list[str]] = {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '^\s*def (__init__|_to_string_flat|_to_string_with_super_chapters|_parse_super_chapters)\b' release_notes_generator/chapters/custom_chapters.pyRepository: AbsaOSS/generate-release-notes
Length of output: 642
🏁 Script executed:
#!/bin/bash
# Get the content of both __init__ definitions
echo "=== __init__ definitions ==="
sed -n '91,95p' release_notes_generator/chapters/custom_chapters.py
echo "---"
sed -n '96,100p' release_notes_generator/chapters/custom_chapters.py
# Get the content of _to_string_with_super_chapters to check for uncategorized fallback
echo ""
echo "=== _to_string_with_super_chapters (first at 269) ==="
sed -n '269,318p' release_notes_generator/chapters/custom_chapters.py
echo ""
echo "=== _to_string_with_super_chapters (second at 319) ==="
sed -n '319,353p' release_notes_generator/chapters/custom_chapters.pyRepository: AbsaOSS/generate-release-notes
Length of output: 4303
🏁 Script executed:
#!/bin/bash
# Check ActionInputs to verify validation scope
find . -name "*.py" -type f | xargs grep -l "class ActionInputs" | head -1Repository: AbsaOSS/generate-release-notes
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check _parse_super_chapters implementations for validation duplication
echo "=== _parse_super_chapters (first at 430) ==="
sed -n '430,441p' release_notes_generator/chapters/custom_chapters.py
echo ""
echo "=== _parse_super_chapters (second at 442) ==="
sed -n '442,460p' release_notes_generator/chapters/custom_chapters.pyRepository: AbsaOSS/generate-release-notes
Length of output: 1495
🏁 Script executed:
#!/bin/bash
# Check ActionInputs.get_super_chapters() to verify validation scope
cat release_notes_generator/action_inputs.py | head -100Repository: AbsaOSS/generate-release-notes
Length of output: 2939
🏁 Script executed:
#!/bin/bash
# Get the full ActionInputs class to find get_super_chapters method
rg -A 30 "def get_super_chapters" release_notes_generator/action_inputs.pyRepository: AbsaOSS/generate-release-notes
Length of output: 1421
🏁 Script executed:
#!/bin/bash
# Check where _to_string_with_super_chapters is called to see which version is used
rg -n "_to_string_with_super_chapters\(\)" release_notes_generator/chapters/custom_chapters.pyRepository: AbsaOSS/generate-release-notes
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Check where _parse_super_chapters is called to see which version is active
rg -n "_parse_super_chapters" release_notes_generator/chapters/custom_chapters.pyRepository: AbsaOSS/generate-release-notes
Length of output: 456
Remove the duplicated class members before merge.
These are unresolved merge artifacts. CI is failing with mypy: no-redef, and Python silently keeps only the last definition of each method:
__init__(lines 91, 96): Identical definitions; later shadows earlier._to_string_with_super_chapters()(lines 269–318 vs 319–353): The later version removes the uncategorized fallback logic, causing unmatched records to disappear from grouped output instead of being collected under## Uncategorized._parse_super_chapters()(lines 430–435 vs 442+): The later version re-validates input thatActionInputs.get_super_chapters()already validates and normalizes, violating the guideline to centralize parsing/validation in the input layer.
Remove all duplicates and keep only the first occurrence of each method.
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 96-96: mypy: Name "init" already defined on line 91 [no-redef]
[error] 98-98: mypy: Attribute "_super_chapters" already defined on line 93 [no-redef]
[error] 99-99: mypy: Attribute "_record_labels" already defined on line 94 [no-redef]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@release_notes_generator/chapters/custom_chapters.py` around lines 91 - 99,
The class contains unresolved duplicate members—remove the later duplicates and
keep only the first occurrence of each: delete the second __init__ duplicate and
the later definitions of _to_string_with_super_chapters and
_parse_super_chapters; ensure the kept _to_string_with_super_chapters is the
first version that preserves the "## Uncategorized" fallback logic so unmatched
records are collected, and ensure the kept _parse_super_chapters is the first
version that does not re-validate/normalize input (leaving validation to
ActionInputs.get_super_chapters()); update imports/types only if needed after
removing the duplicates.
Comment on lines
+421
to
+425
| # Parse super-chapter definitions from action inputs | ||
| self._super_chapters = self._parse_super_chapters(ActionInputs.get_super_chapters()) | ||
|
|
||
| # Parse super-chapter definitions from action inputs | ||
| self._super_chapters = self._parse_super_chapters(ActionInputs.get_super_chapters()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep super-chapter validation in ActionInputs only.
ActionInputs.get_super_chapters() already returns validated, normalized {title, labels} entries. Re-parsing them again here duplicates the input contract and makes the rendering layer responsible for validation drift. I'd keep the constructor-only _parse_super_chapters() and delete the raw-YAML version.
As per coding guidelines, "Centralize parsing and validation in one input layer (ActionInputs with INPUT_* env vars)" and "Must not duplicate validation across modules in Python code".
Also applies to: 429-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@release_notes_generator/chapters/custom_chapters.py` around lines 421 - 425,
The constructor is re-parsing already-validated super-chapter entries causing
duplicated validation; replace the duplicate calls so self._super_chapters is
assigned the validated, normalized output from ActionInputs.get_super_chapters()
only (do not call _parse_super_chapters on that value), remove the redundant
parsing calls around the constructor and the block referenced (including the
repeated section covering the 429-474 region), and leave the standalone
_parse_super_chapters method only for the original raw-YAML parsing path if
still needed; target symbols: self._super_chapters, _parse_super_chapters, and
ActionInputs.get_super_chapters.
Comment on lines
+225
to
+254
| if self.is_closed and sub_hierarchy_issue.is_open: | ||
| sub_row = sub_hierarchy_issue.to_chapter_row() | ||
| # Highlight open children under a closed parent to signal incomplete work | ||
| icon = ActionInputs.get_open_hierarchy_sub_issue_icon() | ||
| header_line, newline, remaining_lines = sub_row.partition("\n") | ||
| header_text = header_line.lstrip() | ||
| indent = header_line[: len(header_line) - len(header_text)] | ||
| sub_row = f"{indent}{icon} {header_text}{newline}{remaining_lines}" | ||
| else: | ||
| sub_row = sub_hierarchy_issue.to_chapter_row() | ||
| row = f"{row}\n{sub_row}" | ||
|
|
||
| # add sub-issues | ||
| if len(self._sub_issues) > 0: | ||
| sub_indent = " " * (self._level + 1) | ||
| for sub_issue in self._sub_issues.values(): | ||
| logger.debug("Rendering sub-issue row for issue #%d", sub_issue.issue.number) | ||
| if sub_issue.is_open: | ||
| continue # only closed issues are reported in release notes | ||
|
|
||
| if not sub_issue.contains_change_increment(): | ||
| continue # skip sub-issues without change increment | ||
|
|
||
| logger.debug("Sub-issue #%s contains change increment", sub_issue.issue.number) | ||
| sub_issue_block = "- " + sub_issue.to_chapter_row() | ||
| logger.debug("Rendering sub-issue row for issue #%s", sub_issue.issue.number) | ||
| if self.is_open: | ||
| if sub_issue.is_open: | ||
| continue # only closed issues are reported in release notes | ||
| if not sub_issue.contains_change_increment(): | ||
| continue # skip sub-issues without change increment | ||
| # Closed parent: render all sub-issues regardless of state or change increment | ||
|
|
||
| logger.debug("Rendering sub-issue #%s", sub_issue.issue.number) | ||
| open_icon_prefix = "" | ||
| if self.is_closed and sub_issue.is_open: | ||
| # Highlight open children under a closed parent to signal incomplete work | ||
| open_icon_prefix = ActionInputs.get_open_hierarchy_sub_issue_icon() + " " | ||
| sub_issue_block = "- " + open_icon_prefix + sub_issue.to_chapter_row() |
There was a problem hiding this comment.
Preserve the list marker when highlighting open children.
Lines 225-232 currently rebuild nested hierarchy rows as 🟡 - ... once the child row already starts with - . That means the first non-space character is no longer the list marker, so the nested Markdown structure breaks and the rendered output no longer matches the documented - 🟡 ... format. The same code path also leaves stray spaces when open-hierarchy-sub-issue-icon is set to "", so “disable highlighting” still changes the output. Please insert the icon after the list marker and make the trailing space conditional. Also worth adding a regression assertion for both - 🟡 ordering and the empty-icon case when you patch this.
💡 Suggested fix
- if self.is_closed and sub_hierarchy_issue.is_open:
- sub_row = sub_hierarchy_issue.to_chapter_row()
- # Highlight open children under a closed parent to signal incomplete work
- icon = ActionInputs.get_open_hierarchy_sub_issue_icon()
- header_line, newline, remaining_lines = sub_row.partition("\n")
- header_text = header_line.lstrip()
- indent = header_line[: len(header_line) - len(header_text)]
- sub_row = f"{indent}{icon} {header_text}{newline}{remaining_lines}"
- else:
- sub_row = sub_hierarchy_issue.to_chapter_row()
+ sub_row = sub_hierarchy_issue.to_chapter_row()
+ if self.is_closed and sub_hierarchy_issue.is_open:
+ # Highlight open children under a closed parent to signal incomplete work
+ icon = ActionInputs.get_open_hierarchy_sub_issue_icon()
+ icon_prefix = f"{icon} " if icon else ""
+ header_line, newline, remaining_lines = sub_row.partition("\n")
+ header_text = header_line.lstrip()
+ indent = header_line[: len(header_line) - len(header_text)]
+ if header_text.startswith("- "):
+ header_text = f"- {icon_prefix}{header_text[2:]}"
+ else:
+ header_text = f"{icon_prefix}{header_text}"
+ sub_row = f"{indent}{header_text}{newline}{remaining_lines}"- open_icon_prefix = ""
- if self.is_closed and sub_issue.is_open:
- # Highlight open children under a closed parent to signal incomplete work
- open_icon_prefix = ActionInputs.get_open_hierarchy_sub_issue_icon() + " "
+ open_icon_prefix = ""
+ if self.is_closed and sub_issue.is_open:
+ # Highlight open children under a closed parent to signal incomplete work
+ icon = ActionInputs.get_open_hierarchy_sub_issue_icon()
+ open_icon_prefix = f"{icon} " if icon else ""
sub_issue_block = "- " + open_icon_prefix + sub_issue.to_chapter_row()As per coding guidelines, "Keep externally-visible strings, formats, and exit codes stable unless intentional in Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@release_notes_generator/model/record/hierarchy_issue_record.py` around lines
225 - 254, The code that prefixes open-child icons currently prepends the icon
before the child's existing "- " list marker (see
sub_hierarchy_issue.to_chapter_row() handling and the open_icon_prefix /
sub_issue_block construction), which breaks nested Markdown and leaves stray
spaces when the icon is empty; change the logic to detect and preserve an
existing leading list marker (the initial "-" and following space) and insert
the icon immediately after that marker (e.g., "- " -> "- {icon} " only when icon
non-empty, otherwise leave as "- "), update both the sub_hierarchy_issue branch
and the sub_issue block where open_icon_prefix is used
(ActionInputs.get_open_hierarchy_sub_issue_icon()), and add regression
assertions that verify the rendered output begins with "- 🟡" when icon set and
remains "- " when icon is empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds
super-chaptersas a new optional input that lets users group existing custom chapters under top-level##headings in the release notes output.Previously, all custom chapters rendered as flat
###headings. With super chapters, users can declare label-based groupings (e.g. "Module A", "Module B") that wrap the matching records under a##heading, with the regular chapter titles nested as###inside. Records not matched by any super chapter fall through to an## Uncategorizedsection.Key changes:
ActionInputs.get_super_chapters()parses the newsuper-chaptersYAML input (mirrorsget_chapters()pattern)CustomChapters._to_string_with_super_chapters()refactored into focused helpers and extended with an uncategorized fallback_record_labelsnow tracks all records (including label-less COH-routed ones) so they appear in the fallbackUNCATEGORIZED_CHAPTER_TITLEconstant extracted to keep the contract-visible string stableRelease Notes
super-chaptersinput: group release note chapters under top-level##headings by label## Uncategorizedfallback sectionRelated
Closes #99
Summary by CodeRabbit
New Features
Documentation
Tests