Skip to content

Refactor attach_orphan_specifiers; add attach_to_next_fence helper#273

Merged
leynos merged 20 commits intomainfrom
refactor-orphan-specifiers-iterator-h1724j
Apr 27, 2026
Merged

Refactor attach_orphan_specifiers; add attach_to_next_fence helper#273
leynos merged 20 commits intomainfrom
refactor-orphan-specifiers-iterator-h1724j

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented Apr 23, 2026

Summary

  • Reworks attach_orphan_specifiers to use a Peekable iterator with a dedicated helper to attach to the next fence
  • Removes the orphan_specifier_target helpers and simplifies the control flow
  • Preserves existing behavior while improving handling of blank lines and attachment logic
  • Docs: README.md, docs/developers-guide.md, and docs/users-guide.md updated to reflect the new behavior and helper

Changes

Core Refactor

  • Convert lines iteration to a Peekable iterator: let mut lines = lines.iter().peekable();
  • Replace index-based traversal with peeking, buffering surrounding blank lines and attaching when a following fence with a null language is found
  • Attach orphaned language specifiers to the opening fence line when the fence has a null language, preserving indentation
  • Maintain previous behavior for cases where no suitable fence is found (lines are emitted unchanged)

Helper

  • Introduce attach_to_next_fence as a helper to perform the lookahead logic
  • Keep existing attach_specifier_to_fence helper for building the final fence line

Docs

  • Update README.md to describe attaching orphan specifiers and the normalized fence behavior
  • Update docs/developers-guide.md to explain the architecture changes, describe attach_to_next_fence, and centralize lookahead logic
  • Update docs/users-guide.md to document fence normalization and orphan-specifier attachment behavior

Cleanup

  • Remove helper functions: orphan_specifier_target and orphan_specifier_target_without_language
  • Reduce code complexity by centralizing logic in a single pass over a Peekable iterator

Tests

  • Expand test coverage for orphan specifier handling; added tests for blank-line handling and multiple blank lines dropped on attachment
  • Updated existing tests to reflect the new helper-based flow
  • Tests ensure orphaned language specifiers attach to the following unlabelled fence when possible and that non-attachable cases are preserved

Behavior

  • Handles multiple blank lines between an orphan specifier and the following fence
  • If no appropriate fence is found, the orphan specifier line is emitted as-is along with any collected blank lines
  • Keeps existing attachment semantics unchanged for non-orphan lines

Test plan

  • Build and run existing test suite (cargo test)
  • Validate orphaned language specifiers attach to the following fence when that fence has a null language
  • Validate that blank lines between the orphan specifier and the fence are preserved
  • Validate that multiple blank lines are dropped when an attachment occurs (new test)
  • Validate behavior when there is no following fence or no null-language fence to attach to
  • Manual review on representative inputs

📎 Task: https://www.devboxer.com/task/4514e7a4-abf4-4250-a7d4-aed05c8c7898

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Refactor orphan-specifier handling to use a Peekable iterator and delegate lookahead/attachment to attachment::attach_to_next_fence, which buffers intervening blank lines and either rewrites the next unlabeled opening fence with the specifier (dropping buffered blanks) or emits the original specifier plus buffered blanks unchanged. Add attachment module, unit tests, and developer/user docs updates.

Changes

Cohort / File(s) Summary
Iterator refactor & orchestration
src/fences.rs
Replace index-based scanning with a Peekable iterator; call attachment::attach_to_next_fence for orphan-specifier handling; preserve public signatures and emit original lines when attachment fails.
Attachment helper module
src/fences/attachment.rs
Add new module with attach_to_next_fence and AttachmentOutcome implementing lookahead over blank lines, attachability detection for the next unlabeled opening fence, indentation selection rules, fence rewriting on attach, preservation on failure, and FenceTracker updates.
Attachment unit tests
src/fences/tests.rs
Add rstest-driven unit tests for attach_to_next_fence covering Attached vs Preserved outcomes, iterator consumption, FenceTracker transitions and indentation handling.
Integration tests: orphan specifiers
tests/fences.rs
Replace single-case test with parameterised tests covering single and multiple blank-line buffering and non-attachable following content; assert correct attachment or preservation behaviour.
Docs: developer & user guides + README
docs/developers-guide.md, docs/users-guide.md, README.md
Document fences stage behaviour: safe compression to three backticks, orphan-specifier lookahead semantics, buffering and conditional dropping of blank lines on successful attachment, and indentation selection rules.
Makefile robustness
Makefile
Make CARGO and MDLINT defaults resolve via command -v with explicit fallbacks to environment-robust paths.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(220,230,241,0.5)
  participant Spec as "Orphan Specifier Line"
  end
  rect rgba(240,255,240,0.5)
  participant Att as "attach_to_next_fence (Peekable)"
  end
  rect rgba(255,250,230,0.5)
  participant Buf as "Buffered Blank Lines"
  end
  rect rgba(255,230,230,0.5)
  participant Fence as "Next Non-Blank Line / Opening Fence"
  end
  rect rgba(245,245,245,0.5)
  participant Out as "Output Stream"
  end

  Spec->>Att: invoke with specifier, spec_indent, specifier_line, tracker, out
  Att->>Att: clone/peek and buffer intervening blank lines into Buf
  Att->>Fence: peek next non-blank line
  alt next is unlabeled opening fence (language null/empty)
    Att->>Fence: consume fence line
    Att->>Att: choose indentation (fence vs specifier rules)
    Att->>Fence: rewrite fence line with appended specifier
    Att->>Out: emit rewritten fence
    Att->>Buf: discard buffered blanks
    Att->>Fence: update FenceTracker (enter fence)
    Att-->>Spec: return AttachmentOutcome::Attached
  else no eligible fence found
    Att->>Out: emit original specifier line
    Att->>Out: emit buffered blank lines
    Att->>Fence: leave FenceTracker unchanged
    Att-->>Spec: return AttachmentOutcome::Preserved
  end
Loading

Possibly related issues

Poem

✨ Orphan tags now find their place,
Peekable lookahead keeps the pace,
Buffered blanks released or kept,
Fences don language adept,
Parsing tidy, neatly traced.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning, 5 inconclusive)

Check name Status Explanation Resolution
Unit Architecture ❌ Error The attach_to_next_fence function violates separation of concerns by blending query logic (lookahead) with command logic (output emission and state mutation) within a single helper that takes mutable references and performs side-effects internally. Refactor into two distinct units: a pure query function returning lookahead results and attachment decision, and a command function accepting that decision to perform emission and tracker mutation. Make all side-effects explicit in the caller's control flow.
Testing (Unit) ⚠️ Warning PR adds src/fences/tests.rs containing direct unit tests for private helper attach_to_next_fence (pub(super)), violating the requirement that unit tests must not drive internal helpers and must exercise the appropriate functional boundary. Remove src/fences/tests.rs entirely. Ensure tests/fences.rs comprehensively tests the public attach_orphan_specifiers function's observable behaviour at the correct functional boundary.
Testing (Overall) ❓ Inconclusive Test summaries claim coverage of blank-line dropping, preservation, and iterator management, but actual test code implementation cannot be verified from provided summaries alone. Retrieve and inspect actual test implementations in src/fences/tests.rs and tests/fences.rs to confirm assertions explicitly validate blank-line presence/absence in output and state mutations.
User-Facing Documentation ❓ Inconclusive Verify that new or changed user-facing functionality related to fences/orphan specifiers is clearly documented in docs/users-guide.md with appropriate examples and usage details. Provide the actual output from the shell commands to determine whether documentation exists and is complete. Analyse whether docs/users-guide.md contains --fences section, whether README.md appropriately signposts this functionality, and verify no language variant synchronisation issues exist.
Developer Documentation ❓ Inconclusive PR documents new internal APIs and architectural changes in developers-guide.md, but does not explicitly confirm Makefile build-requirement changes are documented or that formal roadmap/execplan documents exist with completed items marked off. Inspect docs/developers-guide.md to verify Makefile tooling changes are documented. Search repository for ROADMAP or execplan documents and confirm completion items are formally marked off.
Module-Level Documentation ❓ Inconclusive The raw summary does not explicitly confirm module-level documentation exists for the new production module src/fences/attachment.rs, preventing verification of documentation completeness. Supply the actual code contents of src/fences/attachment.rs to verify module-level docstrings and public documentation are present and complete.
Testing (Compile-Time / Ui) ❓ Inconclusive Unable to assess test requirements without access to pull request changes and repository structure. Provide the pull request diff and clarify whether compile-time or snapshot testing applies to the modifications.
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring work: extracting the orphan specifier attachment logic into a dedicated helper function and converting to a Peekable iterator pattern.
Description check ✅ Passed The description comprehensively covers the refactoring changes, documents the new attach_to_next_fence helper, lists documentation updates, and describes test expansions—all matching the changeset content.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Property / Proof) ✅ Passed The PR refactors attach_orphan_specifiers with iterator-based control flow, introducing invariants around blank-line buffering, FenceTracker state transitions, and indentation preservation. The unit tests adequately verify critical behavioural properties including blank-line handling and iterator exhaustion across primary code paths.
Domain Architecture ✅ Passed The refactoring maintains proper domain architecture by extracting orphan specifier attachment into a private helper with explicit state management and clear contracts, shifting from index-based to iterator-based abstractions.
Observability ✅ Passed The PR refactors internal implementation details of a local file-processing CLI tool without changing observable behaviour. The tool processes markdown locally without network calls or distributed systems concerns, making observability instrumentation inappropriate.
Security And Privacy ✅ Passed The pull request introduces no security vulnerabilities or privacy risks. Changes are restricted to Markdown fence parsing logic using safe string operations on internal fence marker data, improved tool resolution in the Makefile following secure PATH practices, and documentation updates with no exposed credentials or sensitive information.
Performance And Resource Use ✅ Passed Refactoring replaces index-based scanning with peekable iterator pattern, reducing net lines whilst centralising lookahead logic with bounded cost and no algorithmic regression.
Concurrency And State ✅ Passed PR contains no concurrency, async execution, threading, locks, or shared mutable state. Code is straightforward single-threaded text processing using standard Rust ownership patterns with explicit mutable references passed through linear call chains.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor-orphan-specifiers-iterator-h1724j

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

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Apr 23, 2026

Reviewer's Guide

Refactors attach_orphan_specifiers to iterate with a Peekable iterator instead of index-based access, inlining the orphan-specifier targeting logic while preserving existing behavior and improving handling of blank lines and fence attachment conditions.

Class diagram for functions in fences module after refactor

classDiagram
    class FencesModule {
        +attach_orphan_specifiers(lines: Vec_String) Vec_String
        +attach_specifier_to_fence(fence_line: str, specifier: str, spec_indent: str) String
        -normalize_specifier(line: str) Tuple_String_String
        -FENCE_RE Regex
        -ORPHAN_LANG_RE Regex
        -is_null_lang(lang: str) bool
    }

    class OrphanSpecifierHelpersOld {
        -orphan_specifier_target(lines: Vec_String, start: usize) Option_usize
        -orphan_specifier_target_without_language(lines: Vec_String, start: usize) Option_usize
    }

    FencesModule .. OrphanSpecifierHelpersOld : refactor_inlines_logic
Loading

File-Level Changes

Change Details Files
Refactor attach_orphan_specifiers to use a Peekable iterator with inlined orphan-specifier attachment logic.
  • Replace index-based while loop over lines with a Peekable iterator over &String values.
  • Detect orphan language specifier lines only when preceded by either no output or a blank line in the output buffer.
  • When an orphan specifier is found, collect following blank lines via peeking, then look ahead to a non-blank line to decide whether to attach.
src/fences.rs
Improve attachment behavior for orphan specifiers while preserving previous semantics where no suitable fence exists.
  • On encountering a non-blank lookahead line, check FENCE_RE and extract the language capture group for null-language detection.
  • If the next non-blank line is an opening fence with a null language, consume it and push a synthesized fence line with the attached specifier and original indentation.
  • If no appropriate fence is found, emit the original orphan specifier line followed by any collected blank lines unchanged.
src/fences.rs
Remove now-redundant helper functions and simplify control flow.
  • Delete orphan_specifier_target and orphan_specifier_target_without_language helpers that implemented index-based lookahead.
  • Consolidate the orphan-specifier targeting and fence matching logic into the main attach_orphan_specifiers loop.
  • Reduce manual index manipulation and early-return branches in favor of clearer, linear iteration.
src/fences.rs

Assessment against linked issues

Issue Objective Addressed Explanation
#102 Refactor attach_orphan_specifiers to use a state machine-style loop over a Peekable iterator for one-pass lookahead instead of index-based traversal.
#102 Remove the reverse-search and buffer manipulation logic (including helper-based target search) previously used to attach orphan specifiers.
#102 Preserve existing observable behavior and keep all tests passing after the refactor.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@leynos leynos marked this pull request as ready for review April 23, 2026 00:27
sourcery-ai[bot]

This comment was marked as resolved.

@coderabbitai coderabbitai Bot added the Issue label Apr 23, 2026
coderabbitai[bot]

This comment was marked as resolved.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Apr 23, 2026

@coderabbitai re:

Preserve intervening blank lines when attaching orphan language specifiers; keep public signature unchanged.

Is this not undesirable behaviour?

It is also not requested by #102

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me fetch issue #102 and examine the actual code to give a well-grounded answer.


🏁 Script executed:

#!/bin/bash
# Fetch issue `#102`
gh issue view 102 --repo leynos/mdtablefix

echo "---DIFF---"
# Show the relevant diff in fences.rs on this PR branch
git diff origin/main -- src/fences.rs

Length of output: 8357


@leynos You are right on both counts.

It was not requested by #102. The sample implementation in that issue only peeks at the immediate next line — it contains no blank-line handling whatsoever. Blank-line tolerance was introduced as a side-effect of the refactor, not as a stated goal.

It is also undesirable. The original documented behaviour was explicit:

Blank lines between the specifier and the fence are skipped.

The new code buffers those blank lines into blank_lines and unconditionally re-emits them (via out.extend(blank_lines)) whether attachment succeeds or not. The updated test expectations — each adding a leading \n before the rewritten fence block — confirm this is a genuine behaviour change, not a preservation of existing behaviour.

Emitting those blank lines into the output means a blank line can appear between the orphan specifier position and its attached fence line, which is a rendered-Markdown observable change and almost certainly not what users expect.

The fix should drop the buffered blank lines on a successful attachment (or simply not buffer them at all, matching the original skip-blank-lines semantics). Something along these lines:

fn attach_to_next_fence<'a, I>(
    lines: &mut std::iter::Peekable<I>,
    specifier: &str,
    indent: &str,
    out: &mut Vec<String>,
    specifier_line: &str,
) where
    I: Iterator<Item = &'a String>,
{
    let mut blank_lines: Vec<String> = Vec::new();

    while let Some(next_line) = lines.peek() {
        if next_line.trim().is_empty() {
            if let Some(blank_line) = lines.next() {
                blank_lines.push(blank_line.clone());
            }
            continue;
        }

        if let Some(captures) = FENCE_RE.captures(next_line) {
            let lang = captures.get(3).map_or("", |m| m.as_str());
            if is_null_lang(lang)
                && let Some(fence_line) = lines.next()
            {
                // Drop blank_lines — original behaviour skipped them.
                out.push(attach_specifier_to_fence(fence_line, specifier, indent));
                return;
            }
        }

        break;
    }

    // No suitable fence found — emit the specifier and any blanks verbatim.
    out.push(specifier_line.to_string());
    out.extend(blank_lines);
}

This restores the skip-blank-lines semantics on a successful attachment whilst still correctly emitting the original specifier line plus any blank lines when no eligible fence is found.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Apr 23, 2026

@coderabbitai Have the following now been resolved?

src/fences.rs (1)

1-6: 🧹 Nitpick | 🔵 Trivial
Update module-level doc comment to mention orphan specifier attachment.
The module documentation only describes compress_fences. Add a sentence describing attach_orphan_specifiers and its purpose to give readers a complete picture of the module's functionality.
As per coding guidelines: "Every module must begin with a //! doc comment that explains the module's purpose and utility."

📝 Proposed documentation update
//! Pre-processing utilities for normalizing fenced code block delimiters.
 //!
 //! `compress_fences` reduces any sequence of three or more backticks or
 //! tildes followed by optional language identifiers to exactly three
 //! backticks.
-//! It preserves indentation and the language list.
+//! It preserves indentation and the language list.
+//!
+//! `attach_orphan_specifiers` reattaches language specifiers that appear on
+//! their own line before a fence, preserving any intervening blank lines.
🤖 Prompt for AI Agents

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
head -20 src/fences.rs

Length of output: 887


@leynos Yes, this has been resolved. The module-level doc comment now reads:

`attach_orphan_specifiers` then finds orphaned fence specifier lines and attaches them to the following fence, preserving the retained indentation and normalized language specifier.

Both functions are documented, satisfying the coding guideline. Note also that the text correctly omits any mention of blank-line preservation — consistent with the earlier discussion about restoring the skip-blank-lines semantics on successful attachment.

@leynos leynos changed the title Refactor attach_orphan_specifiers to use Peekable iterator Refactor attach_orphan_specifiers with helper; remove target helpers Apr 23, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 `@src/fences.rs`:
- Around line 183-186: Update the Rustdoc sentence in the doc block that begins
"When the fence is unindented, the specifier's indentation..." to state that
successful attachment drops intervening blank lines rather than preserves them;
specifically, change the phrase "preserving any intervening blank lines" to
"dropping any intervening blank lines" so the description of fence attachment
behavior accurately reflects the implementation.

In `@tests/fences.rs`:
- Around line 203-208: Replace the duplicate test by converting the two similar
tests into a single parameterized rstest: remove the standalone test
attaches_orphan_specifier_drops_multiple_blank_lines_on_successful_attachment
and merge its input case with the existing compressed-flow blank-line test by
creating a #[rstest] function (e.g. fixes_orphaned_specifier_with_blank_lines)
that takes a #[case] input Vec<String>, add both inputs (one blank line and two
blank lines) as #[case(...)] attributes, and inside call compress_fences(&input)
then attach_orphan_specifiers(...) and assert_eq! against
lines_vec!["```rust","fn main() {}","```"]; this eliminates duplication while
keeping compress_fences and attach_orphan_specifiers behavior covered.
🪄 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: fa5cae04-2928-4010-8a30-537816203225

📥 Commits

Reviewing files that changed from the base of the PR and between 7987eb1 and 54a0f06.

📒 Files selected for processing (2)
  • src/fences.rs
  • tests/fences.rs

Comment thread src/fences.rs Outdated
Comment thread tests/fences.rs Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/fences.rs`:
- Around line 68-74: The test suite lacks a fallback case covering
attach_to_next_fence behavior when buffered blank lines follow an orphan
specifier but the next content is an already-labelled fence or plain text; add a
new rstest in tests/fences.rs that passes inputs like
["Rust","","","```python","print('hi')","```"] and ["Rust","","not a fence"]
through compress_fences and attach_orphan_specifiers and assert the output
equals the original input to ensure orphan specifier plus blank lines remain
unchanged; reference the existing test fixes_orphaned_specifier_with_blank_lines
to mirror structure and use attach_orphan_specifiers/compress_fences in the
assertion.
🪄 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: 9d00aac1-ac2d-42ca-9415-28b76c4c0adb

📥 Commits

Reviewing files that changed from the base of the PR and between 54a0f06 and 9348bba.

📒 Files selected for processing (2)
  • src/fences.rs
  • tests/fences.rs

Comment thread tests/fences.rs
@leynos

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 109-111: The wording says "strictly extends" but the implemented
predicate is spec_indent.starts_with(fence_indent), which allows equal
indentation; update the prose to match the code by replacing "strictly extends"
with language like "is equal to or extends" (or "extends or is equal to") and
explicitly mention that equality is permitted so readers understand
spec_indent.starts_with(fence_indent) covers both equal and deeper indentation;
ensure the sentence referencing spec_indent.starts_with(fence_indent) remains
and adjust surrounding text for clarity.
🪄 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: b616443f-e0e6-4007-9bd7-d1340540db30

📥 Commits

Reviewing files that changed from the base of the PR and between 8a1cbda and 19e829b.

📒 Files selected for processing (1)
  • docs/developers-guide.md

Comment thread docs/developers-guide.md Outdated
@devboxerhub devboxerhub Bot force-pushed the refactor-orphan-specifiers-iterator-h1724j branch from 252b3b7 to c2e1c1e Compare April 23, 2026 19:52
@leynos leynos changed the title Refactor attach_orphan_specifiers with helper; remove target helpers Refactor attach_orphan_specifiers; add attach_to_next_fence helper Apr 23, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Line 61: In the sentence fragment shown ("Together these helpers make the rest
of the processing pipeline deal with a"), add a comma after the preceding
sentence-ending word "fence" so the joined sentence reads "fence. Together,
these helpers ..."; edit the markdown line containing "Together these helpers"
to insert the comma after "Together" to produce "Together, these helpers" for
correct clause separation and improved readability.
- Line 58: Replace the redundant phrase "specifier lines that appear on their
own line before an unlabeled opening" with the concise wording "specifier lines
that appear on a separate line before an unlabelled opening fence"; locate the
existing sentence containing the exact original phrase and update it to the new
text, ensuring spelling uses "unlabelled" and "separate line" as requested.
- Around line 109-112: Update the sentence describing the indentation comparison
to use active voice and clearer wording: change "is equal to or extends" to
"matches or extends" and state that spec_indent.starts_with(fence_indent) checks
prefix membership (i.e., whether fence_indent is a prefix of spec_indent,
including equality). Replace "Equality is permitted here" with an active
phrasing such as "This allows equal indentation" and keep the explanation that
the same predicate covers both matching and deeper indentation. Ensure the
updated text references the method spec_indent.starts_with(fence_indent) exactly
so readers can locate the check.
🪄 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: bfb0d4c1-eb21-4a09-beca-75faff8e636e

📥 Commits

Reviewing files that changed from the base of the PR and between 19e829b and 252b3b7.

📒 Files selected for processing (2)
  • README.md
  • docs/developers-guide.md

Comment thread docs/developers-guide.md Outdated
Comment thread docs/developers-guide.md Outdated
Comment thread docs/developers-guide.md Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
docs/developers-guide.md (1)

102-106: ⚠️ Potential issue | 🟡 Minor

Use en-GB spelling and fix clause punctuation in this paragraph.

Replace unlabeled with unlabelled, and add the comma after Together for consistency and readability.

📝 Proposed doc patch
-  specifier lines that appear on their own line before an unlabeled opening
+  specifier lines that appear on their own line before an unlabelled opening
   fence.
 
-Together these helpers make the rest of the processing pipeline deal with a
+Together, these helpers make the rest of the processing pipeline deal with a

Triage: [type:spelling] [type:syntax/md] [type:docstyle]

As per coding guidelines: "docs/**/*.{md,rst,txt}: Use British English based on Oxford English Dictionary locale (en-GB)" and "docs/**/*.md: Follow markdownlint recommendations for Markdown files."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/developers-guide.md` around lines 102 - 106, Update the paragraph text
to use en-GB spelling and fix punctuation: change the word "unlabeled" to
"unlabelled" in the sentence containing "specifier lines that appear on their
own line before an unlabeled opening fence." and insert a comma after the word
"Together" at the start of the following sentence so it reads "Together, these
helpers..."; ensure no other wording changes are made.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@docs/developers-guide.md`:
- Around line 102-106: Update the paragraph text to use en-GB spelling and fix
punctuation: change the word "unlabeled" to "unlabelled" in the sentence
containing "specifier lines that appear on their own line before an unlabeled
opening fence." and insert a comma after the word "Together" at the start of the
following sentence so it reads "Together, these helpers..."; ensure no other
wording changes are made.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7fc3bd19-7d93-4efc-9878-2d108d3e6ae4

📥 Commits

Reviewing files that changed from the base of the PR and between 252b3b7 and c2e1c1e.

📒 Files selected for processing (4)
  • README.md
  • docs/developers-guide.md
  • src/fences.rs
  • tests/fences.rs

@devboxerhub devboxerhub Bot force-pushed the refactor-orphan-specifiers-iterator-h1724j branch from c2e1c1e to 570b912 Compare April 23, 2026 22:10
@leynos leynos changed the title Refactor attach_orphan_specifiers; add attach_to_next_fence helper Refactor attach_orphan_specifiers; add attach_to_next_fence helper; update docs Apr 23, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
docs/developers-guide.md (2)

221-224: 🧹 Nitpick | 🔵 Trivial

Clarify indentation predicate wording.

The phrase "is equal to or extends" is slightly awkward. Rephrase to use active voice and clarify the starts_with semantics.

Triage: [type:docstyle]

📝 Proposed revision
-- If the specifier's indentation is equal to or extends the fence's, checked
-  via `spec_indent.starts_with(fence_indent)`, the specifier indentation is
-  used. Equality is permitted here, so the same predicate covers both matching
-  and deeper indentation.
+- If the specifier's indentation matches or extends the fence's indentation,
+  checked via `spec_indent.starts_with(fence_indent)`, the specifier
+  indentation is used. The `starts_with` predicate permits equality, so the
+  same check covers both matching and deeper indentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/developers-guide.md` around lines 221 - 224, Rewrite the sentence to use
active voice and clearly explain the predicate: state that if spec_indent
starts_with fence_indent then the specifier’s indentation is considered to
extend (or be the same as) the fence’s indentation and therefore is used;
mention that equality is allowed so the predicate covers both exact matches and
deeper (longer) indentations and reference the symbols spec_indent,
fence_indent, and starts_with in the sentence for clarity.

173-175: ⚠️ Potential issue | 🟡 Minor

Insert comma after "fence" for clause separation.

Add a comma after "fence." to separate the two independent statements.

Triage: [type:grammar]

📝 Proposed fix
-Together these helpers make the rest of the processing pipeline deal with a
+Together, these helpers make the rest of the processing pipeline deal with a
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/developers-guide.md` around lines 173 - 175, The sentence "Together
these helpers make the rest of the processing pipeline deal with a single
normalized fence form and avoid carrying separate logic for detached specifier
lines." needs a comma after "fence" to separate the two independent clauses;
update the sentence so it reads "...deal with a single normalized fence, form
and avoid..." — actually place the comma immediately after "fence" in the phrase
"single normalized fence form" to produce "single normalized fence, form" to
satisfy the reviewer comment (adjust surrounding wording if needed to keep
grammatical sense while inserting the comma next to the token "fence").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 170-171: Replace the US spelling "unlabeled" with the
en-GB-oxendict spelling "unlabelled" in the sentence containing "unlabeled
opening fence" (the phrase starting "specifier lines that appear on their own
line before an unlabeled opening fence"); locate the token "unlabeled" in
docs/developers-guide.md and update it to "unlabelled" (and run a quick grep for
other occurrences of "unlabeled" in the repo to ensure consistency).

---

Duplicate comments:
In `@docs/developers-guide.md`:
- Around line 221-224: Rewrite the sentence to use active voice and clearly
explain the predicate: state that if spec_indent starts_with fence_indent then
the specifier’s indentation is considered to extend (or be the same as) the
fence’s indentation and therefore is used; mention that equality is allowed so
the predicate covers both exact matches and deeper (longer) indentations and
reference the symbols spec_indent, fence_indent, and starts_with in the sentence
for clarity.
- Around line 173-175: The sentence "Together these helpers make the rest of the
processing pipeline deal with a single normalized fence form and avoid carrying
separate logic for detached specifier lines." needs a comma after "fence" to
separate the two independent clauses; update the sentence so it reads "...deal
with a single normalized fence, form and avoid..." — actually place the comma
immediately after "fence" in the phrase "single normalized fence form" to
produce "single normalized fence, form" to satisfy the reviewer comment (adjust
surrounding wording if needed to keep grammatical sense while inserting the
comma next to the token "fence").
🪄 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: f2bcbc7f-5bad-4fea-acb9-d40c7dafa88f

📥 Commits

Reviewing files that changed from the base of the PR and between c2e1c1e and 570b912.

📒 Files selected for processing (4)
  • README.md
  • docs/developers-guide.md
  • src/fences.rs
  • tests/fences.rs

Comment thread docs/developers-guide.md Outdated
@leynos leynos changed the title Refactor attach_orphan_specifiers; add attach_to_next_fence helper; update docs Refactor attach_orphan_specifiers; add attach_to_next_fence helper Apr 23, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 170-173: Change the phrasing and punctuation in the developers
guide: in the sentence describing attach_orphan_specifiers replace "their own
line" with "a separate line" to remove redundant ownership wording, and insert a
comma after the introductory adverb "Together" so the sentence reads "Together,
these helpers make the rest of the processing pipeline..." ensuring the doc line
referring to attach_orphan_specifiers uses the new wording and the following
sentence gains the comma.
🪄 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: 5c64c285-917c-4af9-b5c6-086c778622c4

📥 Commits

Reviewing files that changed from the base of the PR and between 570b912 and 4d81868.

📒 Files selected for processing (1)
  • docs/developers-guide.md

Comment thread docs/developers-guide.md Outdated
@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Apr 23, 2026

@coderabbitai Have the following now been resolved?

Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity.

Please address the comments from this code review:

## Overall Comments
- The new implementation still drops blank lines between an orphan specifier and the attached fence (they are collected in `blank_lines` but never emitted when `attached == true`), which contradicts the PR description about preserving these lines—either adjust the logic to emit them or update the description/tests if the previous behavior (dropping them) is indeed intended.

## Individual Comments

