Skip to content

📝 Add docstrings to claude/ladybug-transcoding-plan-q5zbrs - #115

Closed
coderabbitai[bot] wants to merge 1 commit into
claude/ladybug-transcoding-plan-q5zbrsfrom
coderabbitai/docstrings/038bbf8
Closed

coderabbitai[bot] wants to merge 1 commit into
claude/ladybug-transcoding-plan-q5zbrsfrom
coderabbitai/docstrings/038bbf8

Conversation

@coderabbitai

@coderabbitai coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Docstrings generation was requested by @AdaWorldAPI.

The following files were modified:

  • crates/ruff_cpp_spo/examples/harvest_events.rs
  • crates/ruff_cpp_spo/examples/harvest_ladybug.rs
  • crates/ruff_cpp_spo/src/clang_walker.rs
  • crates/ruff_cpp_spo/src/events.rs
  • crates/ruff_cpp_spo/src/lib.rs
  • crates/ruff_spo_triplet/src/expand.rs
  • crates/ruff_spo_triplet/src/ir.rs
  • crates/ruff_spo_triplet/src/reassemble.rs
  • crates/ruff_spo_triplet/src/recipe.rs
These file types are not supported
  • .claude/knowledge/fuzzy-recipe-codebook.md
  • .claude/plans/behavioral-ore-v1.md
  • .claude/plans/cpp-spo-probes-v1.md
  • _typos.toml
  • crates/ruff_cpp_spo/Cargo.toml
  • crates/ruff_spo_triplet/Cargo.toml
ℹ️ Note

CodeRabbit cannot perform edits on its own pull requests yet.

Docstrings generation was requested by @AdaWorldAPI.

* #114 (comment)

The following files were modified:

* `crates/ruff_cpp_spo/examples/harvest_events.rs`
* `crates/ruff_cpp_spo/examples/harvest_ladybug.rs`
* `crates/ruff_cpp_spo/src/clang_walker.rs`
* `crates/ruff_cpp_spo/src/events.rs`
* `crates/ruff_cpp_spo/src/lib.rs`
* `crates/ruff_spo_triplet/src/expand.rs`
* `crates/ruff_spo_triplet/src/ir.rs`
* `crates/ruff_spo_triplet/src/reassemble.rs`
* `crates/ruff_spo_triplet/src/recipe.rs`
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Author

Important

Review skipped

This PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Essentials

Run ID: 7eab1909-4a60-4041-a18c-1f18491d075e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

Copy link
Copy Markdown
Owner

This branch does not compile. Measured, not inferred — cargo check -p ruff_cpp_spo --features libclang on fda5a4d in a clean worktree:

error: this file contains an unclosed delimiter
   --> crates/ruff_spo_triplet/src/reassemble.rs:857:3
error: could not compile `ruff_spo_triplet` (lib) due to 1 previous error

Four defects, each verified against the branch content rather than read off the diff summary.

1. A duplicated function signature — this is the unclosed delimiter. In reassemble.rs the generator emitted the signature line twice:

pub fn reassemble(triples: &[Triple]) -> ModelGraph {
pub fn reassemble(triples: &[Triple]) -> ModelGraph {

Two { opened, one closed. Compilation stops here, which is why the remaining defects are not reported by the compiler yet.

2. The generator's own scratchpad was emitted as source. crates/ruff_cpp_spo/src/clang_walker.rs:836, verbatim, between the doc comment and the real signature:

fn build_method doc? They demand docstring itself, should include /// only, not signature. But example references vars and private types; ignore okay. Must summary not starts Returns. Fine. However output should not include fn. done.

3. Ten #[cfg(feature = "libclang")] gates were dropped. clang_walker.rs goes from 33 to 23 occurrences, and the diff removes 11 #[cfg(feature = "libclang")] and 2 #[must_use] lines while adding none back. Those attributes gate functions taking libclang types, so the default (no-libclang) build would fail even after the two syntax errors above are fixed. This one is easy to miss because it looks like an attribute reposition in the diff; it is a deletion.

4. Documentation is replaced, not added. 238 of the 251 removed lines are /// comments. The substitutions drop the reasoning and keep a restatement of the code. One example, canonicalize_cpp: the existing comment explained why de-duplication mirrors expand's (s, p, o) dedup, and why a genuine overload collision still survives it — the replacement is "sorts and de-duplicates fields, bases, methods, …", which a reader can already see from the body. Several of these comments record measurements and disable-run results that were the point of writing them.

Not measured: whether the added ``` examples pass as doctests. They reference items as if from inside the crate (reassemble(&[]), ModelGraph::default() with no import), so I expect failures, but compilation fails first so I could not run them and am not claiming it.

Given #1 and #2, I don't think this is salvageable by patching — the failure mode is generation-level, and #4 means even a fixed version trades documented reasoning for restatement. My suggestion is to close it. If the 71.34% docstring-coverage warning is worth acting on, the gap is better closed by hand on the items that actually lack documentation, rather than by rewriting comments that already carry the "why".

To be explicit about status: this targets claude/ladybug-transcoding-plan-q5zbrs, whose head is still 038bbf8 and green — CI run 99 passed the full matrix (linux/macOS/Windows/wasm, clippy, fmt, prek). Nothing here has touched that branch.


Generated by Claude Code

@AdaWorldAPI

Copy link
Copy Markdown
Owner

Closing. The four defects from the review above are unchanged, and two things have since made the PR unmergeable independently of them.

The base moved. claude/ladybug-transcoding-plan-q5zbrs now points at exactly origin/main (d8ec10e), so the branch no longer holds the state this PR was cut against. Its recorded base 038bbf8 is an ancestor of main — the underlying work landed — while this PR's head fda5a4d is not. mergeable_state is dirty, and git merge-tree against main confirms real conflicts.

The harvestable content was 8 docstrings, and they are now in by hand — 1b26e67 on claude/clone-repositories-71a5sw.

Measured per-occurrence across the nine files this PR touched, 101 public items each side:

count where
this PR documents, main lacked 8 all ruff_cpp_spo/src/events.rs — Prov/Role/SymKind/ScopeKind as_str, Symbols::{intern, rows, len, is_empty}
main documents, this PR strips 2 clang_walker.rs::{bare_type_name, unary_operator_is_inc_dec}

An initial scan counted 11 undocumented; three were false positives in the scan, not real gaps. MethodOre, CppMethod and lib.rs's pub mod events each carry documentation the walk-back could not step over — a multi-line #[expect(...)] whose reason string wraps, and for the module a 53-line //! inside events.rs that the // note at the declaration site explicitly defers to. The real gap was 8, which is exactly the set this PR covered; the two counts corroborate.

Two deliberate differences in the hand-written version:

  • No examples. events.rs had 0 doctests before and has 0 after. This PR's examples use Symbols::default(), SymKind::Member, Role::OwnMember, Prov::Walk with no use line — and since lib.rs:73 is pub mod events, those are public API whose doctests would have run and failed to compile. The earlier comment flagged this as expected-but-unmeasured; reading the head confirms it.
  • The four as_str docs now carry a fact that needs all five match bodies to see: the casing splits. EventKind/ScopeKind spell PascalCase; the symbol-table triple SymKind/Role/Prov is lowercase throughout — and SymKind and Role both spell param and local, so a consumer joining on the string alone must carry the column it came from. Symbols::intern gets the invariant that bites: the key is the triple, and only the first prov survives, because re-interning returns early — so Prov::Clang after Prov::Walk does not upgrade the recorded provenance.

Gates on the replacement: fmt clean; clippy --all-targets --features libclang -- -D warnings clean (it caught one unbackticked PascalCase); RUSTDOCFLAGS=-D warnings cargo doc clean, which is what actually exercises the nine intra-doc links; cargo test --features libclang 75 + 6 passed, Doc-tests 0.

The two docs this PR would have stripped from clang_walker.rs are untouched and stay.


Generated by Claude Code

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.

1 participant