fix: changelog-generator.sh の複雑度を 35 → 23 に低減 - #699
Conversation
主な変更: - 6 つの section 出力ブロック (BREAKING / FEATURES / FIXES / PERF / DOCS / OTHER) を共通 `render_section` 関数化。各ブロックの `if + for` を 6 重複から 1 つに集約 - 引数パースの単純な `--all`/`--include-all` を case の `|` で統合 - `format_message` を抽出し、PR 番号置換を sed → bash パラメータ展開 (SC2001 解消) - `categorize_commit` を抽出して種別ごとの代入を関数化 - `resolve_since_tag` 抽出 - nameref と `set -u` の組み合わせがバグるため、indirect expansion で配列を参照 機能変更なし、出力フォーマット変更なし。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors the changelog generator script by extracting repetitive logic into reusable functions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Claude finished @keito4's task in 1m 36s —— View job コードレビュー:
|
| 観点 | 評価 |
|---|---|
render_section による 6 重複ブロックの統合 |
✓ 明確な複雑度削減 |
--all|--include-all の case 統合 |
✓ KISS 原則に沿った簡素化 |
categorize_commit の抽出 |
✓ 単一責任の明確化 |
sed → bash パラメータ展開 (SC2001 解消) |
✓ shellcheck 準拠 |
resolve_since_tag の抽出 |
✓ グローバル状態の変更箇所が明確 |
set -euo pipefail との相互作用 |
✓ && で grep の非ゼロ終了を適切にハンドリング |
Verdict: APPROVE — minor / recommendation のみで、機能・安全性に影響する問題はありません。
🔍 AI Code Review (Local Hook)Models: Codex (default) / Gemini (default) ✨ Gemini ReviewMCP issues detected. Run /mcp list for status.## 指摘事項
全体的な正確性の判定: 理由: 信頼度スコア: 0.9 🤖 Generated by post_pr_ai_review.py hook |
|
(aside) |
|
Claude finished @keito4's task in 7m 6s —— View job CI修正作業 ✅
実施内容PR #700 (Windows ネイティブ環境サポート) が main にマージされたことで、このブランチが
現在の状況
CI が完了すれば |
![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 (1)
script/changelog-generator.sh (1)
113-129: Drop the deadsize_varand its no-op silencer.
size_varis defined (line 115) and immediately referenced via: "${size_var}"(line 128) only to suppress a shellcheck warning — it is never actually consumed; the section size is taken from${#keys[@]}on line 118. Removing both lines makes the intent clearer.♻️ Proposed cleanup
render_section() { local title=$1 name=$2 - local size_var="${name}[@]" local -a keys=() eval "keys=(\"\${!${name}[@]}\")" [[ ${`#keys`[@]} -eq 0 ]] && return 0 printf '### %s\n\n' "$title" local hash short msg val_var for hash in "${keys[@]}"; do short=${hash:0:7} val_var="${name}[$hash]" msg=$(format_message "${!val_var}") printf -- '- %s ([%s](%s/commit/%s))\n' "$msg" "$short" "$REPO_URL" "$hash" done printf '\n' - : "${size_var}" # silence shellcheck "unused" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelog-generator.sh` around lines 113 - 129, In render_section(), remove the unused local variable size_var and the trailing no-op silencer line ': "${size_var}"' so the function relies solely on keys array length; specifically delete the declaration "local size_var=\"${name}[@]\"" and the final ": \"${size_var}\"" reference, leaving the rest of render_section (including keys, loop, and printf) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/changelog-generator.sh`:
- Around line 75-91: The categorize_commit function can yield a non-zero exit
(via the case default branch when [[ "$INCLUDE_ALL" == "true" ]] is false) which
trips set -e and aborts the script; fix it by ensuring categorize_commit always
returns success: after the case block in categorize_commit (the case handling
feat/fix/perf/docs/* and writing to FEATURES/FIXES/PERF/DOCS/OTHER), add an
explicit success return (e.g., return 0) so that calls to categorize_commit
never propagate a non-zero status; reference categorize_commit, the case default
clause that writes to OTHER["$hash"], and the INCLUDE_ALL variable when making
the change.
---
Nitpick comments:
In `@script/changelog-generator.sh`:
- Around line 113-129: In render_section(), remove the unused local variable
size_var and the trailing no-op silencer line ': "${size_var}"' so the function
relies solely on keys array length; specifically delete the declaration "local
size_var=\"${name}[@]\"" and the final ": \"${size_var}\"" reference, leaving
the rest of render_section (including keys, loop, and printf) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8479ce20-109c-4527-ae7f-21d409d82c8b
📒 Files selected for processing (1)
script/changelog-generator.sh
| categorize_commit() { | ||
| local hash=$1 subject=$2 body=$3 | ||
| local regex='^([a-z]+)(\([^)]+\))?: (.+)$' | ||
| [[ "$subject" =~ $regex ]] || return 0 | ||
| local type="${BASH_REMATCH[1]}" | ||
| local message="${BASH_REMATCH[3]}" | ||
|
|
||
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Conventional Commits](https://conventionalcommits.org/). | ||
| echo "$body" | grep -q "BREAKING CHANGE" && BREAKING["$hash"]="$message" | ||
|
|
||
| " | ||
| case "$type" in | ||
| feat) FEATURES["$hash"]="$message" ;; | ||
| fix) FIXES["$hash"]="$message" ;; | ||
| perf) PERF["$hash"]="$message" ;; | ||
| docs) DOCS["$hash"]="$message" ;; | ||
| *) [[ "$INCLUDE_ALL" == "true" ]] && OTHER["$hash"]="$message ($type)" ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Repro: categorize_commit-style function returning non-zero in a while-loop body under set -e.
cat >/tmp/repro.sh <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
INCLUDE_ALL=false
declare -A OTHER
f() {
local type="chore" hash="$1" message="m"
case "$type" in
feat) :;;
*) [[ "$INCLUDE_ALL" == "true" ]] && OTHER["$hash"]="$message ($type)" ;;
esac
}
i=0
while IFS= read -r line; do
i=$((i+1))
f "$i"
echo "processed: $line"
done <<< $'a\nb\nc'
echo "done, processed=$i"
EOF
bash /tmp/repro.sh; echo "exit=$?"Repository: keito4/config
Length of output: 64
🏁 Script executed:
cat -n script/changelog-generator.sh | sed -n '75,91p'Repository: keito4/config
Length of output: 737
🏁 Script executed:
cat -n script/changelog-generator.sh | head -5 && echo "---" && cat -n script/changelog-generator.sh | sed -n '97,105p'Repository: keito4/config
Length of output: 459
🏁 Script executed:
if [ -f test/integration/development-tools.bats ]; then
cat -n test/integration/development-tools.bats | sed -n '35,52p'
else
find . -name "development-tools.bats" -o -name "*changelog*test*" -o -name "*changelog*bats" 2>/dev/null | head -5
fiRepository: keito4/config
Length of output: 700
categorize_commit can return non-zero and trip set -e, aborting changelog generation early.
The last statement of categorize_commit is the case, whose exit status is whatever the matched clause returns. The default branch on line 89 is an &&-list:
*) [[ "$INCLUDE_ALL" == "true" ]] && OTHER["$hash"]="$message ($type)" ;;
When INCLUDE_ALL=false (the default) and $type is anything other than feat|fix|perf|docs (e.g. chore, refactor, style, ci, build, test, revert), the [[ ]] test fails, the &&-list returns 1, the case returns 1, and the function returns 1. The call site at line 100 sits in the body of a while loop (not in a test, &&/||, or ! context), so set -e (line 4) propagates that failure and the script exits before build_changelog ever runs — silently skipping any tail of commits and writing a truncated CHANGELOG.md.
The integration test at test/integration/development-tools.bats:35-52 only exercises feat: commits, so it doesn't catch this.
Suggested fix: ensure the function always returns 0
categorize_commit() {
local hash=$1 subject=$2 body=$3
local regex='^([a-z]+)(\([^)]+\))?: (.+)$'
[[ "$subject" =~ $regex ]] || return 0
local type="${BASH_REMATCH[1]}"
local message="${BASH_REMATCH[3]}"
- echo "$body" | grep -q "BREAKING CHANGE" && BREAKING["$hash"]="$message"
+ if [[ "$body" == *"BREAKING CHANGE"* ]]; then
+ BREAKING["$hash"]="$message"
+ fi
case "$type" in
feat) FEATURES["$hash"]="$message" ;;
fix) FIXES["$hash"]="$message" ;;
perf) PERF["$hash"]="$message" ;;
docs) DOCS["$hash"]="$message" ;;
- *) [[ "$INCLUDE_ALL" == "true" ]] && OTHER["$hash"]="$message ($type)" ;;
+ *) if [[ "$INCLUDE_ALL" == "true" ]]; then OTHER["$hash"]="$message ($type)"; fi ;;
esac
+ return 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| categorize_commit() { | |
| local hash=$1 subject=$2 body=$3 | |
| local regex='^([a-z]+)(\([^)]+\))?: (.+)$' | |
| [[ "$subject" =~ $regex ]] || return 0 | |
| local type="${BASH_REMATCH[1]}" | |
| local message="${BASH_REMATCH[3]}" | |
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |
| and this project adheres to [Conventional Commits](https://conventionalcommits.org/). | |
| echo "$body" | grep -q "BREAKING CHANGE" && BREAKING["$hash"]="$message" | |
| " | |
| case "$type" in | |
| feat) FEATURES["$hash"]="$message" ;; | |
| fix) FIXES["$hash"]="$message" ;; | |
| perf) PERF["$hash"]="$message" ;; | |
| docs) DOCS["$hash"]="$message" ;; | |
| *) [[ "$INCLUDE_ALL" == "true" ]] && OTHER["$hash"]="$message ($type)" ;; | |
| esac | |
| } | |
| categorize_commit() { | |
| local hash=$1 subject=$2 body=$3 | |
| local regex='^([a-z]+)(\([^)]+\))?: (.+)$' | |
| [[ "$subject" =~ $regex ]] || return 0 | |
| local type="${BASH_REMATCH[1]}" | |
| local message="${BASH_REMATCH[3]}" | |
| if [[ "$body" == *"BREAKING CHANGE"* ]]; then | |
| BREAKING["$hash"]="$message" | |
| fi | |
| case "$type" in | |
| feat) FEATURES["$hash"]="$message" ;; | |
| fix) FIXES["$hash"]="$message" ;; | |
| perf) PERF["$hash"]="$message" ;; | |
| docs) DOCS["$hash"]="$message" ;; | |
| *) if [[ "$INCLUDE_ALL" == "true" ]]; then OTHER["$hash"]="$message ($type)"; fi ;; | |
| esac | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelog-generator.sh` around lines 75 - 91, The categorize_commit
function can yield a non-zero exit (via the case default branch when [[
"$INCLUDE_ALL" == "true" ]] is false) which trips set -e and aborts the script;
fix it by ensuring categorize_commit always returns success: after the case
block in categorize_commit (the case handling feat/fix/perf/docs/* and writing
to FEATURES/FIXES/PERF/DOCS/OTHER), add an explicit success return (e.g., return
0) so that calls to categorize_commit never propagate a non-zero status;
reference categorize_commit, the case default clause that writes to
OTHER["$hash"], and the INCLUDE_ALL variable when making the change.
|
🎉 This PR is included in version 1.111.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
script/changelog-generator.shのリファクタリングで循環的複雑度を 35 → 23 に低減（Issue #646 に部分対応）。主な変更
render_section関数化。各ブロックのif + forを 6 重複から 1 つに集約--all/--include-allを case の|で統合format_messageを抽出し、PR 番号置換を sed → bash パラメータ展開 (SC2001 解消)categorize_commitを抽出して種別ごとの代入を関数化resolve_since_tagを抽出set -uの組み合わせはバグるため、indirect expansion で配列参照機能変更なし、出力フォーマット変更なし。
Test plan
npm run shellcheck緑npm run lint緑npm test95 件パスbash script/changelog-generator.sh --since 7d93477 --include-all --dry-runで旧版と同じ出力フォーマット🤖 Generated with Claude Code
Summary by CodeRabbit