Conversation
Reviewer's GuideThis PR introduces a dedicated manifest module with YAML parsing and file-loading helpers wrapped in anyhow for robust error handling, refactors existing tests and Cucumber steps to leverage these helpers, adds new fixtures and scenarios to cover valid/invalid manifests, and updates documentation and configuration to reflect the new loader design. Entity relationship diagram for manifest YAML parsingerDiagram
NETSUKE_MANIFEST ||--o{ TARGETS : contains
NETSUKE_MANIFEST {
string netsuke_version
Target[] targets
}
TARGETS {
string name
Recipe recipe
bool phony
bool always
}
TARGETS }o--|| RECIPE : has
RECIPE {
string kind
string command
}
Flow diagram for manifest loading from file or stringflowchart TD
A[User or Test] -->|YAML string| B[from_str]
A -->|File path| C[from_path]
C --> D[Read file contents]
D --> B
B --> E[NetsukeManifest]
B -->|Error| F[anyhow::Error]
C -->|Error| F
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate the manifest parsing infrastructure by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Manifest as manifest module
participant FS as Filesystem
participant Serde as serde_yml
participant AST as NetsukeManifest
Test->>Manifest: from_path(path)
Manifest->>FS: Read file
FS-->>Manifest: File contents (YAML)
Manifest->>Manifest: from_str(yaml)
Manifest->>Serde: Deserialize YAML
Serde-->>Manifest: NetsukeManifest or error
Manifest-->>Test: Result<NetsukeManifest, anyhow::Error>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (3)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
docs/**/*.md📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
**/*.md📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🪛 YAMLlint (1.37.1)tests/data/missing_recipe.yml[error] 4-4: too many blank lines (1 > 0) (empty-lines) tests/data/rules.yml[error] 13-13: too many blank lines (1 > 0) (empty-lines) 🔇 Additional comments (9)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
tests/data/missing_recipe.yml (1)
1-4: Terminate file with a newline.End the fixture with a trailing newline to comply with POSIX text-file conventions and avoid diff-noise in future edits.
- - name: hi + - name: hi +tests/data/rules.yml (1)
1-13: Add trailing newline for consistency.Mirror the convention used by other fixture files.
- rule: compile - + rule: compile +src/lib.rs (1)
1-5: Update crate-level doc comment to reflect new public surface.The comment still claims the library "only exposes the command line interface". With
manifestnow re-exported this statement is stale. Amend to mention manifest helpers to keep docs trustworthy.docs/rust-doctest-dry-guide.md (1)
648-658: Remove dangling comma after “accessed on July 15, 2025”.The comma is unnecessary and breaks the flow of the footnote entry. Drop it to match preceding citations.
- accessed on July 15, 2025, + accessed on July 15, 2025
📜 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 (13)
Cargo.toml(1 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/rust-doctest-dry-guide.md(2 hunks)src/lib.rs(1 hunks)src/manifest.rs(1 hunks)tests/ast_tests.rs(16 hunks)tests/data/invalid_version.yml(1 hunks)tests/data/missing_recipe.yml(1 hunks)tests/data/rules.yml(1 hunks)tests/data/unknown_field.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/steps/manifest_steps.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/rust-doctest-dry-guide.mddocs/roadmap.mddocs/netsuke-design.md
**/*.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.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.
Files:
src/lib.rstests/steps/manifest_steps.rssrc/manifest.rstests/ast_tests.rs
🧬 Code Graph Analysis (1)
tests/steps/manifest_steps.rs (2)
tests/ast_tests.rs (1)
parse_manifest(8-10)src/manifest.rs (1)
from_path(44-49)
🔇 Additional comments (20)
Cargo.toml (1)
11-11: Remove the suggested feature override for anyhow. The crate’s default features include only std and backtrace is opt-in, so disabling defaults and re-enabling std is redundant.Likely an incorrect or invalid review comment.
src/lib.rs (1)
6-9: Confirm module-level doc comment in src/manifest.rsLeading
//!header verified in the first 5 lines ofsrc/manifest.rs. No action required.tests/data/invalid_version.yml (1)
1-7: LGTM - Valid test fixture for invalid version scenario.The fixture correctly demonstrates an invalid
netsuke_versionformat ("1" instead of semantic versioning like "1.0.0") for testing parser validation logic.tests/data/unknown_field.yml (1)
1-8: LGTM - Valid test fixture for unknown field scenario.The fixture correctly includes an
extrafield that should trigger serde'sdeny_unknown_fieldsvalidation during parsing.docs/roadmap.md (1)
27-28: LGTM - Roadmap updates accurately reflect completed tasks.The marked items correctly correspond to the implemented manifest parsing functionality with semver validation and YAML deserialisation logic.
Also applies to: 35-36
tests/features/manifest.feature (1)
21-36: LGTM - Comprehensive test coverage for manifest parsing scenarios.The new scenarios effectively cover both positive (rules parsing) and negative (unknown fields, invalid versions, missing recipes) test cases, providing thorough validation of the manifest parsing functionality.
tests/steps/manifest_steps.rs (3)
5-5: LGTM - Import updated to include manifest module.The import correctly adds the
manifestmodule to support the refactored parsing logic.
13-13: LGTM - Refactored to use centralised manifest parsing.The change from manual file reading and YAML deserialisation to
manifest::from_pathimproves code maintainability and ensures consistent error handling across the codebase.
75-84: LGTM - New test step supports expanded feature scenarios.The
first_rule_namestep function correctly implements the assertion logic for verifying rule names in parsed manifests, supporting the new BDD scenarios.src/manifest.rs (3)
1-6: Module documentation follows coding guidelines correctly.The module begins with
//!doc comments as required and clearly explains the module's purpose and utility. The documentation is concise and follows en-GB spelling conventions.
7-9: Import organisation is clean and appropriate.The imports are minimal and focused, bringing in only the necessary dependencies for the module's functionality.
30-49: File loading function is well-implemented with proper error context.The
from_pathfunction correctly handles file I/O errors with meaningful context usingwith_context. The path handling usingAsRef<Path>is idiomatic and flexible. The function properly chains the file reading with the YAML parsing throughfrom_str.docs/netsuke-design.md (2)
596-600: Documentation accurately reflects the new manifest parsing functionality.The text correctly describes the convenience functions in
src/manifest.rsand their return type (anyhow::Result). The description of "straightforward error handling" aligns with the implementation. The British spelling of "artefacts" is correct per the coding guidelines.
607-610: Testing section update is comprehensive and accurate.The documentation correctly describes the new test fixtures under
tests/data/and their usage in the test suite. The mention of both valid and invalid permutations aligns with the actual test files, and the description of using the new parsing API instead of directserde_ymlcalls is accurate.tests/ast_tests.rs (6)
3-3: Import update is correct and minimal.The addition of the
manifestmodule to the imports is appropriate and necessary for the updated test functionality.
7-10: Test helper function updated correctly.The
parse_manifesthelper now properly delegates tomanifest::from_strand returnsanyhow::Result<NetsukeManifest>, which aligns with the new API. The function maintains its role as a convenience wrapper whilst adopting the new error handling approach.
21-21: All existing tests updated to use new manifest API consistently.The systematic replacement of direct
serde_ymlcalls withmanifest::from_strthroughout the test suite is thorough and correct. This ensures all tests benefit from the improved error handling and consistency provided by the new manifest module.Also applies to: 51-51, 56-56, 65-65, 79-79, 90-90, 99-99, 108-108, 121-121, 139-139, 155-155, 181-181, 202-202, 247-247, 260-260
362-375: File loading tests provide good coverage of the new functionality.The tests for
manifest::from_pathcorrectly verify both successful loading and error handling for missing files. The tests appropriately useexpectfor successful cases and verify error conditions for failures.
377-389: Parameterised tests efficiently cover multiple manifest examples.The use of
rstestto parameterise tests for different manifest files is excellent. The test verifies that different manifest types can be loaded and that their first target names match expectations, providing good coverage with minimal code duplication.
391-399: Invalid manifest tests ensure proper error handling.The parameterised tests for invalid manifests correctly verify that parsing fails for various error conditions. This provides important coverage for the error handling paths in the manifest parsing code.
1 participant
Summary
manifestmoduleTesting
make fmtmake lintmake testmake markdownlintmake nixie(fails: FileNotFound copying files)https://chatgpt.com/codex/tasks/task_e_68840c33dba0832286a182bfe899d6b9
Summary by Sourcery
Introduce a new manifest module to parse Netsukefile YAML via from_str and from_path helper functions, replace raw serde_yaml usage throughout tests and Cucumber steps, and broaden test coverage with YAML fixtures and BDD scenarios while updating related documentation and dependencies.
New Features:
Enhancements:
Build:
Documentation:
Tests: