Tighten break Cow tests and test helpers - #81
Merged
Conversation
Contributor
Reviewer's GuideThis PR refactors the format_breaks function to return Cow instead of String, reducing unnecessary cloning of unmodified lines, and updates all relevant callers and tests to accommodate the new return type while simplifying test data creation. Class diagram for refactored format_breaks function and usageclassDiagram
class format_breaks {
+format_breaks(lines: &[String]) -> Vec<Cow<'_, str>>
}
class Cow {
<<enum>>
+Borrowed(&'a str)
+Owned(String)
}
class process_lines {
+process_lines(lines: &[String], opts: FormatOpts) -> Vec<String>
}
format_breaks --> Cow : returns Vec<Cow<'_, str>>
process_lines --> format_breaks : calls
process_lines --> Cow : converts Cow to String
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
OverviewThis PR refactors Key ChangesCore Functionality
Test Infrastructure
Proc-Macro Support
Documentation
Lint Management
Testing & Verification
WalkthroughRefactor format_breaks to return Vec<Cow<'_, str>> so unchanged lines and synthetic thematic breaks can be borrowed. Update unit/property and integration tests to assert borrowing semantics. Add a test-macros proc-macro for fixture lint suppression, extract and export test helpers into tests/support, rewire tests to use them, and update docs and dev-dependencies. Changesformat_breaks Cow allocation and test infrastructure
Sequence Diagram(s)(Changes are library return-type updates, test infra extraction, proc-macro addition and docs updates; no new multi-component runtime sequence diagram required.) Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 15❌ Failed checks (15 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
lodyai
Bot
force-pushed
the
codex/refactor-format_breaks-to-use-cowstr
branch
from
May 25, 2026 19:33
bfaaac2 to
547d7b5
Compare
Owner
Author
|
@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. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
out.push(Cow::Borrowed(THEMATIC_BREAK_LINE.as_str()));
|
leynos
changed the title
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 errors, 7 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rust-testing-with-rstest-fixtures.md`:
- Around line 1158-1161: The paragraph describing the utility macro exceeds 80
columns; reflow it to wrap at 80 characters per line so it fits the
documentation style. Edit the paragraph that mentions the lines_vec! macro and
split the long sentence into multiple lines (or into two sentences) so no line
exceeds 80 columns, preserving the text that explains using lines_vec! in
fixtures to avoid repetitive .to_string() calls.
In `@tests/common/mod.rs`:
- Line 40: Replace the forbidden #[allow(dead_code, reason = "helper used
selectively across modules")] attribute with the corresponding expectation
attribute: change the attribute to #[expect(dead_code, reason = "helper used
selectively across modules")] on the same item (the helper in
tests/common/mod.rs), matching the same fix applied at the other occurrence
referenced near line 10 so the lint now records an expectation instead of
allowing the lint.
- Line 10: Replace the forbidden top-level attribute #[allow(unused_macros,
reason = "...")] with the narrowly scoped lint expectation attribute
#[expect(unused_macros, reason = "...")] in tests/common/mod.rs; locate the
attribute on the module (the current #[allow(...)] line) and change the
attribute name to expect while keeping the same lint and reason text so it
complies with the guideline that #[allow] is not used and only #[expect(lint,
reason = "...")] is permitted.
- Line 118: Replace the forbidden attribute #[allow(dead_code, reason = "used
selectively across integration tests")] with the approved #[expect(...)] form;
locate the attribute instance (the #[allow(dead_code, reason = "used selectively
across integration tests")] annotation in tests/common/mod.rs — and change it to
#[expect(dead_code, reason = "used selectively across integration tests")] so
the intent is preserved while complying with the guideline (also update the
identical instance referenced on line 10).
- Line 106: Replace the forbidden attribute #[allow(dead_code, reason = "helper
used selectively across modules")] with the approved #[expect(dead_code, reason
= "helper used selectively across modules")] attribute; locate the module-level
attribute in tests' helper module (the exact attribute instance shown) and
change "allow" to "expect" while preserving the dead_code lint and the reason
string.
- Line 23: Replace the forbidden attribute #[allow(unused_macros, reason =
"macros are optional helpers across modules")] with the approved #[expect(...)]
variant: locate the attribute in tests::common::mod.rs (module common) and
change the attribute name from allow to expect while preserving the same lint
name unused_macros and the reason string so it becomes #[expect(unused_macros,
reason = "macros are optional helpers across modules")].
- Line 129: Replace the forbidden attribute usage: change the module-level
attribute #[allow(dead_code, reason = "used selectively across integration
tests")] to the approved #[expect(dead_code, reason = "used selectively across
integration tests")] so the symbol at the top of tests/common/mod.rs uses
#[expect] instead of #[allow]; update any other identical attributes (see the
similar occurrence referenced at line 10) to the same #[expect(...)] form to
comply with the coding guidelines.
In `@tests/prelude/mod.rs`:
- Line 13: Replace the forbidden attribute #[allow(unused_imports, reason =
"re-exporting common test utilities")] with the expected attribute
#[expect(unused_imports, reason = "re-exporting common test utilities")] so the
module-level attribute uses #[expect(...)] instead of #[allow(...)]; update the
single attribute instance shown (the #[allow(...)] on the tests prelude module)
accordingly.
- Line 5: Replace the forbidden #[allow(...)] attribute with #[expect(...)] on
the unused_imports attribute; specifically change #[allow(unused_imports, reason
= "re-exporting common test utilities")] to #[expect(unused_imports, reason =
"re-exporting common test utilities")] (same pattern as the similar attribute on
line 3) so the lint uses an expectation instead of an allow.
- Line 7: Replace the forbidden attribute on the tests/prelude/mod.rs module:
change the #[allow(unused_imports, reason = "re-exporting common test
utilities")] attribute to the approved #[expect(..., reason = "...")] form
(i.e., use #[expect(unused_imports, reason = "re-exporting common test
utilities")]) so the unused_imports warning is marked with expect rather than
allow; update the attribute on the module where the current #[allow(...)]
appears (same pattern used on line 3).
- Line 3: Replace the forbidden attribute usage #[allow(unused_imports, reason =
"re-exporting common test utilities")] in tests/prelude/mod.rs with a narrowly
scoped expect attribute: use #[expect(unused_imports, reason = "...")] (or the
appropriate lint name) keeping the same reason text ("re-exporting common test
utilities") so the intent is preserved; update the attribute directly above the
re-exporting imports or module to reference the correct lint symbol
(unused_imports) and remove the #[allow(...)] entry entirely.
🪄 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: 7f507bec-a080-4a68-a003-8c0709c8a39b
📒 Files selected for processing (5)
docs/rust-testing-with-rstest-fixtures.mdsrc/breaks.rstests/breaks.rstests/common/mod.rstests/prelude/mod.rs
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (3 errors, 6 warnings)
|
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== tests/common/mod.rs ==="
cat tests/common/mod.rs
echo ""
echo "=== tests/prelude/mod.rs ==="
cat tests/prelude/mod.rs
echo ""
echo "=== CHANGELOG.md (first 80 lines) ==="
head -80 CHANGELOG.md 2>/dev/null || echo "CHANGELOG.md not found"
echo ""
echo "=== docs/developers-guide.md (last 60 lines) ==="
tail -60 docs/developers-guide.md 2>/dev/null || echo "not found"
echo ""
echo "=== Check for proptest in Cargo.toml ==="
grep -n "proptest" Cargo.toml 2>/dev/null || echo "proptest not found in Cargo.toml"
echo ""
echo "=== src/breaks.rs ==="
cat src/breaks.rs
echo ""
echo "=== tests/breaks.rs ==="
cat tests/breaks.rs
echo ""
echo "=== Count #[allow] vs #[expect] in test files ==="
echo "--- #[allow] in tests/ ---"
rg -n '#\[allow' tests/
echo "--- #[expect] in tests/ ---"
rg -n '#\[expect' tests/
echo ""
echo "=== docs/users-guide.md (last 40 lines) ==="
tail -40 docs/users-guide.md 2>/dev/null || echo "not found"Length of output: 15306
No open items require a GitHub issue. The PR is ready for re-review. ✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Align the `format_breaks` documentation with the implementation so the example shows thematic break output borrowed from the static break line instead of constructing an owned `String`.
Assert the `Cow` variants returned by `format_breaks` so tests cover the borrowed-output optimisation directly instead of relying on `PartialEq`. Remove the redundant `string_vec!` helper and replace broad lint-integrity suppression with scoped helper-item allowances.
lodyai
Bot
force-pushed
the
codex/refactor-format_breaks-to-use-cowstr
branch
4 times, most recently
from
May 30, 2026 12:31
798d227 to
a913375
Compare
Change the `format_breaks` rustdoc wording from American spelling to British spelling so the public API documentation follows the requested project style.
Explain why the `allow_fixture_expansion_lints` helper exists: `rstest` fixture expansion interacts with single-line function formatting and the compiler's `unused_braces` lint. Keep the guidance scoped to fixture modules so broad lint suppressions stay out of the tests.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Emit the fixture expansion lint annotation as a narrowly scoped `#[allow(unused_braces)]` and document why `#[expect]` is not usable for this proc-macro expansion site. Add the missing property-test assertion message for fenced thematic-break borrowing so failures identify the static-versus-input borrowing contract.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Place the fixture lint macro outside the `rstest` fixture attribute so it wraps the generated fixture code. Add the same positive expected-line guard to the wrapped blockquote helper that list-item assertions already use, and normalize Oxford spelling in the breaks documentation.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Add user-facing guidance for the `format_breaks` `Cow` return type and how callers can convert values into owned strings. Document the exported integration-test macros and why they use `#[macro_export]` rather than unused-macro suppressions.
Add a threaded test for `format_breaks` so concurrent thematic-break formatting verifies the shared `LazyLock` buffer is borrowed consistently and remains pointer-stable across threads.
Owner
Author
|
@coderabbitai resume |
Owner
Author
|
@coderabbitai review |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add a caption for the exported test macro table and switch the `format_breaks` ownership example to a fenced Rust block with a narrow Markdown lint exception for the local users-guide style.
Owner
Author
|
@coderabbitai Have the following now been resolved? src/breaks.rs (1)
🔧 Proposed fix-/// Consecutive hyphens, asterisks or underscores are replaced with a
-/// standardised line of underscores. Fenced code blocks are ignored so
+/// Consecutive hyphens, asterisks or underscores are replaced with a
+/// standardized line of underscores. Fenced code blocks are ignored so🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== docs/users-guide.md (full) ==="
cat docs/users-guide.md 2>/dev/null || echo "not found"
echo ""
echo "=== docs/developers-guide.md (full) ==="
cat docs/developers-guide.md 2>/dev/null || echo "not found"
echo ""
echo "=== tests/support/fixtures.rs ==="
cat tests/support/fixtures.rs 2>/dev/null || echo "not found"
echo ""
echo "=== tests/support/wrap_assertions.rs ==="
cat tests/support/wrap_assertions.rs 2>/dev/null || echo "not found"
echo ""
echo "=== Search for concurrent/thread/LazyLock tests in src/breaks.rs ==="
grep -n "spawn\|thread\|concurrent\|LazyLock\|ptr::eq" src/breaks.rs 2>/dev/null || echo "none found"
echo ""
echo "=== Full src/breaks.rs tests block ==="
sed -n '/#\[cfg(test)\]/,$ p' src/breaks.rs 2>/dev/null || echo "not found"
echo ""
echo "=== Search for format_breaks or Cow in docs/users-guide.md ==="
grep -n "format_breaks\|Cow\|into_owned\|Library API" docs/users-guide.md 2>/dev/null || echo "none found"
echo ""
echo "=== Search for lines_vec or include_lines in docs/developers-guide.md ==="
grep -n "lines_vec\|include_lines\|macro_export\|Exported\|exported" docs/developers-guide.md 2>/dev/null || echo "none found"
echo ""
echo "=== Check if issue `#342` exists ==="
gh issue view 342 --repo leynos/mdtablefix --json number,title,state 2>/dev/null || echo "issue not found"Length of output: 57027
No open items remain. The PR is ready for re-review. ✅ Actions performedReview triggered.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== src/breaks.rs lines 18-30 ==="
sed -n '18,30p' src/breaks.rs
echo ""
echo "=== Search for standardis/standardiz in src/breaks.rs ==="
rg -n "standardis\|standardiz" src/breaks.rs || echo "none found"Length of output: 742
The en-GB-oxendict (-ize) form "standardized" is already in place. No further action is needed. |
Add the missing caption for the integration-test support module table so the developer guide follows the table documentation style used elsewhere.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cowvariants returned byformat_breaksso tests cover the borrowed-output optimisation directlystring_vec!helper and keep test helper lint suppressions item-scopedTesting
make check-fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_687837d741488322b8f005ad81122c27