### Comment 1
<location path="src/fences.rs" line_range="150" />
<code_context>
 #[must_use]
 pub fn attach_orphan_specifiers(lines: &[String]) -> Vec<String> {
     let mut out = Vec::with_capacity(lines.len());
-    let mut i = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the blank-line scanning and fence-attachment logic into a dedicated helper so the main loop in `attach_orphan_specifiers` just delegates work and no longer needs an `attached` flag.

You can keep the new behavior while reducing complexity by moving the blank‑line scanning and fence matching into a helper and eliminating the `attached` flag from the main loop.

For example:

```rust
fn attach_to_next_fence<'a, I>(
    lines: &mut std::iter::Peekable<I>,
    spec: &str,
    indent: &str,
    out: &mut Vec<String>,
    spec_line: &str,
) -> bool
where
    I: Iterator<Item = &'a String>,
{
    let mut blank_lines = Vec::new();

    while let Some(next_line) = lines.peek() {
        if next_line.trim().is_empty() {
            // consume and collect blanks
            blank_lines.push(lines.next().unwrap().clone());
            continue;
        }

        if let Some(captures) = FENCE_RE.captures(next_line) {
            let lang = captures.get(3).map_or("", |m| m.as_str());
            if is_null_lang(lang) {
                // consume fence and attach specifier
                if let Some(fence_line) = lines.next() {
                    out.push(attach_specifier_to_fence(fence_line, spec, indent));
                    return true;
                }
            }
        }

        break;
    }

    // no attachment: emit original spec line and blanks
    out.push(spec_line.to_string());
    out.extend(blank_lines);
    false
}
```

Then `attach_orphan_specifiers` becomes a thin coordinator:

```rust
pub fn attach_orphan_specifiers(lines: &[String]) -> Vec<String> {
    let mut out = Vec::with_capacity(lines.len());
    let mut lines = lines.iter().peekable();

    while let Some(line) = lines.next() {
        let (spec, indent) = normalize_specifier(line);

        if ORPHAN_LANG_RE.is_match(&spec)
            && out.last().is_none_or(|l: &String| l.trim().is_empty())
        {
            if attach_to_next_fence(&mut lines, &spec, &indent, &mut out, line) {
                continue;
            }
            continue;
        }

        out.push(line.clone());
    }

    out
}
```

This preserves the iterator-based implementation and all new behavior, but:

- The main function only orchestrates: detect orphan specifier → delegate attachment.
- The nested control flow (blank-line collection, fence detection, language inspection) is isolated in one well-named helper.
- The `attached` flag disappears, replaced by the helper’s boolean return.
</issue_to_address>

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Apr 23, 2026

@coderabbitai Have the following now been resolved?

src/fences.rs (1)

1-6: 🧹 Nitpick | 🔵 Trivial
Update module-level doc comment to mention orphan specifier attachment.
The module documentation only describes compress_fences. Add a sentence describing attach_orphan_specifiers and its purpose to give readers a complete picture of the module's functionality.
As per coding guidelines: "Every module must begin with a //! doc comment that explains the module's purpose and utility."

📝 Proposed documentation update
//! Pre-processing utilities for normalizing fenced code block delimiters.
 //!
 //! `compress_fences` reduces any sequence of three or more backticks or
 //! tildes followed by optional language identifiers to exactly three
 //! backticks.
-//! It preserves indentation and the language list.
+//! It preserves indentation and the language list.
+//!
+//! `attach_orphan_specifiers` reattaches language specifiers that appear on
+//! their own line before a fence, preserving any intervening blank lines.
🤖 Prompt for AI Agents

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat src/fences.rs

Length of output: 7730


@leynos Both requirements are fully resolved in the current src/fences.rs.

Overall comment — blank-line handling
attach_to_next_fence collects blank lines into blank_lines but issues a bare return on successful attachment, leaving that buffer unemitted. Blanks are therefore dropped on success, which is the agreed-upon behaviour. The fallback path emits the original specifier line followed by the buffered blanks, so no information is lost when no suitable fence is found. Nothing contradicts the PR description or the tests.

Individual Comment 1 — cyclomatic complexity / attached flag
attach_to_next_fence is already a self-contained private helper. attach_orphan_specifiers is a thin coordinator — it calls normalize_specifier, tests ORPHAN_LANG_RE, delegates to attach_to_next_fence, and continues. There is no attached flag anywhere in the file. The proposed bool return value from the review prompt is redundant: both branches of if attach_to_next_fence(...) { continue; } continue; reduce to the same continue, so the void return in the current implementation is strictly simpler and correct.

No en-GB-oxendict or complexity annulments are required: normalize (with -ize) is the correct Oxford spelling, and the refactor reduces rather than increases complexity.

leynos and others added 11 commits April 27, 2026 03:14
Previously, blank lines between a specifier line and the following opening fence were preserved. This change updates the behavior to drop any intervening blank lines when attaching the specifier to the fence. Corresponding tests were updated and parameterized to cover multiple blank lines scenarios.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Add parameterized tests to verify that orphan specifiers and blank lines remain unchanged when they are not attachable. This ensures the function attach_orphan_specifiers correctly handles these cases without modifying the input.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This update adds comprehensive documentation for the `fences` module
in the developers guide, explaining its purpose, public APIs,
architecture, internal helper semantics, and indentation logic.

The new guide helps maintainers and contributors understand how
fenced code blocks are normalized and processed, facilitating
future improvements and maintenance.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…handling

- Improved explanation of `--fences` option in README.md to detail normalization of fenced code block delimiters, preservation of indentation and language identifiers, and handling of orphan language specifiers.
- Refined developers guide to specify that specifier indentation equal to or extending fence indentation is considered, improving clarity on indentation rules for fenced blocks.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Corrected spelling of 'unlabelled' in code comment to match style.
Improved wording in the explanation of indentation handling for fenced blocks.
These changes improve clarity and consistency in the developers guide documentation.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Refined phrasing for clarity:
- Changed 'on their own line' to 'on a separate line'
- Added a missing comma after 'Together'
These changes enhance the readability of code documentation without altering functionality.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Introduce a new feature to normalize fenced code blocks by reducing delimiter runs to
exactly three backticks while preserving indentation and language identifiers. Implement
logic to attach language specifiers on separate lines to the following unlabelled
opening fence, removing blank lines between when appropriate.

Add documentation in users-guide.md describing the new --fences option and fence normalization behavior.

Include comprehensive tests for the specifier attachment functionality to ensure
correct handling of blank lines and preservation of existing labels.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…metrization

Refactored multiple individual tests for attach_to_next_fence into a single
parameterized test using rstest to improve test clarity and reduce code duplication.
Additionally, updated documentation wording for fence normalization for clarity.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…specifier line

Extended explanation in the users-guide to specify that if no suitable fence follows a language specifier line, that line along with any intervening blank lines are left unchanged, preserving document spacing.

Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Keep the internal fence helper tests behind `#[cfg(test)]` and restore
lost `rstest` and `test` attributes in the integration tests. These
markers are required for the rebased branch to compile with warnings
denied and to keep the orphan-specifier cases covered.
Resolve the post-turn hook failures by making the Makefile fall back to
the standard user install locations for `cargo` and `markdownlint-cli2`
when they are absent from the hook PATH.

Remove extra blank lines in the developer guide so `make markdownlint`
passes after the rebase.
@lodyai lodyai Bot force-pushed the refactor-orphan-specifiers-iterator-h1724j branch from a20e39c to 163379c Compare April 27, 2026 01:15
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== src/fences.rs (full) ==="
cat src/fences.rs

echo ""
echo "=== docs/developers-guide.md lines 160-185 ==="
sed -n '160,185p' docs/developers-guide.md

echo ""
echo "=== docs/users-guide.md ==="
cat docs/users-guide.md

echo ""
echo "=== tests/fences.rs ==="
cat tests/fences.rs

echo ""
echo "=== Cargo.toml (proptest/insta check) ==="
grep -i "proptest\|insta" Cargo.toml 2>/dev/null || echo "(neither found)"

Length of output: 30970


@leynos All three checks are now resolved.


✅ Testing — Resolved

src/fences.rs has a #[cfg(test)] module at the bottom with two rstest-parameterised tests that call attach_to_next_fence directly:

  • attach_to_next_fence_attaches_to_unlabelled_fence — parameterised over ["```"], ["", "```"], ["", "", "```"], covering immediate attachment and attachment after one or more blank lines (verifying blank lines are dropped).
  • attach_to_next_fence_preserves_specifier_when_no_attachment_occurs — covers a non-fence next line (plain text), a labelled fence (not consumed), and an exhausted iterator.

All six required cases are covered. proptest and insta are absent from Cargo.toml, correctly treated as out-of-scope for this PR.


✅ User-Facing Documentation — Resolved

docs/users-guide.md now contains a ## Fence normalisation section that documents:

  1. Delimiter compression — runs of three or more backticks/tildes reduced to three backticks, with indentation and language identifiers preserved.
  2. Orphan specifier attachment — specifier on a separate line attached to the following unlabelled fence; blank lines dropped on successful attachment; specifier and blanks left unchanged when no suitable fence follows.
  3. Two worked before/after examples.

✅ Developer Documentation — Resolved

The grammatical corrections were confirmed in the previous round (full file inspection): line 170 reads "a separate line" and line 173 reads "Together, these helpers…" with the required comma. The current output is consistent with those fixes remaining in place.

leynos added 2 commits April 27, 2026 03:16
Add the blank line required before the CLI matrix harness heading so
`make markdownlint` passes after rebasing onto the latest `origin/main`.
Document that `compress_fences` performs conditional compression and
preserves outer delimiters when shortening them would change inner fence
structure.

Clarify that orphan specifier attachment only runs when the specifier
line starts a block, so specifiers after prose or other content are left
alone.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/fences.rs`:
- Around line 391-433: The inline test module (mod tests) at the bottom of
src/fences.rs pushes the file past the 400-line limit; extract that module into
a new file tests.rs and rewire it as a submodule. Create src/fences/tests.rs
containing the existing test functions
(attach_to_next_fence_attaches_to_unlabelled_fence and
attach_to_next_fence_preserves_specifier_when_no_attachment_occurs) with the
same imports (use super::*; use rstest::rstest;), then in src/fences.rs replace
the inline mod tests { ... } with mod tests; so the tests still reference
FenceTracker and attach_to_next_fence via super. Ensure Cargo test discovery
still runs the moved tests.
🪄 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 Plus

Run ID: 9976d944-8b20-4365-9276-1344acff0017

📥 Commits

Reviewing files that changed from the base of the PR and between a20e39c and b3119d1.

📒 Files selected for processing (6)
  • Makefile
  • README.md
  • docs/developers-guide.md
  • docs/users-guide.md
  • src/fences.rs
  • tests/fences.rs

Comment thread src/fences.rs Outdated
Split orphan specifier attachment into a private helper module with a
pure lookahead step and an explicit `AttachmentOutcome`. This makes the
command-side iterator, output, and tracker mutations visible to callers.

Move direct helper tests into the private `fences` test module and cover
non-empty indentation, mismatched indentation, tracker updates, success,
failure, blank-line handling, and iterator exhaustion.
@lodyai lodyai Bot force-pushed the refactor-orphan-specifiers-iterator-h1724j branch from 00c991d to 8faa394 Compare April 27, 2026 01:30
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 `@src/fences/attachment.rs`:
- Around line 6-10: Add Rustdoc comments to the restricted-public enum
AttachmentOutcome and its variants to document the cross-module API used by
fences.rs and fences/tests.rs: add a top-level /// comment above
AttachmentOutcome describing its role (that it represents the result of an
attachment operation and is visible across modules) and short /// comments for
each variant (Attached — when an item was attached; Preserved — when an existing
attachment was preserved), using /// style Rustdoc so cargo doc can pick it up.

In `@src/fences/tests.rs`:
- Around line 13-17: The tests duplicate initialization of out (Vec<String>) and
tracker (FenceTracker); extract those into rstest fixtures and inject them into
the test functions instead of creating them inline: create fixtures that return
Vec<String> (empty) and FenceTracker::new(), annotate tests (e.g.,
attach_to_next_fence_attaches_to_unlabelled_fence) to accept parameters mut out:
Vec<String> and mut tracker: FenceTracker, remove the local let mut out / let
mut tracker lines, and apply the same fixture-based change to the other tests
that repeat the same setup (the tests around the other similar cases).
🪄 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 Plus

Run ID: 6ca47807-3347-4cb4-a33a-fe863bd9ded4

📥 Commits

Reviewing files that changed from the base of the PR and between b3119d1 and 00c991d.

📒 Files selected for processing (3)
  • src/fences.rs
  • src/fences/attachment.rs
  • src/fences/tests.rs

Comment thread src/fences/attachment.rs
Comment thread src/fences/tests.rs Outdated
@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Apr 27, 2026

@coderabbitai pause

Add Rustdoc for the cross-module AttachmentOutcome enum and move
repeated fence helper test setup into rstest fixtures.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

✅ Actions performed

Reviews paused.

@leynos

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

Delete the private `attach_to_next_fence` test module so fence behaviour is
exercised through the public `attach_orphan_specifiers` boundary.
@coderabbitai

This comment was marked as resolved.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== src/fences/attachment.rs ==="
cat src/fences/attachment.rs 2>/dev/null || echo "(not found)"

echo ""
echo "=== src/fences/tests.rs ==="
cat src/fences/tests.rs 2>/dev/null || echo "(not found)"

echo ""
echo "=== src/fences.rs ==="
cat src/fences.rs 2>/dev/null || echo "(not found)"

echo ""
echo "=== tests/fences.rs ==="
cat tests/fences.rs 2>/dev/null || echo "(not found)"

echo ""
echo "=== docs/users-guide.md ==="
cat docs/users-guide.md 2>/dev/null || echo "(not found)"

echo ""
echo "=== docs/developers-guide.md ==="
cat docs/developers-guide.md 2>/dev/null || echo "(not found)"

echo ""
echo "=== README.md (fences section) ==="
grep -n -i -A 15 "fences\|orphan\|specifier" README.md | head -80

echo ""
echo "=== Makefile ==="
cat Makefile 2>/dev/null || echo "(not found)"

echo ""
echo "=== docs/execplans (listing) ==="
ls -1 docs/execplans/ 2>/dev/null || echo "(not found)"

echo ""
echo "=== ROADMAP / execplan search ==="
fd -i "roadmap\|execplan\|plan" --type f | grep -v "node_modules\|.git\|target" | head -20

Length of output: 47040


@leynos Here is the per-check verdict after inspecting all files.


✅ Unit Architecture — Resolved

next_attachable_fence operates on a cloned Peekable<I> taken by value, returns NextFence::Attachable { blank_count } or NextFence::NotAttachable { blank_count }, and has no side-effects. attach_to_next_fence calls next_attachable_fence(lines.clone()) for the query, then matches the result to consume the original iterator, push to out, and advance tracker. The CQS split is fully in place.


✅ Testing (Unit) — Resolved

src/fences/tests.rs no longer exists, and no #[cfg(test)] mod tests; declaration appears in src/fences.rs. tests/fences.rs covers the required behaviours at the public attach_orphan_specifiers boundary: immediate attachment, attachment after one blank line, attachment after multiple blank lines, non-attachable next line (plain text and labelled fence).


✅ Testing (Overall) — Resolved

fixes_orphaned_specifier_with_blank_lines asserts out == lines_vec!["```rust", "fn main() {}", "```"] — blank lines absent on successful attachment. leaves_orphan_specifier_and_blank_lines_unchanged_when_not_attachable asserts out == input — blank lines present when attachment fails. Both assertions are unambiguous.


✅ User-Facing Documentation — Resolved

docs/users-guide.md has a ## Fence normalization section covering: delimiter compression and structural safety, orphan specifier attachment, blank-line dropping on success, preservation when no suitable fence follows, and two worked before/after examples. README.md lines 64–69 are consistent.


❌ Developer Documentation — Not fully resolved

The ## Fences module section in docs/developers-guide.md is complete. The execplans are all marked COMPLETE. However, the Makefile change — CARGO and MDLINT now resolved via command -v with explicit fallback paths — remains undocumented in the guide (this was flagged in the previous round; an agent prompt was provided but the change was not made).

## Task: document Makefile tooling variable resolution in docs/developers-guide.md

### Background
The Makefile now resolves CARGO and MDLINT dynamically:

  CARGO  ?= $(or $(shell command -v cargo 2>/dev/null),$(HOME)/.cargo/bin/cargo)
  MDLINT ?= $(or $(shell command -v markdownlint-cli2 2>/dev/null),$(HOME)/.bun/bin/markdownlint-cli2)

This is not documented anywhere in the developer guide.

### Action
Append a `## Build tooling` section to `docs/developers-guide.md` (after the
existing `## CLI matrix harness` section) with the following content:

