feat: redundant match arm preprocessing#139
Conversation
The Language::Json => CommentSyntax::CStyle arm was redundant since it had the same value as the wildcard arm. Now Language::Json is handled by the wildcard pattern, with the comment updated to note this. Reviewed-by: conveyor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Summary by CodeRabbit
WalkthroughRemoves a redundant Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.hermes/conveyor/work-59dfbec8/research_analysis.md:
- Around line 3-83: The document has markdownlint MD022 errors due to
inconsistent blank lines around headings; update the markdown so each heading
(e.g., "Issue Summary", "Issue Details", "Location 1: `comment_syntax()`",
"Location 2: `string_syntax()`", and other `###`/`##` headings) has exactly one
blank line before and one blank line after, removing any extra or missing blank
lines to normalize spacing and eliminate MD022 warnings.
In @.hermes/plans/2026-04-11_001536-issue-136-plan.md:
- Around line 3-55: The markdown file
.hermes/plans/2026-04-11_001536-issue-136-plan.md triggers MD022/MD031 due to
missing blank lines around headings and code fences; fix by inserting a single
blank line above and below each top-level heading (e.g., "## Goal", "## Current
Context", "## Proposed Approach", "## Step-by-Step Plan", etc.) and ensure
code-fence or <details> blocks have a blank line before and after the fenced
block or the HTML block; after adding those blank lines re-run markdownlint to
confirm MD022/MD031 are resolved.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 84d75d4c-045c-4449-8ec1-c6f6611f299d
📒 Files selected for processing (5)
.hermes/conveyor/work-59dfbec8/initial_plan.md.hermes/conveyor/work-59dfbec8/research_analysis.md.hermes/plans/2026-04-11_001536-issue-136-plan.mdcrates/diffguard-domain/src/preprocess.rscrates/diffguard-domain/tests/red_tests_work_59dfbec8.rs
| ## Issue Summary | ||
| **Issue**: GitHub issue #136 — "preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard" | ||
|
|
||
| In `crates/diffguard-domain/src/preprocess.rs`, the `Language` enum match arms in `comment_syntax()` and `string_syntax()` methods have redundant arms where `Language::Json` is explicitly matched but then also caught by the catch-all `_ =>` wildcard pattern. | ||
|
|
||
| ## Issue Details | ||
|
|
||
| ### Location 1: `comment_syntax()` (lines 69–86) | ||
|
|
||
| ```rust | ||
| pub fn comment_syntax(self) -> CommentSyntax { | ||
| match self { | ||
| Language::Python | Language::Ruby | Language::Shell => CommentSyntax::Hash, | ||
| Language::Rust | Language::Swift | Language::Scala => CommentSyntax::CStyleNested, | ||
| Language::Sql => CommentSyntax::Sql, | ||
| Language::Xml => CommentSyntax::Xml, | ||
| Language::Php => CommentSyntax::Php, | ||
| Language::Yaml | Language::Toml => CommentSyntax::Hash, | ||
| Language::Json => CommentSyntax::CStyle, // ← REDUNDANT | ||
| _ => CommentSyntax::CStyle, | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Location 2: `string_syntax()` (lines 89–111) | ||
|
|
||
| ```rust | ||
| pub fn string_syntax(self) -> CommentSyntax { | ||
| match self { | ||
| Language::Rust => StringSyntax::Rust, | ||
| Language::Python => StringSyntax::Python, | ||
| Language::JavaScript | Language::TypeScript | Language::Ruby => StringSyntax::JavaScript, | ||
| Language::Go => StringSyntax::Go, | ||
| Language::Shell => StringSyntax::Shell, | ||
| Language::Swift | Language::Scala => StringSyntax::SwiftScala, | ||
| Language::Sql => StringSyntax::Sql, | ||
| Language::Xml => StringSyntax::Xml, | ||
| Language::Php => StringSyntax::Php, | ||
| Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle, // ← GROUP PARTIALLY REDUNDANT | ||
| _ => StringSyntax::CStyle, | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| In `string_syntax()`, the line `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle` is partially redundant because the catch-all `_ => StringSyntax::CStyle` already covers `Language::Json`. However, keeping `YAML` and `TOML` explicitly listed may be intentional for documentation purposes since they are not covered by the catch-all (the catch-all covers C, Cpp, CSharp, Java, Kotlin, JavaScript, TypeScript, Go, Ruby, and Unknown). | ||
|
|
||
| ## Relevant Codebase Areas | ||
|
|
||
| ### File: `crates/diffguard-domain/src/preprocess.rs` | ||
| - **Lines 9–31**: `Language` enum definition with 21 variants (Rust, Python, JavaScript, TypeScript, Go, Ruby, C, Cpp, CSharp, Java, Kotlin, Shell, Swift, Scala, Sql, Xml, Php, Yaml, Toml, Json, Unknown) | ||
| - **Lines 69–86**: `comment_syntax()` method — `Language::Json` arm (line 83) is fully redundant | ||
| - **Lines 89–111**: `string_syntax()` method — `Language::Json` within the group arm (line 108) is redundant (caught by catch-all) | ||
|
|
||
| ### Other Files | ||
| - `fuzz/fuzz_targets/preprocess.rs` — fuzzing target for the preprocessor | ||
| - `crates/diffguard-domain/src/rules.rs` — uses `Language` for rule matching | ||
| - `crates/diffguard-domain/src/preprocess.rs` tests (lines ~1015, ~1039, ~1083, ~2647) | ||
|
|
||
| ## Dependencies and Constraints | ||
| - **No I/O constraint**: diffguard-domain must not use `std::process`, `std::fs`, or `std::env` | ||
| - **Pure functions required**: All logic testable without mocks | ||
| - **Best-effort preprocessing**: Uses C-like syntax heuristics, not full language parsers | ||
| - **Mutation testing**: The crate runs `cargo mutants` for mutation testing | ||
|
|
||
| ## Key Findings | ||
|
|
||
| 1. **`comment_syntax()` redundancy**: The `Language::Json => CommentSyntax::CStyle` arm at line 83 is fully redundant — it produces the same output (`CStyle`) as the catch-all `_ => CommentSyntax::CStyle` at line 84. | ||
|
|
||
| 2. **`string_syntax()` partial redundancy**: In the arm `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle`, only `Json` is redundant since the catch-all already returns `CStyle` for `Json`. `Yaml` and `Toml` are NOT covered by the catch-all. | ||
|
|
||
| 3. **Rustc warning**: This generates E0017 (redundant match arm) or similar warning — the arms are unreachable or provably redundant. | ||
|
|
||
| 4. **Existing test coverage**: Tests exist for `Language::Json.comment_syntax()` (line 1083) and `jsonc_double_slash_comment_ignored` (line 2647), confirming `Json` should return `CStyle`. | ||
|
|
||
| ## Recommended Fix | ||
| - In `comment_syntax()`: Remove the `Language::Json => CommentSyntax::CStyle,` line (83) | ||
| - In `string_syntax()`: Change `Language::Yaml | Language::Toml | Language::Json` to `Language::Yaml | Language::Toml` (removing `Json` from the group) | ||
|
|
||
| ## Risk Assessment | ||
| - **Low risk**: The fix is a pure refactoring — no functional change since `Json` already returns `CStyle` via the catch-all | ||
| - **Test impact**: Existing tests should continue to pass since behavior is unchanged |
There was a problem hiding this comment.
Resolve markdownlint heading spacing warnings (MD022).
Please normalize blank lines around headings in this document to satisfy markdownlint and avoid docs-lint noise.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 56-56: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 61-61: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 81-81: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.hermes/conveyor/work-59dfbec8/research_analysis.md around lines 3 - 83, The
document has markdownlint MD022 errors due to inconsistent blank lines around
headings; update the markdown so each heading (e.g., "Issue Summary", "Issue
Details", "Location 1: `comment_syntax()`", "Location 2: `string_syntax()`", and
other `###`/`##` headings) has exactly one blank line before and one blank line
after, removing any extra or missing blank lines to normalize spacing and
eliminate MD022 warnings.
| ## Goal | ||
|
|
||
| Fix clippy warning: `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` in `crates/diffguard-domain/src/preprocess.rs`. | ||
|
|
||
| ## Current Context | ||
|
|
||
| - **Issue:** #136 — `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` | ||
| - **Location:** `crates/diffguard-domain/src/preprocess.rs` | ||
| - **Branch:** `main` with uncommitted working change | ||
| - **Build status:** Passing | ||
| - **Tests:** Passing (0 failures) | ||
| - **Related work item:** `work-59dfbec8` exists in conveyor with initial plan already written | ||
|
|
||
| ## Proposed Approach | ||
|
|
||
| Remove the redundant `Language::Json` match arms in `comment_syntax()` and `string_syntax()` methods. The wildcard `_ =>` already covers `Language::Json`, so the explicit arms are dead code. | ||
|
|
||
| **Changes:** | ||
|
|
||
| 1. `comment_syntax()` (line ~82-83): Remove `Language::Json => CommentSyntax::CStyle,` — covered by `_ =>` | ||
| 2. `string_syntax()` (line ~107): Change `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,` → `Language::Yaml | Language::Toml => StringSyntax::CStyle,` — `Json` covered by `_ =>` | ||
|
|
||
| Note: The working tree already shows this diff (1 insertion, 2 deletions). This plan verifies the correct approach and defines verification steps. | ||
|
|
||
| ## Step-by-Step Plan | ||
|
|
||
| 1. **Verify current diff** matches the intended fix | ||
| 2. **Run full workspace build** (`cargo build --workspace`) — ensure no new errors | ||
| 3. **Run tests** (`cargo test --workspace`) — confirm no regressions | ||
| 4. **Run clippy** (`cargo clippy --workspace --all-targets`) — verify warning is resolved | ||
| 5. **Commit** with message referencing issue #136 | ||
|
|
||
| ## Files Likely to Change | ||
|
|
||
| - `crates/diffguard-domain/src/preprocess.rs` (1 file, ~3 lines changed) | ||
|
|
||
| ## Tests / Validation | ||
|
|
||
| - `cargo build -p diffguard-domain` — compiles cleanly | ||
| - `cargo test -p diffguard-domain` — all tests pass | ||
| - `cargo clippy -p diffguard-domain` — no redundant match arm warning | ||
|
|
||
| ## Risks | ||
|
|
||
| 1. **Strict compile mode:** If crate uses `#[deny(dead_code)]` or `#[deny(unreachable_patterns)]`, removing the arm could trigger an error. Verified: no such attributes present. | ||
| 2. **Documentation signal:** The removed arm comment (`// JSON supports comments in jsonc/json5 dialects`) served as documentation. The updated comment on line 82 (`// JSON handled by wildcard (jsonc/json5 dialects use C-style comments)`) preserves intent. | ||
|
|
||
| ## Verification Checklist | ||
|
|
||
| - [ ] `cargo build --workspace` succeeds | ||
| - [ ] `cargo test --workspace` passes | ||
| - [ ] `cargo clippy --all-targets` shows no redundant match arm warning | ||
| - [ ] `git diff` shows expected 1 insertion, 2 deletions No newline at end of file |
There was a problem hiding this comment.
Fix markdownlint spacing violations (MD022/MD031).
This file currently triggers markdownlint warnings for heading/code-fence spacing. Please add required blank lines so docs lint stays green.
Suggested markdown adjustment pattern
## Goal
+
Fix clippy warning: `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` in `crates/diffguard-domain/src/preprocess.rs`.
## Current Context
+
- **Issue:** `#136` — `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard`📝 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.
| ## Goal | |
| Fix clippy warning: `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` in `crates/diffguard-domain/src/preprocess.rs`. | |
| ## Current Context | |
| - **Issue:** #136 — `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` | |
| - **Location:** `crates/diffguard-domain/src/preprocess.rs` | |
| - **Branch:** `main` with uncommitted working change | |
| - **Build status:** Passing | |
| - **Tests:** Passing (0 failures) | |
| - **Related work item:** `work-59dfbec8` exists in conveyor with initial plan already written | |
| ## Proposed Approach | |
| Remove the redundant `Language::Json` match arms in `comment_syntax()` and `string_syntax()` methods. The wildcard `_ =>` already covers `Language::Json`, so the explicit arms are dead code. | |
| **Changes:** | |
| 1. `comment_syntax()` (line ~82-83): Remove `Language::Json => CommentSyntax::CStyle,` — covered by `_ =>` | |
| 2. `string_syntax()` (line ~107): Change `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,` → `Language::Yaml | Language::Toml => StringSyntax::CStyle,` — `Json` covered by `_ =>` | |
| Note: The working tree already shows this diff (1 insertion, 2 deletions). This plan verifies the correct approach and defines verification steps. | |
| ## Step-by-Step Plan | |
| 1. **Verify current diff** matches the intended fix | |
| 2. **Run full workspace build** (`cargo build --workspace`) — ensure no new errors | |
| 3. **Run tests** (`cargo test --workspace`) — confirm no regressions | |
| 4. **Run clippy** (`cargo clippy --workspace --all-targets`) — verify warning is resolved | |
| 5. **Commit** with message referencing issue #136 | |
| ## Files Likely to Change | |
| - `crates/diffguard-domain/src/preprocess.rs` (1 file, ~3 lines changed) | |
| ## Tests / Validation | |
| - `cargo build -p diffguard-domain` — compiles cleanly | |
| - `cargo test -p diffguard-domain` — all tests pass | |
| - `cargo clippy -p diffguard-domain` — no redundant match arm warning | |
| ## Risks | |
| 1. **Strict compile mode:** If crate uses `#[deny(dead_code)]` or `#[deny(unreachable_patterns)]`, removing the arm could trigger an error. Verified: no such attributes present. | |
| 2. **Documentation signal:** The removed arm comment (`// JSON supports comments in jsonc/json5 dialects`) served as documentation. The updated comment on line 82 (`// JSON handled by wildcard (jsonc/json5 dialects use C-style comments)`) preserves intent. | |
| ## Verification Checklist | |
| - [ ] `cargo build --workspace` succeeds | |
| - [ ] `cargo test --workspace` passes | |
| - [ ] `cargo clippy --all-targets` shows no redundant match arm warning | |
| - [ ] `git diff` shows expected 1 insertion, 2 deletions | |
| ## Goal | |
| Fix clippy warning: `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` in `crates/diffguard-domain/src/preprocess.rs`. | |
| ## Current Context | |
| - **Issue:** `#136` — `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` | |
| - **Location:** `crates/diffguard-domain/src/preprocess.rs` | |
| - **Branch:** `main` with uncommitted working change | |
| - **Build status:** Passing | |
| - **Tests:** Passing (0 failures) | |
| - **Related work item:** `work-59dfbec8` exists in conveyor with initial plan already written | |
| ## Proposed Approach | |
| Remove the redundant `Language::Json` match arms in `comment_syntax()` and `string_syntax()` methods. The wildcard `_` already covers `Language::Json`, so the explicit arms are dead code. | |
| **Changes:** | |
| 1. `comment_syntax()` (line ~82-83): Remove `Language::Json => CommentSyntax::CStyle,` — covered by `_ =>` | |
| 2. `string_syntax()` (line ~107): Change `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,` → `Language::Yaml | Language::Toml => StringSyntax::CStyle,` — `Json` covered by `_ =>` | |
| Note: The working tree already shows this diff (1 insertion, 2 deletions). This plan verifies the correct approach and defines verification steps. | |
| ## Step-by-Step Plan | |
| 1. **Verify current diff** matches the intended fix | |
| 2. **Run full workspace build** (`cargo build --workspace`) — ensure no new errors | |
| 3. **Run tests** (`cargo test --workspace`) — confirm no regressions | |
| 4. **Run clippy** (`cargo clippy --workspace --all-targets`) — verify warning is resolved | |
| 5. **Commit** with message referencing issue `#136` | |
| ## Files Likely to Change | |
| - `crates/diffguard-domain/src/preprocess.rs` (1 file, ~3 lines changed) | |
| ## Tests / Validation | |
| - `cargo build -p diffguard-domain` — compiles cleanly | |
| - `cargo test -p diffguard-domain` — all tests pass | |
| - `cargo clippy -p diffguard-domain` — no redundant match arm warning | |
| ## Risks | |
| 1. **Strict compile mode:** If crate uses `#[deny(dead_code)]` or `#[deny(unreachable_patterns)]`, removing the arm could trigger an error. Verified: no such attributes present. | |
| 2. **Documentation signal:** The removed arm comment (`// JSON supports comments in jsonc/json5 dialects`) served as documentation. The updated comment on line 82 (`// JSON handled by wildcard (jsonc/json5 dialects use C-style comments)`) preserves intent. | |
| ## Verification Checklist | |
| - [ ] `cargo build --workspace` succeeds | |
| - [ ] `cargo test --workspace` passes | |
| - [ ] `cargo clippy --all-targets` shows no redundant match arm warning | |
| - [ ] `git diff` shows expected 1 insertion, 2 deletions |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 8-8: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 10-10: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 13-13: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 16-16: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 18-18: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 43-43: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.hermes/plans/2026-04-11_001536-issue-136-plan.md around lines 3 - 55, The
markdown file .hermes/plans/2026-04-11_001536-issue-136-plan.md triggers
MD022/MD031 due to missing blank lines around headings and code fences; fix by
inserting a single blank line above and below each top-level heading (e.g., "##
Goal", "## Current Context", "## Proposed Approach", "## Step-by-Step Plan",
etc.) and ensure code-fence or <details> blocks have a blank line before and
after the fenced block or the HTML block; after adding those blank lines re-run
markdownlint to confirm MD022/MD031 are resolved.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adr-011-parse-blame-porcelain-result.md`:
- Line 26: Fix markdownlint MD031/MD022/MD047 by adding a blank line before and
after each fenced code block (e.g., the ```rust fences) and ensuring there is a
blank line above and below each heading in the document, and make sure the file
ends with exactly one trailing newline (single empty line at EOF). Apply these
fixes to all occurrences mentioned (the code fences and headings around the
reported spots) so the ADR is compliant with MD031, MD022 and MD047.
- Line 15: Replace the incorrect Clippy lint name `unnecessary_result_bool` with
the correct `clippy::unnecessary_wraps` in the ADR text; update any nearby
examples or mentions in adr-011-parse-blame-porcelain-result.md to use
`clippy::unnecessary_wraps` and adjust the short description to mention it
detects functions that unnecessarily return Result or Option when they never
return Err/None (ensure the literal string match is changed wherever
`unnecessary_result_bool` appears).
In `@crates/diffguard-core/src/xml_utils.rs`:
- Around line 76-83: The test named escape_xml_does_not_double_escape
contradicts its assertions (which expect double-escaping); rename the test
function to something like escape_xml_double_escapes_already_escaped and update
the test comment to state that already-escaped entities will be escaped again
(e.g., "Already-escaped content is escaped again / double-escaped"), making the
name and comment consistent with the assertions in the function.
In `@crates/diffguard-core/tests/property_tests_escape_xml.rs`:
- Around line 283-296: The tests define a local duplicate escape_xml which hides
regressions in the production implementation; remove the duplicated escape_xml
from the test file and instead call the real escape_xml from xml_utils.rs (or
move the proptests into xml_utils.rs under #[cfg(test)]), ensuring the test
imports or references the production function (escape_xml) so the property tests
validate the actual implementation.
In `@specs-011-parse-blame-porcelain-result.md`:
- Line 11: Polish the doc per the lint hints: reword the sentence around Line 11
and the related phrase at Line 15 for clearer wording, replace the ellipsis at
Line 25 with the preferred style (use three consecutive dots "..." or the
typographic ellipsis character consistently per project convention), and add a
trailing newline at the end of the file (Line 48) to satisfy MD047; keep
behavior text unchanged.
- Around line 1-48: The spec and PR objective are inconsistent: the spec
describes changing parse_blame_porcelain (fn parse_blame_porcelain) but the PR
actually removes a redundant Language::Json match arm in the preprocessing logic
(match on Language::Json in preprocess.rs); fix by either updating the spec to
document the preprocessing change (describe removing the Language::Json arm, its
location, and tests to update) or split into two PRs—one for the
parse_blame_porcelain refactor and one for the Language::Json removal—so each
PR's diff, signature changes (parse_blame_porcelain -> BTreeMap<u32,
BlameLineMeta>), caller updates, and test adjustments are traceable and
consistent with the spec.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 074e84f9-e4ef-4266-be89-eb9e2d52c3af
📒 Files selected for processing (8)
CHANGELOG.mdadr-011-parse-blame-porcelain-result.mdcrates/diffguard-core/src/checkstyle.rscrates/diffguard-core/src/junit.rscrates/diffguard-core/src/lib.rscrates/diffguard-core/src/xml_utils.rscrates/diffguard-core/tests/property_tests_escape_xml.rsspecs-011-parse-blame-porcelain-result.md
|
|
||
| Issue #141 reports that `parse_blame_porcelain` in `crates/diffguard/src/main.rs` (line 1768) is typed to return `Result<BTreeMap<u32, BlameLineMeta>>` but never actually returns `Err`. This is dead code — the function silently skips invalid entries via `continue` rather than propagating errors, and always reaches `Ok(out)` at line 1818. | ||
|
|
||
| Clippy detects this pattern with the `unnecessary_result_bool` lint (or equivalent): *"this function's return value is unnecessarily wrapped by `Result`"*. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct Rust Clippy lint name for functions that unnecessarily return Resultwhen they never returnErr? Is it unnecessary_result_boolorclippy::unnecessary_wraps?
💡 Result:
The correct Rust Clippy lint name for functions that unnecessarily return Result (or Option) when they never return Err (or None) is clippy::unnecessary_wraps. Neither unnecessary_result_bool nor clippy::unnecessary_wraps (without the clippy:: prefix in the name itself) is correct; the full lint is clippy::unnecessary_wraps. This pedantic lint checks private functions that only return Ok or Some, suggesting to remove the unnecessary wrapper from the return type. It explicitly handles Result returning only Ok, as shown in the source code example: rust fn get_cool_number(a: bool, b: bool) -> Result<i32, !> { // or any Error type if a && b { return Ok(50); } Ok(10) } There is no lint named unnecessary_result_bool; that appears to be a misremembering. The lint originated from issues requesting detection of "needless/unnecessary Result" and was implemented as unnecessary_wraps in Clippy version 1.50.0.
Citations:
- 1: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/unnecessary_wraps.rs
- 2: Add new lint
unnecessary_wraprust-lang/rust-clippy#6070 - 3: unnecessary_wraps is too strict rust-lang/rust-clippy#6427
- 4: unnecessary_wraps is a bad default lint rust-lang/rust-clippy#6726
- 5: https://rust-lang.github.io/rust-clippy/beta/
Update the Clippy lint name from unnecessary_result_bool to clippy::unnecessary_wraps.
unnecessary_result_bool does not exist in Clippy. The correct lint is clippy::unnecessary_wraps, which detects functions that unnecessarily return Result or Option when they never return Err or None.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adr-011-parse-blame-porcelain-result.md` at line 15, Replace the incorrect
Clippy lint name `unnecessary_result_bool` with the correct
`clippy::unnecessary_wraps` in the ADR text; update any nearby examples or
mentions in adr-011-parse-blame-porcelain-result.md to use
`clippy::unnecessary_wraps` and adjust the short description to mention it
detects functions that unnecessarily return Result or Option when they never
return Err/None (ensure the literal string match is changed wherever
`unnecessary_result_bool` appears).
| ### Changes Required | ||
|
|
||
| 1. **Function signature (line 1768):** | ||
| ```rust |
There was a problem hiding this comment.
Fix markdownlint violations in ADR formatting.
The reported MD031 (blanks around fences), MD022 (blanks around headings), and MD047 (single trailing newline) should be resolved to keep docs lint-clean.
Also applies to: 35-35, 44-44, 54-54, 73-73, 76-76, 79-79, 110-110
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 26-26: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adr-011-parse-blame-porcelain-result.md` at line 26, Fix markdownlint
MD031/MD022/MD047 by adding a blank line before and after each fenced code block
(e.g., the ```rust fences) and ensuring there is a blank line above and below
each heading in the document, and make sure the file ends with exactly one
trailing newline (single empty line at EOF). Apply these fixes to all
occurrences mentioned (the code fences and headings around the reported spots)
so the ADR is compliant with MD031, MD022 and MD047.
| fn escape_xml_does_not_double_escape() { | ||
| // Already-escaped content should not be double-escaped | ||
| assert_eq!(escape_xml("&"), "&amp;"); | ||
| assert_eq!(escape_xml("<script>"), "&lt;script&gt;"); | ||
| assert_eq!( | ||
| escape_xml(""quoted""), | ||
| "&quot;quoted&quot;" | ||
| ); |
There was a problem hiding this comment.
Test name/comment contradict asserted behavior.
Lines 76-83 assert that already-escaped entities are escaped again, but the test name/comment says the opposite. Please rename and reword to avoid misleading future maintenance.
✏️ Suggested fix
- fn escape_xml_does_not_double_escape() {
- // Already-escaped content should not be double-escaped
+ fn escape_xml_double_escapes_existing_entities() {
+ // Already-escaped content is escaped again (non-idempotent by design)
assert_eq!(escape_xml("&"), "&amp;");
assert_eq!(escape_xml("<script>"), "&lt;script&gt;");📝 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.
| fn escape_xml_does_not_double_escape() { | |
| // Already-escaped content should not be double-escaped | |
| assert_eq!(escape_xml("&"), "&amp;"); | |
| assert_eq!(escape_xml("<script>"), "&lt;script&gt;"); | |
| assert_eq!( | |
| escape_xml(""quoted""), | |
| "&quot;quoted&quot;" | |
| ); | |
| fn escape_xml_double_escapes_existing_entities() { | |
| // Already-escaped content is escaped again (non-idempotent by design) | |
| assert_eq!(escape_xml("&"), "&amp;"); | |
| assert_eq!(escape_xml("<script>"), "&lt;script&gt;"); | |
| assert_eq!( | |
| escape_xml(""quoted""), | |
| "&quot;quoted&quot;" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-core/src/xml_utils.rs` around lines 76 - 83, The test named
escape_xml_does_not_double_escape contradicts its assertions (which expect
double-escaping); rename the test function to something like
escape_xml_double_escapes_already_escaped and update the test comment to state
that already-escaped entities will be escaped again (e.g., "Already-escaped
content is escaped again / double-escaped"), making the name and comment
consistent with the assertions in the function.
| fn escape_xml(s: &str) -> String { | ||
| let mut out = String::with_capacity(s.len()); | ||
| for c in s.chars() { | ||
| match c { | ||
| '&' => out.push_str("&"), | ||
| '<' => out.push_str("<"), | ||
| '>' => out.push_str(">"), | ||
| '"' => out.push_str("""), | ||
| '\'' => out.push_str("'"), | ||
| _ => out.push(c), | ||
| } | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
Property tests are validating a duplicate implementation, not production code.
Lines 283-296 reimplement escape_xml locally, so this test suite can pass even if crates/diffguard-core/src/xml_utils.rs regresses. Please bind these tests to the real implementation (e.g., move proptests into src/xml_utils.rs under #[cfg(test)]).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-core/tests/property_tests_escape_xml.rs` around lines 283 -
296, The tests define a local duplicate escape_xml which hides regressions in
the production implementation; remove the duplicated escape_xml from the test
file and instead call the real escape_xml from xml_utils.rs (or move the
proptests into xml_utils.rs under #[cfg(test)]), ensuring the test imports or
references the production function (escape_xml) so the property tests validate
the actual implementation.
| # Specification: Remove Unnecessary Result from parse_blame_porcelain | ||
|
|
||
| ## Overview | ||
|
|
||
| Refactor `parse_blame_porcelain` function to return `BTreeMap<u32, BlameLineMeta>` directly instead of `Result<BTreeMap<u32, BlameLineMeta>>`, since the function never returns `Err`. | ||
|
|
||
| ## Feature / Behavior Description | ||
|
|
||
| The function `parse_blame_porcelain` parses git blame porcelain output and returns a mapping from line numbers to their metadata. Currently it returns `Result<...>` but always succeeds. After the change, it returns the `BTreeMap` directly. | ||
|
|
||
| **No behavioral change** — the function continues to silently skip malformed entries via `continue` statements. Only the type signature changes. | ||
|
|
||
| ## Non-Goals | ||
|
|
||
| - No changes to parsing logic or error handling behavior | ||
| - No new functionality | ||
| - No changes to other functions or their signatures | ||
|
|
||
| ## Changes | ||
|
|
||
| | Location | Before | After | | ||
| |----------|--------|-------| | ||
| | Line 1768 (function signature) | `fn parse_blame_porcelain(blame_text: &str) -> Result<BTreeMap<u32, BlameLineMeta>>` | `fn parse_blame_porcelain(blame_text: &str) -> BTreeMap<u32, BlameLineMeta>` | | ||
| | Line 1818 (return) | `Ok(out)` | `out` | | ||
| | Lines 1861-1862 (caller) | `.with_context(...)?` | Remove `.with_context()` and `?` | | ||
| | Line 4068 (test) | `.expect("parse")` | Remove `.expect()` | | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| 1. `cargo clippy -p diffguard` reports no warnings related to `parse_blame_porcelain` | ||
| 2. `cargo test -p diffguard` passes with all tests green, including `parse_blame_porcelain_extracts_line_metadata` | ||
| 3. The function signature is `fn parse_blame_porcelain(&str) -> BTreeMap<u32, BlameLineMeta>` | ||
| 4. All call sites are updated to handle the non-Result return type | ||
|
|
||
| ## Dependencies | ||
|
|
||
| - `anyhow` (already in dependency tree for `Result` type alias) | ||
| - No new dependencies required | ||
|
|
||
| ## Test Plan | ||
|
|
||
| 1. **Existing test** (`parse_blame_porcelain_extracts_line_metadata`): Update to call function without `.expect()` and verify it passes | ||
| 2. **Clippy check**: Run `cargo clippy -p diffguard` to confirm no lint | ||
| 3. **Full test suite**: Run `cargo test -p diffguard` to ensure no regressions | ||
|
|
||
| ## File Changes | ||
|
|
||
| - `crates/diffguard/src/main.rs` — 4 locations (signature, return, caller, test) No newline at end of file |
There was a problem hiding this comment.
PR scope appears mismatched with the stated objective.
This spec documents a refactor in crates/diffguard/src/main.rs (parse_blame_porcelain), but the PR objective describes removing a redundant Language::Json match arm in crates/diffguard-domain/src/preprocess.rs. Please either align this spec to the preprocessing change or split this into a separate PR/work item to keep traceability clean.
🧰 Tools
🪛 LanguageTool
[style] ~11-~11: Try moving the adverb to make the sentence clearer.
Context: ...No behavioral change — the function continues to silently skip malformed entries via continue statements. Only the typ...
(SPLIT_INFINITIVE)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ... - No changes to parsing logic or error handling behavior - No new functionality...
(QB_NEW_EN_HYPHEN)
[style] ~25-~25: Consider using the typographical ellipsis character here instead.
Context: ... | out | | Lines 1861-1862 (caller) | .with_context(...)? | Remove .with_context() and ? |...
(ELLIPSIS)
🪛 markdownlint-cli2 (0.22.0)
[warning] 48-48: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs-011-parse-blame-porcelain-result.md` around lines 1 - 48, The spec and
PR objective are inconsistent: the spec describes changing parse_blame_porcelain
(fn parse_blame_porcelain) but the PR actually removes a redundant
Language::Json match arm in the preprocessing logic (match on Language::Json in
preprocess.rs); fix by either updating the spec to document the preprocessing
change (describe removing the Language::Json arm, its location, and tests to
update) or split into two PRs—one for the parse_blame_porcelain refactor and one
for the Language::Json removal—so each PR's diff, signature changes
(parse_blame_porcelain -> BTreeMap<u32, BlameLineMeta>), caller updates, and
test adjustments are traceable and consistent with the spec.
|
|
||
| The function `parse_blame_porcelain` parses git blame porcelain output and returns a mapping from line numbers to their metadata. Currently it returns `Result<...>` but always succeeds. After the change, it returns the `BTreeMap` directly. | ||
|
|
||
| **No behavioral change** — the function continues to silently skip malformed entries via `continue` statements. Only the type signature changes. |
There was a problem hiding this comment.
Address the reported doc/lint nits in this spec.
The static hints are valid here: wording polish on Line 11/Line 15, ellipsis style on Line 25, and missing trailing newline on Line 48 (MD047).
Also applies to: 15-15, 25-25, 48-48
🧰 Tools
🪛 LanguageTool
[style] ~11-~11: Try moving the adverb to make the sentence clearer.
Context: ...No behavioral change — the function continues to silently skip malformed entries via continue statements. Only the typ...
(SPLIT_INFINITIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs-011-parse-blame-porcelain-result.md` at line 11, Polish the doc per the
lint hints: reword the sentence around Line 11 and the related phrase at Line 15
for clearer wording, replace the ellipsis at Line 25 with the preferred style
(use three consecutive dots "..." or the typographic ellipsis character
consistently per project convention), and add a trailing newline at the end of
the file (Line 48) to satisfy MD047; keep behavior text unchanged.
EffortlessSteven
left a comment
EffortlessSteven
left a comment
There was a problem hiding this comment.
LGTM — small, focused, correct. The redundant Language::Json arm is cleanly removed by consolidating it into the wildcard. Merge.
|
Closing — this PR has cross-contamination from unrelated conveyor work items and fails Gate: Issue linked (references work item ID instead of a GitHub issue). Replacement: PR #251 with only the 2 targeted preprocess.rs changes. CI passes locally. |
EffortlessSteven
deleted the
feat/work-59dfbec8/preprocess.rs--redundant-match-arm---lan
branch
1 participant
Automated PR from conveyor work-59dfbec8