fix: 型安全性の改善 (#787) - #788
Conversation
- script/lib/brew_categories.py: 型アノテーション完全追加 - from typing import Any を追加 - load_manifest: dict -> dict[str, Any] - matches: dict -> dict[str, Any] - categorize: 引数・戻り値の型アノテーション追加 - emit_human/emit_brew: 型アノテーション追加 - main: 戻り値 -> None を追加 - commitlint.config.js: JSDoc型アノテーション追加 - getStagedFiles: @returns {string[]} 追加 - releaseTypeRule: @param/@returns 追加 Closes #787 Co-authored-by: keito4 <keito4@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR improves type safety across two utility files. JSDoc comments document helper function contracts in ChangesJavaScript Documentation
Python Type Safety
🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 9s —— View job コードレビュー: fix: 型安全性の改善
全体として型安全性を向上させる良い変更です。2点ほど確認事項があります。
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a600d322b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def load_manifest(path: Path) -> dict: | ||
| def load_manifest(path: Path) -> dict[str, Any]: |
There was a problem hiding this comment.
When brew-deps.sh runs on a machine where python3 is still Python 3.8, this annotation is evaluated at import time and dict[str, Any] raises TypeError: 'type' object is not subscriptable before the script can do any work. The wrapper only checks that python3 exists and the docs advertise this as a generic Homebrew helper, so this change regresses users with older but still common python3 installations unless annotations are postponed or typing.Dict/List style is used.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commitlint.config.js (1)
36-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the return value to match the JSDoc and correct the logic.
The JSDoc documents that the function should return
[true]on pass, but the implementation always returns a 2-element tuple[isReleaseType, message]. WhenisReleaseTypeistrue, the function returns[true, "Changes in ... require..."], which includes an error message even though validation passed. This contradicts the JSDoc and is semantically incorrect.🐛 Proposed fix to match JSDoc behavior
const isReleaseType = releaseTypeAllowList.has(parsed.type || ''); - return [ - isReleaseType, - `Changes in ${touched.join(', ')} require a release-triggering type (feat|fix|perf|revert|docs) to keep semantic-release automated.`, - ]; + return isReleaseType + ? [true] + : [false, `Changes in ${touched.join(', ')} require a release-triggering type (feat|fix|perf|revert|docs) to keep semantic-release automated.`]; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commitlint.config.js` around lines 36 - 39, The function currently always returns a two-element tuple [isReleaseType, message], which causes a success case to include an error message; update the return logic so that when isReleaseType is true it returns [true] (matching the JSDoc), and when false it returns [false, `Changes in ${touched.join(', ')} require a release-triggering type (feat|fix|perf|revert|docs) to keep semantic-release automated.`]; locate the variables isReleaseType and touched in commitlint.config.js and implement this conditional return to match the documented behavior.
🧹 Nitpick comments (1)
script/lib/brew_categories.py (1)
37-37: ⚡ Quick winConsider TypedDict for match specification structure.
The
match_specdictionary has a known structure with keys "exact", "prefix", and "regex" (all mapping to lists of strings). Defining this with aTypedDictwould provide better type safety thandict[str, Any]and align with the PR's goal to reduceanyusage.♻️ Proposed TypedDict definition
+from typing import Any, TypedDict + + +class MatchSpec(TypedDict, total=False): + exact: list[str] + prefix: list[str] + regex: list[str] + + +class CategorySpec(TypedDict, total=False): + id: str + title: str + match: MatchSpecThen update the function signature:
-def matches(match_spec: dict[str, Any], item: str) -> bool: +def matches(match_spec: MatchSpec, item: str) -> bool:And update
categorize:def categorize( items: list[str], - categories: list[dict[str, Any]], + categories: list[CategorySpec], ) -> tuple[list[tuple[str, list[str]]], list[str]]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/lib/brew_categories.py` at line 37, Define a TypedDict (e.g., MatchSpec) describing the match specification with keys "exact", "prefix", and "regex" each typed as list[str], import TypedDict and List from typing, then change the matches function signature from matches(match_spec: dict[str, Any], item: str) to matches(match_spec: MatchSpec, item: str) and update any callers such as categorize to accept/annotate MatchSpec instead of dict[str, Any] so the structure is strongly typed across the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@commitlint.config.js`:
- Around line 36-39: The function currently always returns a two-element tuple
[isReleaseType, message], which causes a success case to include an error
message; update the return logic so that when isReleaseType is true it returns
[true] (matching the JSDoc), and when false it returns [false, `Changes in
${touched.join(', ')} require a release-triggering type
(feat|fix|perf|revert|docs) to keep semantic-release automated.`]; locate the
variables isReleaseType and touched in commitlint.config.js and implement this
conditional return to match the documented behavior.
---
Nitpick comments:
In `@script/lib/brew_categories.py`:
- Line 37: Define a TypedDict (e.g., MatchSpec) describing the match
specification with keys "exact", "prefix", and "regex" each typed as list[str],
import TypedDict and List from typing, then change the matches function
signature from matches(match_spec: dict[str, Any], item: str) to
matches(match_spec: MatchSpec, item: str) and update any callers such as
categorize to accept/annotate MatchSpec instead of dict[str, Any] so the
structure is strongly typed across the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fa5c4f8-ed87-443e-b7ea-ccefae23d317
📒 Files selected for processing (2)
commitlint.config.jsscript/lib/brew_categories.py
|
🎉 This PR is included in version 1.113.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Closes #787
変更内容
script/lib/brew_categories.py: Python型アノテーション完全追加commitlint.config.js: JSDoc型アノテーション追加Generated with Claude Code
Summary by CodeRabbit