- `CARGO` defaults to the first `cargo` found on `PATH` via `command -v`, with
  an explicit fallback to `~/.cargo/bin/cargo`. Override by setting `CARGO` in
  the environment.
- `MDLINT` defaults to the first `markdownlint-cli2` found on `PATH`, with an
  explicit fallback to `~/.bun/bin/markdownlint-cli2`. Override by setting
  `MDLINT` in the environment.
- Both variables use the Make `$(or …,…)` pattern so they resolve at make-time
  rather than at shell-invocation time.

Keep the prose consistent with the existing guide style. Do not alter any
existing content.

✅ Module-Level Documentation — Resolved

src/fences/attachment.rs opens with //! Orphan fence specifier attachment helpers.. AttachmentOutcome and both variants (Attached, Preserved) have doc comments. attach_to_next_fence has a doc comment describing the query/command split. attach_specifier_to_fence has a doc comment with a # Examples block.


✅ Testing (Compile-Time / UI) — Not applicable

The PR introduces no UI or compile-time test artefacts. The existing insta snapshot harness in tests/cli_matrix.rs covers --fences end-to-end; no additional snapshot or UI tests are required.

@leynos leynos merged commit 6efb824 into main Apr 27, 2026
3 checks passed
@leynos leynos deleted the refactor-orphan-specifiers-iterator-h1724j branch April 27, 2026 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor attach_orphan_specifiers to use state machine with Peekable iterator

1 participant