feat: support :integer option type in CLI spec - #13
Conversation
Widen option_def's type union and render a VALUE placeholder for any non-boolean option. Needed by ecosystem-manager's --max-concurrency; registry-manager's byte-identical help fixtures are unchanged. Resolves #12
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| assert {:jobs, :integer} in Spec.strict_switches(spec) | ||
| assert Spec.render_command_help(spec, "run") =~ "--jobs VALUE" | ||
| assert Spec.validate_opts(spec, "run", jobs: 4) == :ok |
There was a problem hiding this comment.
validate_opts/3 に対して jobs: 4（整数値）のみ検証していますが、:integer 型のオプションに文字列や浮動小数点数が渡された場合（例: jobs: "four" や jobs: 3.14）のバリデーション失敗ケースがテストされていません。OptionParser が :integer を正しく拒否するかどうかは実装依存であり、将来の変更で誤った値が通過するリスクがあります。正常系に加えて異常系のテストケースも追加することを推奨します。
There was a problem hiding this comment.
修正済み(2b08f44)。整数でない値の拒否は OptionParser(strict)の責務で、Parser がパース段階のエラーに変換します。Parser.parse(spec, ["run", "--jobs", "four"]) が {:error, ...} になる negative テストを追加しました。
|
|
||
| # 整数でない値の拒否は OptionParser(strict)の責務で、 | ||
| # Parser がパース段階のエラーに変換する | ||
| assert {:error, message} = ToolKit.CLI.Parser.parse(spec, ["run", "--jobs", "four"]) |
There was a problem hiding this comment.
ℹ️ [LOW] テストで ToolKit.CLI.Parser.parse/2 を直接呼び出していますが、このモジュールへの alias や import がテストファイル冒頭に存在しない場合、コンパイルエラーになります。現在 mix test が通っているとのことなので問題はないと思われますが、他のテストケースとの一貫性のため、モジュール参照を Spec と同様に短縮形で統一するか、明示的に alias ToolKit.CLI.Parser を追加することを検討してください。
There was a problem hiding this comment.
対応済み(6adf83e)。alias ToolKit.CLI.Parser を追加して短縮形に統一しました(CI の credo strict がこの点を検出して failure になっていたため、同 commit で解消済みです)。
| type: :boolean | :string, | ||
| type: :boolean | :string | :integer, | ||
| alias: atom() | nil, | ||
| values: [String.t()] | nil, |
There was a problem hiding this comment.
ℹ️ [LOW] :integer 型のオプションに対して values フィールドが nil 以外（例: ["1", "2", "4"] のような候補リスト）を持つケースを将来的に許容するかどうかが不明確です。現在の render_values/1 が :string 向けに設計されている場合、:integer で values が非 nil のときの描画が意図通りになるか確認が必要です。型定義上 values: [String.t()] | nil のままであり、:integer との組み合わせの意味論が曖昧です。将来の拡張を見越してコメントや型定義で制約を明示することを検討してください。
There was a problem hiding this comment.
据え置きます。values は enum 検証(パース済み値が文字列リストに含まれるか)のためのフィールドで、意味論上 :string 専用です。:integer オプションで values を設定すると validate_opts が必ず不一致になるため事実上使えず、使う予定もありません(現行の利用者 rm/tm/em に該当なし)。必要になった時点で型定義の制約強化を検討します。
|
|
||
| # 整数でない値の拒否は OptionParser(strict)の責務で、 | ||
| # Parser がパース段階のエラーに変換する | ||
| assert {:error, message} = Parser.parse(spec, ["run", "--jobs", "four"]) |
There was a problem hiding this comment.
ℹ️ [LOW] Spec.validate_opts(spec, "run", jobs: 4) で :ok を確認していますが、jobs: -1 や jobs: 0 など境界値・負値のケースが検証されていません。また、Parser.parse/2 で "four" を渡した際のエラーメッセージが "--jobs" を含むことのみを確認していますが、エラーの種類（型不一致）を示すメッセージ内容についてもアサーションを追加すると、将来のリグレッション検出に役立ちます。
There was a problem hiding this comment.
据え置きます。負値・0 などの値域検証は spec エンジンの責務ではなくツール側のビジネスロジックの責務です(OptionParser は任意の整数を受理し、例えば max_concurrency の下限はツールが判断する)。エラーメッセージ文言のアサーション追加は、文言変更のたびに壊れる brittle なテストになるため、エラーであること + 対象オプション名の包含の検証に留めています。
AI レビュー指摘への対応まとめ全 4 スレッド対応済み(修正 2・据え置き 2)。CI は 6adf83e で全 green。
補足: 2b08f44 push 時に Actions が発火しない事象があり、空 commit(5629776)で再トリガしています。 |
1 participant
概要
option_defの型を:boolean | :string | :integerに拡張する(K2 追補。ecosystem-manager の--max-concurrency採用時に dialyzer の型不一致で発覚)。Resolves #12 / 親 epic: smkwlab/latex-ecosystem#146
変更内容
option_def.type/strict_switchesの型に:integerを追加確認済み
mix test --cover9 doctests + 71 tests, 0 failures ✔(integer オプションの導出・描画・検証テスト追加)mix format --check-formatted/mix credo0 issues /mix dialyzer0 errors ✔merge 後に tag v0.1.2 を発行し、E1(em#9)から参照します。