refactor(cli): extract shared Options concern for CLI commands#44
Merged
Conversation
App/Slides/Lint/Serve each hand-rolled constructor dependency injection,
--help text, and argv flag parsing with a lot of repeated boilerplate.
Introduce Slidict::Cli::Options, a concern that provides:
- `options name: -> { default }, ...` to generate attr_readers and
initialize assignment from one declaration instead of spelling out
each dependency in both the method signature and the body.
- `flag`/`parse_flags!`/`flags_help` so a `-x`/`--xxx` switch is declared
once (with its arg placeholder and description) and drives both argv
parsing and the --help listing, instead of a hand-written case/when
parser and a separately hand-formatted help heredoc that could drift
apart.
- `usage`/`print_help` for the surrounding Usage:/Commands: text, with
`flags_help` interpolated in for the option list.
- Named SUCCESS/FAILURE exit code constants instead of bare 0/1 literals
scattered through every command method.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCLI の共通オプション DSL、 ChangesCLI 共通化と新機能追加
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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)
lib/slidict/cli/options.rb (1)
82-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winフラグのキー導出は long switch を優先してください。
Line 83 は
switches.last依存なので、flag("--output", "-o", ...)のように宣言順が変わると:oに保存されます。Flag#longと同じ考え方で long switch を優先すると、DSL の順序依存を避けられます。修正案
def flag(*switches, desc:, arg: nil, coerce: nil, group: :default) - key = switches.last.sub(/\A-+/, "").tr("-", "_").to_sym + canonical = switches.find { |switch| switch.start_with?("--") } || switches.last + key = canonical.sub(/\A-+/, "").tr("-", "_").to_sym flag_groups[group] << Flag.new(switches: switches, key: key, arg: arg, coerce: coerce, desc: desc) end🤖 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 `@lib/slidict/cli/options.rb` around lines 82 - 85, The flag key derivation in `Options#flag` currently depends on `switches.last`, so the saved key changes with declaration order and can become the short switch name. Update `Options#flag` to derive `key` by preferring the long switch first, consistent with `Flag#long`, and only fall back to another switch if no long one exists. This will keep `flag("--output", "-o", ...)` and similar DSL calls stable regardless of argument order.
🤖 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.
Inline comments:
In `@lib/slidict/cli/options.rb`:
- Around line 64-74: The options DSL in options and its generated initialize
currently accepts arbitrary **overrides and silently ignores keys not present in
defaults, which can hide typos in DI keyword names. Add explicit validation in
the generated initialize to compare overrides.keys against defaults.keys and
raise ArgumentError for any unknown keyword, while still applying resolve_option
only to the allowed keys so behavior matches normal Ruby keyword arguments.
---
Nitpick comments:
In `@lib/slidict/cli/options.rb`:
- Around line 82-85: The flag key derivation in `Options#flag` currently depends
on `switches.last`, so the saved key changes with declaration order and can
become the short switch name. Update `Options#flag` to derive `key` by
preferring the long switch first, consistent with `Flag#long`, and only fall
back to another switch if no long one exists. This will keep `flag("--output",
"-o", ...)` and similar DSL calls stable regardless of argument order.
🪄 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: 617a9aac-88c5-4b47-9921-163e887cd6b2
📒 Files selected for processing (6)
lib/slidict.rblib/slidict/cli/app.rblib/slidict/cli/lint.rblib/slidict/cli/options.rblib/slidict/cli/serve.rblib/slidict/cli/slides.rb
Comment on lines
+64
to
+74
| def options(**defaults) | ||
| attr_reader(*defaults.keys) | ||
|
|
||
| include(Module.new do | ||
| define_method(:initialize) do |**overrides| | ||
| defaults.each do |name, default| | ||
| instance_variable_set(:"@#{name}", resolve_option(name, default, overrides)) | ||
| end | ||
| end | ||
| end) | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
未定義のキーワード引数を黙って無視しないでください。
Line 68 の **overrides は defaults に存在しないキーも受け取れますが、Line 69-71 では無視されます。DI 用のキーワード名を typo した場合にデフォルトへフォールバックしてしまうため、通常の Ruby キーワード引数と同じく ArgumentError にした方が安全です。
修正案
def options(**defaults)
attr_reader(*defaults.keys)
include(Module.new do
define_method(:initialize) do |**overrides|
+ unknown = overrides.keys - defaults.keys
+ unless unknown.empty?
+ label = unknown.one? ? "keyword" : "keywords"
+ names = unknown.map { |key| ":#{key}" }.join(", ")
+ raise ArgumentError, "unknown #{label}: #{names}"
+ end
+
defaults.each do |name, default|
instance_variable_set(:"@#{name}", resolve_option(name, default, overrides))
end
end
end)📝 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.
Suggested change
| def options(**defaults) | |
| attr_reader(*defaults.keys) | |
| include(Module.new do | |
| define_method(:initialize) do |**overrides| | |
| defaults.each do |name, default| | |
| instance_variable_set(:"@#{name}", resolve_option(name, default, overrides)) | |
| end | |
| end | |
| end) | |
| end | |
| def options(**defaults) | |
| attr_reader(*defaults.keys) | |
| include(Module.new do | |
| define_method(:initialize) do |**overrides| | |
| unknown = overrides.keys - defaults.keys | |
| unless unknown.empty? | |
| label = unknown.one? ? "keyword" : "keywords" | |
| names = unknown.map { |key| ":#{key}" }.join(", ") | |
| raise ArgumentError, "unknown #{label}: #{names}" | |
| end | |
| defaults.each do |name, default| | |
| instance_variable_set(:"@#{name}", resolve_option(name, default, overrides)) | |
| end | |
| end | |
| end) | |
| end |
🤖 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 `@lib/slidict/cli/options.rb` around lines 64 - 74, The options DSL in options
and its generated initialize currently accepts arbitrary **overrides and
silently ignores keys not present in defaults, which can hide typos in DI
keyword names. Add explicit validation in the generated initialize to compare
overrides.keys against defaults.keys and raise ArgumentError for any unknown
keyword, while still applying resolve_option only to the allowed keys so
behavior matches normal Ruby keyword arguments.
Adds --language LANG so LLM-generated slide titles/bullets, and any interactive questions still needing an answer, are produced in a language other than English (e.g. Japanese). Questions are translated one at a time as plain text rather than a batched JSON array, since that's a task small/local models follow far more reliably; translation failures fall back to English with a warning instead of aborting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Lets SLIDICT_LLM_BASE_URL, SLIDICT_LLM_API_KEY, SLIDICT_LLM_MODEL, SLIDICT_FRAMEWORK, and SLIDICT_METHOD be set via a .env file instead of the shell environment, so per-project config doesn't need to live in a shell profile. CLI flags still take precedence over both .env and real environment variables. `slidict init` scaffolds a starter .env and adds it to .gitignore so secrets like API keys stay out of version control. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
spec/slidict/llm/client_spec.rb (1)
108-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuboCop指摘（セミコロン/長い行/Block too many lines）と重複コードの解消。
instance_double(Net::HTTP)を作ってrequest_bodyをキャプチャするパターンが3箇所（115, 129, 222行目付近）でほぼ同一に重複しており、それが原因で以下のRuboCop違反が発生しています。
- Line 115, 129, 222:
Style/Semicolon（request_body = req.body; response）- Line 220:
Layout/LineLength(125/120)- Line 210-246:
Metrics/BlockLength([27/25])共通ヘルパーを抽出することで重複と3つの違反をまとめて解消できます。
♻️ 提案
def stub_http_capturing_request(content) http = instance_double(Net::HTTP) response = Net::HTTPOK.new("1.1", "200", "OK") allow(response).to receive(:body) .and_return({ "choices" => [{ "message" => { "content" => content } }] }.to_json) captured = nil allow(http).to receive(:request) do |req| captured = req.body response end allow(Net::HTTP).to receive(:start).and_yield(http) -> { captured } endit "includes the language instruction in the prompt when language: is given" do - content = [{ "title" => "Observability", "bullets" => %w[a b] }].to_json - http = instance_double(Net::HTTP) - response = Net::HTTPOK.new("1.1", "200", "OK") - allow(response).to receive(:body).and_return({ "choices" => [{ "message" => { "content" => content } }] }.to_json) - request_body = nil - allow(http).to receive(:request) { |req| request_body = req.body; response } - allow(Net::HTTP).to receive(:start).and_yield(http) + content = [{ "title" => "Observability", "bullets" => %w[a b] }].to_json + request_body = stub_http_capturing_request(content) client.generate_slides(deck, language: "Japanese") - expect(JSON.parse(request_body)["messages"].first["content"]).to include("Japanese") + expect(JSON.parse(request_body.call)["messages"].first["content"]).to include("Japanese") end同様の置き換えを123-135行目・217-230行目にも適用してください。
Also applies to: 209-246
🤖 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 `@spec/slidict/llm/client_spec.rb` around lines 108 - 135, The spec has repeated Net::HTTP stubbing/setup across multiple examples, which is causing the semicolon, line-length, and block-length RuboCop offenses. Extract the shared HTTP mock/capture logic into a small helper (for example, a method or shared helper near generate_slides examples) and reuse it in the language-related examples and the later block that stubs request bodies. Keep the helper centered around the existing generate_slides, instance_double(Net::HTTP), and request_body capture pattern so the duplicated setup is removed from each example.Source: Linters/SAST tools
lib/slidict/cli/app.rb (2)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--languageのヘルプ説明が実際の挙動と食い違っています。説明文は「LLM生成スライドのみに影響」となっていますが、
questions_for（386-403行目）では対話質問の翻訳にも--languageが使われています。ユーザーが誤解する可能性があるため説明を更新することを推奨します。📝 提案
flag "--language", arg: "LANG", desc: "Generate slide titles and bullets in the given language\n" \ - "(e.g. Japanese); only affects LLM-generated slides" + "(e.g. Japanese); also translates the interactive prompts\n" \ + "asked below; only takes effect when an LLM is configured"🤖 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 `@lib/slidict/cli/app.rb` around lines 39 - 40, The `--language` help text in `App` is misleading because it says the option only affects LLM-generated slides, but `questions_for` also uses it when translating interactive questions. Update the description on the `flag "--language"` definition to reflect both behaviors, and keep the wording aligned with how `questions_for` applies the language setting.
322-327: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
.envの作成時にパーミッションを制限すべきです。
ENV_TEMPLATEのコメントにはSLIDICT_LLM_API_KEY=sk-...のような秘密情報の保存が想定されていますが、File.writeはデフォルトのumask依存パーミッションで作成するため、他ユーザーが読み取れる可能性があります。🔒 修正案
def write_env_file return false if File.exist?(".env") File.write(".env", ENV_TEMPLATE) + File.chmod(0o600, ".env") true end🤖 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 `@lib/slidict/cli/app.rb` around lines 322 - 327, `.env` 作成時の権限が umask 依存のままで、秘密情報を他ユーザーが読める可能性があります。`write_env_file` で `.env` を作る処理を見直し、`File.write` だけでなく `File.open` や `FileUtils.touch` などで作成権限を明示的に 0600 相当に制限するように修正してください。`ENV_TEMPLATE` を書き込む前に、`.env` の所有者のみ読み書き可能であることを保証する実装にしてください。spec/slidict/cli/app_spec.rb (1)
237-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SLIDICT_METHODの優先順位・デフォルト適用が実質未検証です。2つのテストとも
SLIDICT_FRAMEWORK（ファイル拡張子）のみをアサートしており、SLIDICT_METHOD/--methodが実際に反映されているかは確認していません（250行目のテスト名は「--method の優先」も謳っていますが--methodフラグ自体渡していません）。options[:presentation_method]や生成された内容を検証するアサーションの追加を推奨します。🤖 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 `@spec/slidict/cli/app_spec.rb` around lines 237 - 263, The current CLI specs only verify framework-based file output, so SLIDICT_METHOD/--method behavior is not actually covered. Update the app_spec examples around cli.run to assert the presentation method is set from options[:presentation_method] or reflected in generated content, and add a true override case that passes --method alongside the env vars so the precedence over SLIDICT_METHOD is verified.
🤖 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.
Inline comments:
In `@lib/slidict/cli/app.rb`:
- Around line 313-334: `init` のファイルI/Oエラーが未捕捉で、`write_env_file` と
`ensure_env_gitignored` が権限エラー等をそのまま例外送出しています。`init` かこれらのメソッド周辺で
`Errno::EACCES` などを捕捉し、`auth` や LLM 呼び出しと同様に丁寧なエラーメッセージを `@output` に出して
`FAILURE` を返すようにしてください。`init`, `write_env_file`, `ensure_env_gitignored`
を基点に、成功時の `SUCCESS` と同じ一貫した戻り値設計に揃えてください。
---
Nitpick comments:
In `@lib/slidict/cli/app.rb`:
- Around line 39-40: The `--language` help text in `App` is misleading because
it says the option only affects LLM-generated slides, but `questions_for` also
uses it when translating interactive questions. Update the description on the
`flag "--language"` definition to reflect both behaviors, and keep the wording
aligned with how `questions_for` applies the language setting.
- Around line 322-327: `.env` 作成時の権限が umask
依存のままで、秘密情報を他ユーザーが読める可能性があります。`write_env_file` で `.env` を作る処理を見直し、`File.write`
だけでなく `File.open` や `FileUtils.touch` などで作成権限を明示的に 0600
相当に制限するように修正してください。`ENV_TEMPLATE` を書き込む前に、`.env`
の所有者のみ読み書き可能であることを保証する実装にしてください。
In `@spec/slidict/cli/app_spec.rb`:
- Around line 237-263: The current CLI specs only verify framework-based file
output, so SLIDICT_METHOD/--method behavior is not actually covered. Update the
app_spec examples around cli.run to assert the presentation method is set from
options[:presentation_method] or reflected in generated content, and add a true
override case that passes --method alongside the env vars so the precedence over
SLIDICT_METHOD is verified.
In `@spec/slidict/llm/client_spec.rb`:
- Around line 108-135: The spec has repeated Net::HTTP stubbing/setup across
multiple examples, which is causing the semicolon, line-length, and block-length
RuboCop offenses. Extract the shared HTTP mock/capture logic into a small helper
(for example, a method or shared helper near generate_slides examples) and reuse
it in the language-related examples and the later block that stubs request
bodies. Keep the helper centered around the existing generate_slides,
instance_double(Net::HTTP), and request_body capture pattern so the duplicated
setup is removed from each example.
🪄 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: 1d0c3881-dc1e-4df0-b5f1-4af32ab5b4fc
📒 Files selected for processing (13)
.gitignoreCHANGELOG.mdREADME.mdbin/slidictlib/slidict.rblib/slidict/cli/app.rblib/slidict/env.rblib/slidict/llm/client.rblib/slidict/version.rbspec/slidict/cli/app_spec.rbspec/slidict/env_spec.rbspec/slidict/llm/client_spec.rbspec/spec_helper.rb
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- lib/slidict/version.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/slidict.rb
Comment on lines
+313
to
+334
| def init | ||
| env_created = write_env_file | ||
| @output.puts(env_created ? "Created .env" : ".env already exists, leaving it unchanged") | ||
|
|
||
| gitignore_updated = ensure_env_gitignored | ||
| @output.puts "Added .env to .gitignore" if gitignore_updated | ||
| SUCCESS | ||
| end | ||
|
|
||
| def write_env_file | ||
| return false if File.exist?(".env") | ||
|
|
||
| File.write(".env", ENV_TEMPLATE) | ||
| true | ||
| end | ||
|
|
||
| def ensure_env_gitignored | ||
| return false if File.exist?(".gitignore") && File.readlines(".gitignore", chomp: true).include?(".env") | ||
|
|
||
| File.open(".gitignore", "a") { |f| f.puts(".env") } | ||
| true | ||
| end |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
init でのファイルI/Oエラーが未捕捉です。
write_env_file/ensure_env_gitignored は書き込み不可なディレクトリ（権限なし、読み取り専用FSなど）で Errno::EACCES 等を素のまま送出します。他のコマンド（auth, LLM呼び出しなど）はエラーを捕捉して丁寧なメッセージ + FAILURE を返す方針なので、init だけスタックトレースで落ちるのは一貫性を欠きます。
🛡️ 修正案
def init
env_created = write_env_file
`@output.puts`(env_created ? "Created .env" : ".env already exists, leaving it unchanged")
gitignore_updated = ensure_env_gitignored
`@output.puts` "Added .env to .gitignore" if gitignore_updated
SUCCESS
+ rescue SystemCallError => e
+ `@output.puts` "Error: could not write files (#{e.message})"
+ FAILURE
end📝 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.
Suggested change
| def init | |
| env_created = write_env_file | |
| @output.puts(env_created ? "Created .env" : ".env already exists, leaving it unchanged") | |
| gitignore_updated = ensure_env_gitignored | |
| @output.puts "Added .env to .gitignore" if gitignore_updated | |
| SUCCESS | |
| end | |
| def write_env_file | |
| return false if File.exist?(".env") | |
| File.write(".env", ENV_TEMPLATE) | |
| true | |
| end | |
| def ensure_env_gitignored | |
| return false if File.exist?(".gitignore") && File.readlines(".gitignore", chomp: true).include?(".env") | |
| File.open(".gitignore", "a") { |f| f.puts(".env") } | |
| true | |
| end | |
| def init | |
| env_created = write_env_file | |
| `@output.puts`(env_created ? "Created .env" : ".env already exists, leaving it unchanged") | |
| gitignore_updated = ensure_env_gitignored | |
| `@output.puts` "Added .env to .gitignore" if gitignore_updated | |
| SUCCESS | |
| rescue SystemCallError => e | |
| `@output.puts` "Error: could not write files (#{e.message})" | |
| FAILURE | |
| end | |
| def write_env_file | |
| return false if File.exist?(".env") | |
| File.write(".env", ENV_TEMPLATE) | |
| true | |
| end | |
| def ensure_env_gitignored | |
| return false if File.exist?(".gitignore") && File.readlines(".gitignore", chomp: true).include?(".env") | |
| File.open(".gitignore", "a") { |f| f.puts(".env") } | |
| true | |
| end |
🤖 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 `@lib/slidict/cli/app.rb` around lines 313 - 334, `init`
のファイルI/Oエラーが未捕捉で、`write_env_file` と `ensure_env_gitignored`
が権限エラー等をそのまま例外送出しています。`init` かこれらのメソッド周辺で `Errno::EACCES` などを捕捉し、`auth` や LLM
呼び出しと同様に丁寧なエラーメッセージを `@output` に出して `FAILURE` を返すようにしてください。`init`,
`write_env_file`, `ensure_env_gitignored` を基点に、成功時の `SUCCESS`
と同じ一貫した戻り値設計に揃えてください。
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
1 participant
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.
App/Slides/Lint/Serve each hand-rolled constructor dependency injection,
--help text, and argv flag parsing with a lot of repeated boilerplate.
Introduce Slidict::Cli::Options, a concern that provides:
options name: -> { default }, ...to generate attr_readers andinitialize assignment from one declaration instead of spelling out
each dependency in both the method signature and the body.
flag/parse_flags!/flags_helpso a-x/--xxxswitch is declaredonce (with its arg placeholder and description) and drives both argv
parsing and the --help listing, instead of a hand-written case/when
parser and a separately hand-formatted help heredoc that could drift
apart.
usage/print_helpfor the surrounding Usage:/Commands: text, withflags_helpinterpolated in for the option list.scattered through every command method.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit
新機能
slidict initにより LLM/機能設定用の.envを作成し、.gitignoreに追記します。--languageで生成するタイトル/箇条書きを指定言語化し、対話質問も翻訳します。改善
.envを読み込み、未設定は補完（CLIフラグ優先）するようになりました。