Skip to content

keep / --no-keep の no-op 指定を警告する#50

Merged
TwoSquirrels merged 10 commits into
mainfrom
feat/noop-keep-warnings
Jul 11, 2026
Merged

keep / --no-keep の no-op 指定を警告する#50
TwoSquirrels merged 10 commits into
mainfrom
feat/noop-keep-warnings

Conversation

@TwoSquirrels

@TwoSquirrels TwoSquirrels commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Closes #31

概要

keep の ID が登録済みライブラリに一致しない場合、および --no-keep が何も除外しなかった場合に、stderr へ警告を出す。「typo 検出」ではなく「効果を持たなかった指示 (no-op) の事実を告げる」と位置づけ、意図の推測はせず事実と処方 (library list / library add) を述べる。これにより typo の人にも、「config はコミット済みだが登録がまだ」の clone 直後の人にも、同じ文面が正しく機能する。

変更内容

  • keep の未登録 ID 警告Inventory::is_registered (純粋な照合) を追加し、実効 keep のうち登録に一致しない ID を集約 1 行で警告。std は既存の warn_std_compiler の管轄のため対象外 (自動登録失敗時の二重警告を避ける)
  • --no-keep の no-op 警告 — (config の keep ∪ --keep) のどれにも一致しなかった --no-keep を警告。こちらの typo は「keep されたまま #include が残り提出先で初めて壊れる」という keep 側より硬い失敗につながるため同梱。Settings::resolve の戻りに no-op リストを追加 (唯一のプラミング変更)
  • docs — spec (en/ja) に警告仕様を追記。cheatsheet (en/ja) の「-k したはずのライブラリが展開されてしまう」FAQ を、警告の有無で原因 (ID 不一致 / "..." include) を切り分けられる形に改稿
  • architecture.md — エラーと警告の線引きを記録: 閉じた語彙 (config のキー名) はエラー・開いた語彙 (ライブラリ ID) は警告 / 命令形 (show/delete) はエラー・宣言形 (keep) は警告 / 警告は正当な操作で必ず消える「行き止まりの無い」ものだけを出し、exit code は変えない

警告のみの非破壊的変更で、既存の挙動 (出力・終了コード) は変わらない。

検証

  • clippy 警告ゼロ、全テスト (単体 180 + CLI 17 + libraries 5) 通過
  • 実バイナリで end-to-end 確認:
    • -k ac-libary (typo) → 処方付き警告、出力は生成され exit 0 のまま
    • --no-keep ac-libary (typo) → no-op 警告
    • --no-keep std は既定の keep に一致するため警告なし (常用を邪魔しない)
    • 登録済み ID の keep → 警告なし
    • config 内の複数未登録 ID → 1 行に集約された警告

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added warnings when keep or --no-keep entries do not affect the resolved configuration.
    • Combined multiple affected library IDs into a single warning without changing the exit code.
    • Avoided duplicate warnings for entries already canceled by --no-keep.
  • Documentation

    • Clarified error-versus-warning behavior and troubleshooting guidance for library selection and include syntax in English and Japanese documentation.

TwoSquirrels and others added 4 commits July 11, 2026 15:01
Keeping works by routing a registered library through its dummy dir, so
a keep ID that matches no registration is a complete no-op: a typo'd
-k silently expands the library, and a committed .risundlerc.toml on a
freshly cloned machine references libraries that are not registered yet
(#31). State the fact and the remedy on stderr without guessing intent,
aggregated into one line; the run still succeeds because the expanded
output is a working submission. std stays out of this warning — its
absence is already reported by warn_std_compiler.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same no-op detection as unregistered keeps, but the consequence of a
typo'd --no-keep is harder: the library stays kept, so an unresolvable
#include survives into the submission. Settings::resolve now also
returns the --no-keep IDs that matched neither the configured keeps nor
--keep, and the handler reports them on stderr (#31).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The spec records the warning, its std exclusion, and why it is not an
error; the cheatsheet FAQ now separates the two causes of an expanded
kept library by whether the warning appeared.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closed vocabulary (our own schema) errors, open vocabulary (library
IDs, machine-dependent) warns; imperative direct objects error,
declarative settings warn. Warnings must be escapable by a legitimate
action and never change the exit code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d8524854-a0c5-410a-907f-9a141e0b5a86

📥 Commits

Reviewing files that changed from the base of the PR and between a2dd98e and e295ae1.

📒 Files selected for processing (7)
  • docs/architecture.md
  • docs/cheatsheet.ja.md
  • docs/cheatsheet.md
  • docs/spec.ja.md
  • docs/spec.md
  • src/bundle/inventory.rs
  • src/commands/bundle.rs

📝 Walkthrough

Walkthrough

Changes

Keep and no-keep diagnostics

Layer / File(s) Summary
Resolution and inventory support
src/bundle/inventory.rs, src/commands/bundle.rs
Adds exact library registration checks and returns effective settings with no-op diagnostic metadata.
Warning behavior and validation
src/commands/bundle.rs
Adds aggregated warnings for unregistered keep IDs and ineffective --no-keep IDs, excluding std, with corresponding tests.
Policy and troubleshooting documentation
docs/architecture.md, docs/spec*, docs/cheatsheet*
Documents warning semantics, no-op conditions, and conditional troubleshooting steps.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant BundleCommand
  participant Resolution
  participant Inventory
  User->>BundleCommand: run bundle with keep/no-keep options
  BundleCommand->>Resolution: resolve configuration and CLI settings
  Resolution-->>BundleCommand: effective settings and diagnostic metadata
  BundleCommand->>Inventory: check kept IDs with is_registered
  BundleCommand-->>User: emit aggregated warnings and bundle output
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: warning on no-op keep / --no-keep specifications.
Linked Issues check ✅ Passed The implementation matches the linked issue by warning on unregistered keep and no-op --no-keep IDs, excluding std and keeping exit codes unchanged.
Out of Scope Changes check ✅ Passed All code and doc changes support the warning feature; no unrelated functionality or broad refactors are introduced.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/noop-keep-warnings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

TwoSquirrels and others added 6 commits July 11, 2026 15:45
--no-keep std is a documented way to make std expansion explicit (it
suppresses the std-not-kept warning), so reporting it as a no-op when
the configured keep omits std contradicts that intent and the spec's
std exclusion. Mirror the STD_ID exclusion warn_unregistered_keeps
already applies.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The implementation checks the keep after --no-keep subtraction, so an
ID both kept and no-kept (a resolved contradiction) warns nowhere by
design. Align the spec wording with that semantics instead of the raw
config/CLI keep.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
warn_std_not_kept got its material from a raw-argument rescan in run()
while the no-op --no-keep IDs rode out of resolve as an unlabeled tuple
element; the next keep/no-keep diagnostic would have had to pick a
mechanism again. Resolution now names both fields and is the single
channel for warning material that only the merge can compute.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
warn_noop_no_keeps built a throwaway Vec<&str> solely to satisfy the
&[&str] signature; a generic AsRef<str> bound lets it pass its &[String]
directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@TwoSquirrels TwoSquirrels marked this pull request as ready for review July 11, 2026 08:00
@TwoSquirrels TwoSquirrels requested a review from Copilot July 11, 2026 08:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@TwoSquirrels TwoSquirrels merged commit c8e8f45 into main Jul 11, 2026
10 checks passed
@TwoSquirrels TwoSquirrels deleted the feat/noop-keep-warnings branch July 11, 2026 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

keep / --no-keep の指定が何にも一致しなかったら警告する (no-op 検出)

2 participants