Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughReturn Utf8PathBuf inside io::Result from ensure_manifest_exists; switch inputs to camino Utf8Path types; validate file name and directory state; create parent directories and persist a NamedTempFile robustly; update callers to convert and unwrap the Result. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestRunner as Test Runner
participant Run as run()
participant Helper as ensure_manifest_exists()
participant FS as Filesystem
TestRunner->>Run: start
Run->>Helper: ensure_manifest_exists(temp_dir: Utf8Path, cli_file: Utf8Path)
alt cli_file is relative
Note right of Helper #DDEBF7: Resolve manifest_path = temp_dir.join(cli_file)
end
Helper->>Helper: validate file_name -> Err(InvalidInput) if missing
Helper->>FS: ensure_parent_directory(parent) --map_err-->
Helper->>FS: NamedTempFile::new_in(parent) --map_err-->
Helper->>FS: write_all(content) --map_err-->
Helper->>FS: persist(tempfile -> manifest_path) --map_err-->
alt any I/O fails
Helper-->>Run: Err(io::Error) (propagated)
else success
Helper-->>Run: Ok(Utf8PathBuf manifest_path)
Run->>Run: convert to std::path::PathBuf and assign
end
Run-->>TestRunner: continue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Comment |
Reviewer's GuideRefactor ensure_manifest_exists into a fallible API by returning io::Result and mapping filesystem errors to include manifest path context. Extract helper functions for directory creation, temp file handling, writing, and persisting with contextual error messages. Update documentation, examples, and tests to handle the new error propagation behavior, and bump dependencies to support UTF-8 path handling. Entity relationship diagram for manifest file creation and error contexterDiagram
MANIFEST_FILE {
Utf8PathBuf path
String content
}
ERROR_CONTEXT {
String error_message
Utf8PathBuf manifest_path
}
MANIFEST_FILE ||--o{ ERROR_CONTEXT : "error mapped to manifest path context"
Class diagram for process_steps test changesclassDiagram
class ProcessStepsTest {
+test_manifest_creation()
+test_manifest_error_handling()
}
ProcessStepsTest : <<updated>>
ProcessStepsTest : Now handles Result from ensure_manifest_exists
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_support/src/lib.rs (1)
133-184: Add targeted tests for new failure paths.Assert the enriched diagnostics by forcing failures (e.g., unwritable parent dir, directory-as-file path).
I can add rstest cases that:
- pass a trailing-separator path (directory) and assert
InvalidInput.- point
dest_dirto a read-only temp dir and assert the mapped error text contains the target path.
Do you want me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test_support/src/lib.rs(4 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions when conditional criteria have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer expect over allow for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
tests/steps/process_steps.rstest_support/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/steps/process_steps.rstest_support/src/lib.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection using mockable (traits like Env and Clock) where appropriate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/process_steps.rs
🧬 Code graph analysis (2)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
ensure_manifest_exists(133-184)
test_support/src/lib.rs (1)
test_support/src/env.rs (1)
write_manifest(167-179)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (stable)
🔇 Additional comments (7)
tests/steps/process_steps.rs (1)
85-85: Make manifest failure messages actionable.Include the error chain in the panic to speed up triage.
[ suggest_recommended_refactor ]
- cli.file = ensure_manifest_exists(dir.path(), &cli.file).expect("manifest"); + cli.file = ensure_manifest_exists(dir.path(), &cli.file) + .unwrap_or_else(|e| panic!("manifest: {}", test_support::display_error_chain(&e)));test_support/src/lib.rs (6)
30-30: Imports update — LGTM.Bring
iointo scope withWriteto support fallible I/O.
115-131: Docs: switch to Errors section with updated example — LGTM.Document failure modes and show
.expect("manifest")at call site.
153-161: Temp file creation with context — LGTM.Map
NamedTempFile::new_infailures with path context.
162-170: Manifest write mapping — LGTM.Propagate
write_manifesterrors with clear diagnostics.
183-184: ReturnOk(manifest_path)— LGTM.Finish with an explicit
Okto match the new signature.
133-139: Approve — call sites handle ResultMark resolved. Both usages consume the Result via .expect(): tests/steps/process_steps.rs:85 and doc example in test_support/src/lib.rs (lines 129–130).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test_support/src/lib.rs (1)
141-149: Early reject of directory-like targets (no file name) is good.This prevents persisting to paths without a terminal file component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test_support/src/lib.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
test_support/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
test_support/src/lib.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from a library; convert to domain error enums at API boundaries
Files:
test_support/src/lib.rs
🧬 Code graph analysis (1)
test_support/src/lib.rs (1)
test_support/src/env.rs (1)
write_manifest(167-179)
🔇 Additional comments (5)
test_support/src/lib.rs (5)
30-30: ImportioandWriteis appropriate for fallible I/O.Proceed; this enables
io::Resultand trait bounds used below.
161-169: Temp-file creation error mapping is clear.Message includes the target context; keep as-is.
170-178: Write step correctly propagates errors with path context.LGTM; the message is specific and stable for tests.
192-193: ReturningOk(manifest_path)is correct and stable.This preserves absolute semantics for relative inputs joined to
temp_dir.
129-131: Approve and merge — call-sites handle the new ResultFound a single usage at tests/steps/process_steps.rs:85 which already calls .expect("manifest"); no other usages found.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: test_support/src/lib.rs ❌ New issue: Bumpy Road Ahead Suppress |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…fest helper Rebased onto origin/main. Merged helper refactor with Result-based errors and capability FS ops. Cleaned conflict artifacts and ensured Utf8Path usage across tests. Ran make check-fmt, make test, and make lint: all passing.
leynos
force-pushed
the
codex/refactor-write_manifest-to-return-result-type
branch
from
36620a6 to
eca6741
Compare
|
@sourcery-ai review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test_support/src/lib.rs:281-290` </location>
<code_context>
+ use std::io;
+ use tempfile::TempDir;
+
+ #[test]
+ fn existing_directory_manifest_path_is_rejected() {
+ let temp = TempDir::new().expect("temp dir");
+ let temp_path = Utf8Path::from_path(temp.path()).expect("utf-8 path");
+ let dir = temp.path().join("dir");
+ fs::create_dir(&dir).expect("create dir");
+
+ let err = ensure_manifest_exists(temp_path, Utf8Path::new("dir"))
+ .expect_err("existing directory");
+ assert_eq!(err.kind(), io::ErrorKind::NotADirectory);
+ let msg = err.to_string();
+ assert!(msg.contains(dir.to_str().expect("utf-8")), "message: {msg}");
+ }
+
+ #[test]
+ fn read_only_parent_reports_target_path() {
+ let temp = TempDir::new().expect("temp dir");
+ let temp_path = Utf8Path::from_path(temp.path()).expect("utf-8 path");
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add a test for manifest creation when the parent directory does not exist.
Add a test case for manifest creation when the parent directory is missing, ensuring correct error handling and messaging for directory creation failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)docs/behavioural-testing-in-rust-with-cucumber.md(1 hunks)docs/reliable-testing-in-rust-via-dependency-injection.md(3 hunks)docs/rust-doctest-dry-guide.md(2 hunks)test_support/Cargo.toml(1 hunks)test_support/src/lib.rs(2 hunks)tests/steps/process_steps.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Use explicit caret SemVer requirements for all dependencies
Disallow wildcard (*) and open-ended (>=) requirements; use ~ only with a documented reason
Files:
Cargo.tomltest_support/Cargo.toml
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
In fenced code blocks for docs, explicitly mark code fences with rust (```rust) for clarity
Files:
docs/reliable-testing-in-rust-via-dependency-injection.mddocs/behavioural-testing-in-rust-with-cucumber.mddocs/rust-doctest-dry-guide.md
docs/reliable-testing-in-rust-via-dependency-injection.md
📄 CodeRabbit inference engine (AGENTS.md)
Follow the project’s guide for reliable testing in Rust via dependency injection
Files:
docs/reliable-testing-in-rust-via-dependency-injection.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
Files:
docs/reliable-testing-in-rust-via-dependency-injection.mddocs/behavioural-testing-in-rust-with-cucumber.mddocs/rust-doctest-dry-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/reliable-testing-in-rust-via-dependency-injection.mddocs/behavioural-testing-in-rust-with-cucumber.mddocs/rust-doctest-dry-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/reliable-testing-in-rust-via-dependency-injection.mddocs/behavioural-testing-in-rust-with-cucumber.mddocs/rust-doctest-dry-guide.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
test_support/src/lib.rstests/steps/process_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
test_support/src/lib.rstests/steps/process_steps.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from a library; convert to domain error enums at API boundaries
Files:
test_support/src/lib.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Large blocks of test data should be moved to external data files
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Mock non-deterministic dependencies (Env, Clock) via dependency injection using mockable crate
Test documentation should omit examples that merely reiterate test logic
Files:
tests/steps/process_steps.rs
🧬 Code graph analysis (2)
test_support/src/lib.rs (1)
test_support/src/env.rs (1)
write_manifest(167-179)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
ensure_manifest_exists(137-156)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (8)
docs/reliable-testing-in-rust-via-dependency-injection.md (1)
25-25: LGTM! Formatting change follows guidelines.The separator update to underscores improves consistency.
docs/rust-doctest-dry-guide.md (1)
231-232: LGTM! Formatting changes are consistent.The split lines improve readability whilst maintaining correct markdown structure.
tests/steps/process_steps.rs (1)
86-92: LGTM! Proper Utf8Path conversion with clear error messages.The conversion to Utf8Path types aligns with the updated API and includes appropriate error messages for debugging.
test_support/src/lib.rs (5)
137-156: LGTM! Clean API refactoring with proper error handling.The function now correctly:
- Uses Utf8Path types throughout
- Validates directory targets early
- Returns descriptive errors instead of panicking
- Handles the happy path efficiently
This aligns well with the PR objectives to propagate filesystem errors properly.
158-173: LGTM! Well-structured path resolution.The helper correctly validates that the path includes a filename and returns appropriate errors.
175-181: Extract complexity to meet cyclomatic complexity guidelines.The
create_manifest_fileorchestrates multiple operations but remains clear. However, the PR comments flagged nested complexity in the overall flow. This decomposition helps address those concerns.
204-218: LGTM! Robust persist handling with race condition mitigation.The function correctly handles
AlreadyExistsas a success case, preventing race-induced failures in concurrent test environments.
273-312: LGTM! Comprehensive test coverage.The tests properly verify:
- Directory rejection with correct error kind
- Error messages containing the target path
- Read-only parent handling
Good defensive testing for edge cases.
✅ Actions performedReviews paused. |
Add a test that verifies ensure_manifest_exists creates a manifest and its parent directories when the target path’s parent does not exist. Also checks that minimal manifest content is written (contains ).
When the resolved manifest parent path exists but is not a directory, return an AlreadyExists error that explicitly mentions the target manifest path. This makes the failure clearer and matches test expectations when a file occupies the would-be parent directory. Rationale: - Prevents later, confusing I/O errors at temp-file creation time. - Produces a precise, actionable error for callers.
- Use `netsuke::…` paths in ninja_gen doctests so they resolve when built as part of the workspace. - Expose `runner::doc` helpers unconditionally (still doc-hidden) so doctests can import them without cfg(doctest) gating. - Update stdlib doctest to use `add_template`/`get_template` API and a deterministic current working directory path. - Reword long markdown line in doctest guide to satisfy MD013. These changes make all doctests compile and pass under `cargo test --doc`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Correct inline code spans, punctuation, and sentence spacing around file names (, ). Improves readability and complies with markdown lint rules.
Refactor ensure_parent_directory by extracting the logic that finds the first existing ancestor into find_existing_ancestor. This reduces nesting and clarifies intent, while preserving prior behaviour (including the non-directory parent error case).
leynos
deleted the
codex/refactor-write_manifest-to-return-result-type
branch
1 participant
Summary
io::Resultfromensure_manifest_existsTesting
make fmtmake lintmake testcloses #62
https://chatgpt.com/codex/tasks/task_e_68c615a741a08322937728e45293fdd6
Summary by Sourcery
Implement error propagation in manifest creation by converting ensure_manifest_exists to return io::Result, refactor supporting helpers to use UTF-8 paths with enriched context errors, update tests and process step usage accordingly, adjust documentation formatting, and add necessary dependencies.
New Features:
Enhancements:
Build:
Documentation:
Tests: