chore(agents): update skill metadata timestamps, source URLs, and documentation wording across dot_agents files

2026-11-05 12:17:36 +00:00 · 2026-08-13 21:04:10 -04:00
1 parent 12155ad58c
commit c96c19c0f9
@@ -12,7 +12,7 @@ license: MIT
 compatibility: Rust 1.70+, Cargo
 metadata:
  author: apollographql
-  version: "1.1.0"
+  version: "1.1.1"
 allowed-tools: Bash(cargo:*) Bash(rustc:*) Bash(rustfmt:*) Bash(clippy:*) Read Write Edit Glob Grep
 ---

@@ -24,7 +24,7 @@ Apply these guidelines when writing or reviewing Rust code. Based on Apollo Grap

 Before reviewing, familiarize yourself with Apollo's Rust best practices. Read ALL relevant chapters in the same turn in parallel. Reference these files when providing feedback:

- [Chapter 1 - Coding Styles and Idioms](references/chapter_01.md): Borrowing vs cloning, Copy trait, Option/Result handling, iterators, comments
+- [Chapter 1 - Coding Styles and Idioms](references/chapter_01.md): Borrowing vs cloning, Copy trait, Option/Result handling, iterators, comments, when to extract a function (duplication vs. wrong abstraction)
 - [Chapter 2 - Clippy and Linting](references/chapter_02.md): Clippy configuration, important lints, workspace lint setup
 - [Chapter 3 - Performance Mindset](references/chapter_03.md): Profiling, avoiding redundant clones, stack vs heap, zero-cost abstractions
 - [Chapter 4 - Error Handling](references/chapter_04.md): Result vs panic, thiserror vs anyhow, error hierarchies
@@ -76,6 +76,7 @@ let new_num = increment(num); // `num` still usable after this point
 * All fields are `Copy` themselves.
 * The struct is `small`, up to 2 (maybe 3) words of memory or 24 bytes (each word is 64 bits/8bytes).
 * The struct **represents a "plain data object"**, without resourcing to ownership (no heap allocations. Example: `Vec` and `Strings`).
+* ❗**The type does not also implement `Iterator`.** Even if every field is `Copy`, never put `Copy` and `Iterator` on the same type (see [§1.5](#15-iterator-iter-vs-for)).

 ❗**Rust Arrays are stack allocated.** Which means they can be copied if their underlying type is `Copy`, but this will be allocated in the program stack which can easily become a stack overflow. More on [Chapter 3 - Stack vs Heap](./chapter_03.md#33-stack-vs-heap-be-size-smart)

@@ -351,6 +352,7 @@ for value in vec.iter().enumerate()
 * Avoid needlessly collect/allocate of a collection (e.g. vector) just to throw it away later by some larger operation or by another iteration.
 * Prefer `iter` over `into_iter` unless you don't need the ownership of the collection.
 * Prefer `iter` over `into_iter` for collections that inner type implements `Copy`, e.g. `Vec<i32>`.
+* **Never implement (or derive) both `Copy` and `Iterator` on the same type.** Copying an iterator and advancing one copy leaves the other untouched, which silently yields wrong results -- it is a well-known footgun. The standard library hit exactly this: it is why `Range` historically could not be `Copy`, and why the new `core::range` types (stabilized in Rust 1.96) implement `IntoIterator` instead of `Iterator` so they *can* be `Copy`. If you need an iterator on a `Copy` type, implement `IntoIterator` and return a separate iterator struct.
 * For summing numbers prefer `.sum` over `.fold`. `.sum` is specialized for summing values, so the compiler knows it can make optimizations on that front, while fold has a blackbox closure that needs to be applied at every step. If you need to sum by an initial value, just added in the expression `let my_sum = [1, 2, 3].sum() + 3`.

 ## 1.6 Comments: Context, not Clutter
@@ -459,6 +461,8 @@ mod tests {

 Let **structure** and **naming** replace commentary, and enhance its documentation with **tests as living documentation**.

+> ❗ Splitting is about **naming and clarity**, not deduplication. Before extracting a helper to remove *duplicated* lines, check [§1.8](#18-when-to-extract-a-function-and-when-not-to).
+
 ### 📝 TODOs are not comments - track them properly

 Avoid leaving lingering `// TODO: Lorem Ipsum` comments in the code. Instead:
@@ -550,3 +554,94 @@ group_imports = "StdExternalCrate"
 ```

 > As of Rust version 1.88, it is necessary to execute rustfmt in nightly to correctly reorder code `cargo +nightly fmt`.
+
+## 1.8 When to Extract a Function (and When Not To)
+
+This chapter ([§1.6](#-breaking-up-long-functions-over-commenting-them)) and [Chapter 8](./chapter_08.md#85-replace-comments-with-code) recommend splitting long functions and replacing narrative comments with named helpers. That advice is about **naming and clarity** -- it is *not* a license to hunt down every repeated line. Extraction has a cost: every helper adds indirection, and a **wrong abstraction is much harder to remove than a little duplication**.
+
+> "Duplication is far cheaper than the wrong abstraction." -- [Sandi Metz, The Wrong Abstraction](https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction)
+
+### 📏 The Rule of Three
+
+From [Martin Fowler's *Refactoring*](https://martinfowler.com/books/refactoring.html), attributed to Don Roberts:
+
+> The first time you do something, you just do it. The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway. The third time you do something similar, you refactor.
+
+Two occurrences of a couple of lines are usually **fine**. Wait for the third before reaching for a helper -- by then you will know what the abstraction actually *is*, instead of guessing at it.
+
+### 🧠 DRY is about knowledge, not text
+
+[DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) (*The Pragmatic Programmer*) says every piece of **knowledge** should have a single representation. It does **not** say "no two code fragments may look alike". Two blocks that *happen* to look the same today, but represent **different decisions**, will evolve in different directions. Forcing them into one helper couples code that should be free to diverge -- this is **coincidental duplication**, and deduplicating it is how wrong abstractions are born.
+
+### 🧪 Examples
+
+How these principles play out in code:
+
+#### ✅ Coincidental duplication -- leave it inline:
+```rust
+// Similar-looking lines, but the `.env` and `.toml` formats are
+// unrelated decisions that will evolve independently:
+writeln!(env_file, "{key}={value}")?;
+// ... elsewhere ...
+writeln!(toml_file, "{key} = \"{value}\"")?;
+```
+
+#### ❌ Deduplicating it forces a flag parameter -- the classic smell:
+```rust
+// One helper, two behaviors, and a `bool` to pick between them:
+fn write_entry(out: &mut impl Write, key: &str, value: &str, quoted: bool) -> io::Result<()> {
+    if quoted {
+        writeln!(out, "{key} = \"{value}\"")
+    } else {
+        writeln!(out, "{key}={value}")
+    }
+}
+```
+
+Every new variant will grow another parameter or branch. And the `bool` is a smell in its own right: `write_entry(out, key, value, true)` tells the reader nothing at the call site -- Martin Fowler calls this a [Flag Argument](https://martinfowler.com/bliki/FlagArgument.html). When a mode selector is genuinely warranted, a two-variant enum (`Quoting::Quoted`) keeps call sites readable, and Clippy's [`fn_params_excessive_bools`](https://rust-lang.github.io/rust-clippy/master/index.html#fn_params_excessive_bools) lints against accumulating them. No enum rescues *this* design, though: its variants would exist only to encode the callers' differences.
+
+#### ✅ Shared knowledge -- extract it:
+```rust
+// The definition of "retryable" is ONE business decision used in many
+// places. If it changes, it must change everywhere at once:
+fn is_retryable(status: StatusCode) -> bool {
+    status == StatusCode::TOO_MANY_REQUESTS || status.is_server_error()
+}
+```
+
+### 🔧 Unwinding a wrong abstraction
+
+If you find yourself maintaining an abstraction like `write_entry`, [Sandi Metz's advice](https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction) is to **unwind it, not patch it**:
+
+1. **Re-inline** the helper's body into every caller, substituting each call site's concrete arguments.
+2. **Reduce** each call site -- with the parameters now literal, dead branches and unused generality fall away.
+3. **Reevaluate** what the callers *actually* share, now that every one of them is explicit.
+4. **Reconstruct** one or more abstractions from what remains -- or none, if the sharing turns out to be coincidental.
+
+> 🤖 Doing this by hand is tedious, which is why wrong abstractions survive on sunk cost. For an AI coding agent, unwinding is fast, mechanical work -- ask for it explicitly instead of letting the agent keep patching a helper that fights back.
+
+### ✅ Extract a function when:
+* The logic appears in **3+ places** *and* represents the **same decision** (Rule of Three).
+* The name **adds meaning** the code alone does not (`is_retryable`, `authorize`) -- rather than restating it (`add_one_to_counter`).
+* It produces a **unit worth testing** in isolation (see [§1.6](#-breaking-up-long-functions-over-commenting-them)).
+* It hides genuine complexity behind a **small, honest signature** -- few parameters, no flags.
+
+### ❌ Don't extract when:
+* It is **1-2 lines used in fewer than 3 places** -- the helper is pure indirection.
+* The call sites are only *almost* identical, and unifying them requires **flag parameters** or extra branches.
+* The only motivation is **line count**. A single-caller helper pays off when its *name* clarifies intent ([§1.6](#-breaking-up-long-functions-over-commenting-them), [§8.5](./chapter_08.md#85-replace-comments-with-code)) -- not when it merely relocates code.
+* You are **guessing** at a future abstraction. Wait for the third usage to reveal its real shape.
+
+### 🧫 Test code: readability beats DRY
+
+In tests, be **even more tolerant of duplication**. The Google Testing Blog calls this [DAMP -- "Descriptive And Meaningful Phrases"](https://testing.googleblog.com/2019/12/testing-on-toilet-tests-too-dry-make.html) ([*Software Engineering at Google*, ch. 12](https://abseil.io/resources/swe-book/html/ch12.html)): each test should read as a **self-contained story** -- setup, action, assertion -- without the reader chasing helpers to reconstruct what actually happened.
+
+* **Tests have no tests.** Logic moved into a shared helper (loops, branches, clever parametrization) is itself untested -- a bug there silently weakens every test built on it.
+* **A failing test should be diagnosable on sight.** The reader is usually debugging a broken build; make the expected behavior visible in the test body, not three helpers away.
+* **Shared test helpers couple unrelated tests.** Change one behavior and dozens of tests fail at once -- for the helper's sake, not the behavior's.
+
+Where to draw the line:
+* ✅ Share **setup and fixtures** -- a shared setup function or `rstest` cases, as [Chapter 5](./chapter_05.md#51-tests-as-living-documentation) recommends. Constructing a test server twice is boilerplate, not knowledge.
+* ❌ Keep each test's **action and assertion inline**, even when they look repetitive across tests.
+
+> 🚨 When in doubt, **prefer duplication**. A duplicated line is trivially fixed later; a wrong abstraction accretes parameters and conditionals, because each maintainer keeps patching it instead of undoing it.
@@ -174,6 +174,8 @@ fn the_function_accepts_all_strings_with_a(#[case] input: &str) {
 > * It's harder for both IDEs and humans to run/locate specific tests.
 > * Expectation vs condition naming is now visually inverted (expectation first).

+> ❗ Share **setup**, not the test itself: keep each test's action and assertion inline, even when repetitive. Tests tolerate duplication better than production code -- see [Chapter 1, §1.8](./chapter_01.md#-test-code-readability-beats-dry).
+
 ## 5.2 Add Test Examples to your Docs

 We will deep dive into docs at a later stage, so in this section we will just briefly go over how to add tests to you docs. Rustdoc can turn examples into executable tests using `///` with a few advantages:
@@ -282,9 +284,15 @@ Rust comes with 2 macros to make assertions:

 ### 🚨 `assert!` reminders
 * Rust asserts support formatted strings, like the previous examples, those strings will be printed in case of failure, so it is a good practice to add what the actual state was and how it differs from the expected.
-* If you don't care about the exact pattern matching value, using `matches!` combined with `assert!` might be a good alternative.
+* If you don't care about the exact pattern matching value, using `matches!` combined with `assert!` might be a good alternative. Note the message string is an argument to `assert!`, not to `matches!`:
 ```rust
-assert!(matches!(error, MyError::BadInput(_), "Expected `BadInput`, found {error}"));
+assert!(matches!(error, MyError::BadInput(_)), "Expected `BadInput`, found {error}");
+```
+* Since Rust 1.96 prefer the stabilized [`assert_matches!`](https://doc.rust-lang.org/std/macro.assert_matches.html) (and [`debug_assert_matches!`](https://doc.rust-lang.org/std/macro.debug_assert_matches.html)) over `assert!(matches!(..))`. It prints the actual value on failure for free, so you don't have to write the message yourself:
+```rust
+use std::assert_matches::assert_matches;
+
+assert_matches!(error, MyError::BadInput(_));
 ```
 * Use `#[should_panic]` wisely. It should only be used when panic is the desired behavior, prefer result instead of panic.
 * There are some other that can enhance your testing experience like:
@@ -102,6 +102,8 @@ fn save_auth_user(&self) -> Result<PathBuf, MyError> {
 }
 ```

+> ❗ Extract when the **name adds meaning** -- not to deduplicate a couple of similar-looking lines. See [Chapter 1, §1.8](./chapter_01.md#18-when-to-extract-a-function-and-when-not-to) for when duplication is the better trade-off.
+
 ## 8.6 `TODO` should become issues

 Don't leave `// TODO:` scattered around the codebase with no owner. Instead: