Migrate manifest parsing from serde_yml to serde_saphyr#215
Conversation
Reviewer's GuideThis PR migrates the manifest parsing pipeline from the archived serde_yml crate to serde_saphyr, using serde_json::Value as the intermediate representation. It systematically replaces imports and APIs, refactors the expansion and deserialization flow to work with JSON values, introduces differentiated error diagnostics for parsing vs data errors with accurate source spans, and refreshes documentation and test suites to align with the new parser. Sequence diagram for manifest parsing and deserialization flowsequenceDiagram
participant "User"
participant "Manifest Pipeline"
participant "serde_saphyr"
participant "serde_json"
participant "Jinja Engine"
"User"->>"Manifest Pipeline": Provide Netsukefile YAML
"Manifest Pipeline"->>"serde_saphyr": Parse YAML to serde_json::Value
"serde_saphyr"-->>"Manifest Pipeline": Return serde_json::Value or error
"Manifest Pipeline"->>"Jinja Engine": Expand foreach/when and macros
"Jinja Engine"-->>"Manifest Pipeline": Return expanded serde_json::Value
"Manifest Pipeline"->>"serde_json": Deserialize to NetsukeManifest
"serde_json"-->>"Manifest Pipeline": Return NetsukeManifest or data error
"Manifest Pipeline"->>"User": Return manifest or diagnostics
Class diagram for manifest parsing pipeline migrationclassDiagram
class NetsukeManifest {
netsuke_version: Version
vars: HashMap<String, serde_json::Value>
rules: Vec<Rule>
targets: Vec<Target>
}
class Target {
order_only_deps: StringOrList
vars: HashMap<String, serde_json::Value>
phony: bool
always: bool
}
class StringOrList
class serde_json_Value
class Rule {
// attributes unchanged, but now uses serde_json::Value where applicable
}
NetsukeManifest --> Target : contains
Target --> StringOrList : uses
Target --> serde_json_Value : uses
NetsukeManifest --> serde_json_Value : uses
NetsukeManifest --> Rule : contains
Rule --> serde_json_Value : uses
Class diagram for error diagnostics in manifest parsingclassDiagram
class ManifestError {
Parse: Diagnostic
}
class YamlDiagnostic {
source: serde_saphyr::Error
message: String
}
class DataDiagnostic {
source: serde_json::Error
message: String
}
ManifestError --> YamlDiagnostic : uses for YAML parse errors
ManifestError --> DataDiagnostic : uses for data errors
YamlDiagnostic --> serde_saphyr_Error : wraps
DataDiagnostic --> serde_json_Error : wraps
class serde_saphyr_Error
class serde_json_Error
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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 CodeRabbitRelease Notes
WalkthroughMigrate YAML parsing from serde_yml to serde_saphyr and replace YAML Value usage with serde_json::Value across dependencies, docs, manifest parsing, diagnostics, expansion, rendering and tests; add ManifestSource/ManifestName types and map_data_error diagnostic mapping. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / caller
participant Parser as serde_saphyr
participant Source as ManifestSource
participant Diagnostics as diagnostics
participant Expander as expand_foreach
participant Jinja as Jinja macros & render
CLI->>Parser: parse manifest text
alt parse success
Parser-->>CLI: return ManifestValue + ManifestSource
else parse error
Parser->>Diagnostics: map_yaml_error(err, Source, Name)
Diagnostics-->>CLI: return diagnostic
end
CLI->>Expander: expand_foreach(&mut ManifestValue)
Expander->>ManifestValue: mutate arrays/objects, inject vars
Expander-->>Jinja: pass expanded ManifestValue
alt deserialise/render error
Jinja->>Diagnostics: map_data_error(serde_json::Error, Name)
Diagnostics-->>CLI: return diagnostic
else success
Jinja-->>CLI: render targets
end
note right of Diagnostics #d5f5e3: New ManifestSource/Name\nbyte-index spans and data-error mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![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.
![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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Translates manifest parsing errors into actionable diagnostics.
use miette::{Diagnostic, NamedSource, SourceSpan};
use serde_yml::{Error as YamlError, Location};
use serde_saphyr::{Error as YamlError, Location};❌ New issue: String Heavy Function Arguments Please consider the following guidance where applicable: Use NewTypes to model domain values and eliminate "integer soup". Reach for |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 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(10 hunks)docs/roadmap.md(3 hunks)src/ast.rs(4 hunks)src/manifest.rs(5 hunks)src/manifest/diagnostics.rs(3 hunks)src/manifest/expand.rs(9 hunks)src/manifest/jinja_macros.rs(2 hunks)src/manifest/render.rs(1 hunks)src/manifest/tests.rs(3 hunks)tests/ast_tests.rs(1 hunks)tests/steps/manifest_steps.rs(1 hunks)tests/yaml_error_tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/manifest/diagnostics.rstests/ast_tests.rstests/yaml_error_tests.rssrc/ast.rssrc/manifest/jinja_macros.rstests/steps/manifest_steps.rssrc/manifest/tests.rssrc/manifest/expand.rssrc/manifest.rssrc/manifest/render.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 / -yse / -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...
Files:
src/manifest/diagnostics.rstests/ast_tests.rstests/yaml_error_tests.rssrc/ast.rssrc/manifest/jinja_macros.rstests/steps/manifest_steps.rssrc/manifest/tests.rssrc/manifest/expand.rssrc/manifest.rssrc/manifest/render.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/manifest/diagnostics.rstests/ast_tests.rstests/yaml_error_tests.rsdocs/netsuke-design.mdsrc/ast.rsdocs/roadmap.mdsrc/manifest/jinja_macros.rstests/steps/manifest_steps.rssrc/manifest/tests.rssrc/manifest/expand.rssrc/manifest.rssrc/manifest/render.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Mandate SemVer-compatible caret requirements (e.g., some-crate = "1.2.3") for all dependencies
Prohibit wildcard (*) and open-ended (>=) version requirements; use tilde (~) only with documented reasons to lock to patch updates
Use explicit version ranges and keep dependencies up to date
Files:
Cargo.toml
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/ast_tests.rstests/yaml_error_tests.rssrc/manifest/tests.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/ast_tests.rstests/yaml_error_tests.rstests/steps/manifest_steps.rs
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.mddocs/roadmap.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the reference knowledge base; proactively update docs/ when decisions, requirements, dependencies, or architecture change
Documentation in docs/ must use en-GB-oxendict spelling and grammar (LICENSE file name exempt)
docs/**/*.md: The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle brackets<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding figures, useand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/netsuke-design.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown using make markdownlint; run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/netsuke-design.mddocs/roadmap.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -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/netsuke-design.mddocs/roadmap.md
🧬 Code graph analysis (5)
src/manifest/diagnostics.rs (1)
src/manifest/tests.rs (1)
serde_saphyr(151-151)
tests/ast_tests.rs (2)
src/manifest/diagnostics.rs (1)
serde_saphyr(161-161)src/manifest/tests.rs (1)
serde_saphyr(151-151)
src/manifest/tests.rs (1)
src/manifest/jinja_macros.rs (1)
register_manifest_macros(100-113)
src/manifest/expand.rs (2)
src/manifest/diagnostics.rs (1)
serde_saphyr(161-161)src/manifest/tests.rs (1)
serde_saphyr(151-151)
src/manifest.rs (2)
src/manifest/diagnostics.rs (3)
map_data_error(142-151)map_yaml_error(106-130)serde_saphyr(161-161)src/manifest/tests.rs (1)
serde_saphyr(151-151)
⏰ 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). (8)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (12)
Cargo.toml (1)
22-22: Dependency version is correct.The
serde-saphyr = "0.0.6"declaration follows SemVer conventions. For 0.0.x releases, this is equivalent to^0.0.6, which correctly locks to the exact patch version given the lack of stability guarantees in the 0.0.x range.docs/roadmap.md (1)
25-25: Documentation correctly reflects the migration.The roadmap updates accurately describe the shift to
serde_saphyrandserde_json::Valuefor intermediate representations. The completed checkboxes properly track the migration work.Also applies to: 83-86, 122-125
src/manifest.rs (3)
117-119: Parser migration correctly implemented.The switch from
serde_yml::from_strtoserde_saphyr::from_strproperly uses the new parser, and the error is correctly mapped viamap_yaml_errorto preserve diagnostic information with source spans.
128-132: Variable population logic correctly simplified.The change from
as_mapping()toas_object()reflects the JSON value semantics. Becauseserde_json::Valueobjects haveStringkeys natively (unlike YAML mappings which can have arbitrary keys), the iteration directly yields(String, Value)pairs. This eliminates the need for key conversion and simplifies the code correctly.
139-141: Deserialisation path updated with proper error handling.The switch to
serde_json::from_valueis correct for the new intermediate representation, and the addition ofmap_data_errorensures that deserialisation errors are wrapped with appropriate diagnostic context for display viamiette.docs/netsuke-design.md (2)
48-50: Design documentation thoroughly updated.The design document now accurately describes the use of
serde_saphyrfor YAML parsing andserde_json::Valuefor intermediate representation. The rationale section (lines 401-427) clearly explains whyserde_saphyrwas chosen and how it integrates with JSON-based intermediate structures.Also applies to: 401-427
451-451: Code examples correctly updated.The type signatures in the AST documentation snippets now show
HashMap<String, serde_json::Value>for bothNetsukeManifest.varsandTarget.vars, which accurately reflects the implementation changes.Also applies to: 503-503
tests/steps/manifest_steps.rs (1)
299-299: Value type correctly updated in test.The change from
serde_yml::Value::as_u64toserde_json::Value::as_u64is a straightforward API swap with identical semantics, maintaining the test's assertion logic.tests/ast_tests.rs (1)
220-220: Serialisation correctly updated for round-trip testing.The switch to
serde_saphyr::to_stringensures that macro definitions can be serialised using the same library that parses them, maintaining round-trip consistency in tests.src/manifest/render.rs (1)
5-5: Type alias updated consistently.The
YamlValuealias change matches the same update insrc/manifest.rs. The earlier comment about renaming this alias for clarity applies here as well.src/manifest/jinja_macros.rs (1)
14-14: Macro registration correctly migrated.Both the type alias and the deserialisation path have been updated to use
serde_json::Valueandserde_json::from_value, maintaining consistency with the broader migration whilst preserving error context.Also applies to: 105-105
tests/yaml_error_tests.rs (1)
24-27: Disregard review comment; assertion wording is accurate to serde_yaml output.The review claims serde_saphyr produces "simple key expected ':'" (with "expected"), but the actual error message in the codebase reads "simple key expect ':'" (with "expect"). The proposed diff cannot work: splitting "simple key expect ':'" into the substrings ["simple key", "expected ':'"] fails because the second substring does not exist in the actual message—the message contains "expect ':'" not "expected ':'".
The current assertion at line 26 is already pinned to the exact wording serde_yaml emits. The test is correct as written.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/manifest/diagnostics.rs (2)
241-266: Add a UTF‑8 coverage test for location_to_indexProve correctness on multi‑byte code points to prevent span drift on non‑ASCII.
@@ fn map_yaml_error_defaults_location_when_missing() { let src = ManifestSource::from("foo: [1"); let err = serde_saphyr::Error::Eof { location: serde_saphyr::Location::UNKNOWN, }; let name = ManifestName::from("test"); let diag = map_yaml_error(err, &src, &name); assert!(diag.to_string().contains("line 1, column 1")); } + + #[test] + fn location_to_index_handles_utf8() { + // café: 'é' is multi‑byte + let src = ManifestSource::from("café\n"); + let loc_f = Location::new(1, 2); + let loc_e = Location::new(1, 5); + let idx_f = location_to_index(&src, loc_f); + let idx_e = location_to_index(&src, loc_e); + assert_eq!(&src.as_ref().as_bytes()[idx_f..idx_f + 1], b"f"); + assert_eq!(src.as_ref()[idx_e..].chars().next().unwrap(), 'é'); + }
220-239: Prefix structure errors with diagnostic code for consistencyAlign message format with other diagnostics and prior guidance.
- let message = format!("manifest structure error in {}: {err}", name.as_ref()); + let message = format!( + "[netsuke::manifest::structure] manifest structure error in {}: {err}", + name.as_ref() + );src/manifest.rs (2)
13-13: Rename misleading alias; reflect JSON realityRename YamlValue to DataValue (or IntermediateValue) to reduce cognitive dissonance now that it is serde_json::Value.
-use serde_json::Value as YamlValue; +use serde_json::Value as DataValue; @@ - let mut doc: YamlValue = serde_saphyr::from_str(yaml)... + let mut doc: DataValue = serde_saphyr::from_str(yaml)...#!/usr/bin/env bash # Update references project‑wide rg -nP '\bYamlValue\b' -g '!**/target/**'
85-88: Document and test the constraint: mapping keys must be stringsSwitching to serde_json::Value implies YAML mappings with non‑string keys are unsupported. Surface this in docs and add a failing test that shows the error path so users get a clear message.
Do you want a unit/BDD test added that asserts a readable error on non‑string YAML keys?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/manifest.rs(4 hunks)src/manifest/diagnostics.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/manifest/diagnostics.rssrc/manifest.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 / -yse / -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...
Files:
src/manifest/diagnostics.rssrc/manifest.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/manifest/diagnostics.rssrc/manifest.rs
🧬 Code graph analysis (2)
src/manifest/diagnostics.rs (2)
src/manifest/tests.rs (1)
serde_saphyr(151-151)src/manifest.rs (1)
from_str(111-113)
src/manifest.rs (2)
src/manifest/diagnostics.rs (7)
map_data_error(230-239)map_yaml_error(194-218)serde_saphyr(248-248)from(30-32)from(36-38)from(71-73)from(77-79)src/manifest/tests.rs (1)
serde_saphyr(151-151)
🔍 Remote MCP Deepwiki
Based on my search results, I can now provide you with a comprehensive summary of relevant context for this PR review.
Relevant Context for PR #215 Review
Key Context: This is a Migration PR
The DeepWiki results reveal an important mismatch: the current codebase (as documented in DeepWiki) uses serde_yml and explicitly chose it over serde_saphyr due to maturity and serde integration concerns. This PR proposes reversing that decision. The PR context indicates that the current repository likely reflects the post-migration state, while DeepWiki reflects the pre-migration documentation.
Manifest Parsing Pipeline Impact
The migration from serde_yml to serde_saphyr and the switch to serde_json::Value as an intermediate representation affects a critical multi-stage parsing process:
- Original design (serde_yml): YAML parsed to
serde_yml::Value→ templating on native YAML types → deserialization to NetsukeManifest - New design (serde_saphyr + serde_json): YAML parsed via serde_saphyr → intermediate
serde_json::Value→ templating → deserialization to NetsukeManifest
Critical consideration: The original design explicitly relied on preserving native YAML types (booleans, numbers) for Jinja templating control flow. Using serde_json::Value as an intermediate representation requires careful validation that Jinja conditionals and loops still function correctly with JSON-equivalent type semantics.
Diagnostics Module Refactoring
The src/manifest/diagnostics.rs changes introduce new public types (ManifestSource and ManifestName) as domain-specific wrappers. The PR objectives note a CodeScene flag for "String Heavy Function Arguments" (53.3% string args vs. 39% threshold). The suggested fix is to use these NewType wrappers to reduce raw string parameters from 53.3% to approximately 21.4%, addressing code quality without changing runtime behavior.
Review focus: The new helper functions like location_to_index and saturating_usize represent a shift in how serde_saphyr::Location (different from serde_yml::Location) is converted to diagnostic spans. Verify this span calculation logic produces correct line/column hints.
Error Diagnostics Alignment
The PR adds map_data_error to handle serde_json::Error (deserialization stage) in addition to the existing map_yaml_error for YAML parsing errors. This reflects the two-stage error handling: YAML parsing (serde_saphyr + map_yaml_error) and JSON deserialization (serde_json + map_data_error). Test expectations in yaml_error_tests.rs show updated diagnostic messages and span positions, indicating the parser behavior has changed.
Breaking Changes to Public API
Three items are newly exposed in the public API via src/manifest.rs:
ManifestNameManifestSourcemap_data_error
This expands the surface area for manifest error handling, requiring attention to whether downstream consumers need updates.
⏰ 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). (4)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (5)
src/manifest/diagnostics.rs (2)
118-135: Span construction is robust; clamp start/end defensivelyLGTM. The newline/edge handling avoids OOB and produces minimal spans.
193-218: YAML diagnostic wiring looks correctSource, span, help and message are consistent with miette expectations. Proceed.
src/manifest.rs (3)
23-25: Re‑exports look rightExpose ManifestName/ManifestSource/map_data_error alongside map_yaml_error. No issues.
74-76: Map YAML parse errors with source/name contextGood. Keep passing ManifestSource and ManifestName to preserve helpful spans.
95-98: Wire data deserialisation errors to diagnosticsLGTM. The new map_data_error keeps error provenance and unifies reporting.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/manifest/diagnostics.rs (4)
96-116: Reject prior suggestion to use Location::index(); it does not exist.The serde_saphyr Location API intentionally exposes only line()/column() (1‑based). Keep computing byte offsets from those values. (docs.rs)
14-16: Derive Eq/Hash and implement Display on ManifestSource for parity and ergonomics.Model the domain value fully; enable map/set keys and formatted use.
-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ManifestSource(String); @@ impl AsRef<str> for ManifestSource { fn as_ref(&self) -> &str { self.0.as_str() } } + +impl std::fmt::Display for ManifestSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0.as_str()) + } +}
152-159: Avoid .lines().nth(line_idx) on hot paths.Iterate once with split_inclusive and stop at line_idx; this prevents O(n²) behaviour for large inputs.
- let line_idx = usize::try_from(loc.line().saturating_sub(1)).unwrap_or(usize::MAX); - let line = src.as_ref().lines().nth(line_idx).unwrap_or(""); + let line_idx = usize::try_from(loc.line().saturating_sub(1)).unwrap_or(usize::MAX); + let mut line = ""; + for (i, seg) in src.as_ref().split_inclusive('\n').enumerate() { + if i == line_idx { + line = if let Some(without_nl) = seg.strip_suffix('\n') { + without_nl.strip_suffix('\r').unwrap_or(without_nl) + } else { + seg + }; + break; + } + }
229-239: Prefix structure errors with a stable diagnostic code in the message.Align message style with other diagnostics.
- let message = format!("manifest structure error in {}: {err}", name.as_ref()); + let message = format!( + "[netsuke::manifest::structure] manifest structure error in {}: {err}", + name.as_ref() + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/manifest.rs(4 hunks)src/manifest/diagnostics.rs(4 hunks)src/manifest/expand.rs(8 hunks)src/manifest/jinja_macros.rs(2 hunks)src/manifest/render.rs(4 hunks)src/manifest/tests.rs(2 hunks)tests/ast_tests.rs(2 hunks)tests/manifest_jinja_tests.rs(1 hunks)tests/steps/manifest_steps.rs(2 hunks)tests/yaml_error_tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
tests/ast_tests.rssrc/manifest/expand.rstests/steps/manifest_steps.rssrc/manifest/tests.rstests/yaml_error_tests.rstests/manifest_jinja_tests.rssrc/manifest/render.rssrc/manifest/diagnostics.rssrc/manifest/jinja_macros.rssrc/manifest.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 / -yse / -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...
Files:
tests/ast_tests.rssrc/manifest/expand.rstests/steps/manifest_steps.rssrc/manifest/tests.rstests/yaml_error_tests.rstests/manifest_jinja_tests.rssrc/manifest/render.rssrc/manifest/diagnostics.rssrc/manifest/jinja_macros.rssrc/manifest.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
tests/ast_tests.rssrc/manifest/expand.rstests/steps/manifest_steps.rssrc/manifest/tests.rstests/yaml_error_tests.rstests/manifest_jinja_tests.rssrc/manifest/render.rssrc/manifest/diagnostics.rssrc/manifest/jinja_macros.rssrc/manifest.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/ast_tests.rssrc/manifest/tests.rstests/yaml_error_tests.rstests/manifest_jinja_tests.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/ast_tests.rstests/steps/manifest_steps.rstests/yaml_error_tests.rstests/manifest_jinja_tests.rs
🧬 Code graph analysis (6)
tests/ast_tests.rs (4)
tests/yaml_error_tests.rs (1)
err(90-92)src/manifest/diagnostics.rs (1)
serde_saphyr(248-248)src/manifest/tests.rs (2)
serde_saphyr(165-165)serde_saphyr(188-188)src/manifest.rs (1)
from_str(124-126)
src/manifest/expand.rs (2)
src/manifest/diagnostics.rs (5)
from(30-32)from(36-38)from(71-73)from(77-79)serde_saphyr(248-248)src/manifest/tests.rs (2)
serde_saphyr(165-165)serde_saphyr(188-188)
src/manifest/tests.rs (2)
src/manifest/jinja_macros.rs (5)
new(251-257)new(291-309)new(331-335)new(392-399)register_manifest_macros(100-113)src/manifest.rs (1)
from_str(124-126)
tests/manifest_jinja_tests.rs (2)
tests/ast_tests.rs (1)
err(95-98)tests/yaml_error_tests.rs (1)
err(90-92)
src/manifest/diagnostics.rs (2)
src/manifest/tests.rs (2)
serde_saphyr(165-165)serde_saphyr(188-188)src/manifest.rs (1)
from_str(124-126)
src/manifest.rs (1)
src/manifest/diagnostics.rs (6)
map_data_error(230-239)map_yaml_error(194-218)from(30-32)from(36-38)from(71-73)from(77-79)
🔍 Remote MCP Deepwiki, Ref
Concise additional context for PR #215 review
-
High-level change: repository-wide migration from serde_yml → serde_saphyr and use of serde_json::Value as the intermediate untyped representation; Cargo.toml replaces serde_yml with serde-saphyr.,
-
Areas directly affected (files/modules to focus review on):
- manifest parsing & pipeline: src/manifest.rs, src/manifest/expand.rs, src/manifest/render.rs — parsing now produces JSON-like ManifestValue and deserialises via serde_json::from_value.
- diagnostics: src/manifest/diagnostics.rs — new ManifestSource/ManifestName types, new span/index helpers, map_data_error added to convert serde_json deserialization errors to diagnostics; map_yaml_error signatures updated.
- public API/exports: src/manifest.rs now publicly exposes ManifestValue/ManifestMap/ManifestName/ManifestSource and map_data_error — review for semver/public-surface impact.
- AST types/docs: src/ast.rs and docs updated to use serde_json::Value for Vars and examples — check downstream consumers for required changes.
- jinja/macros & manifest macros: src/manifest/jinja_macros.rs signature now takes ManifestValue and uses serde_json::from_value.
- IR/Ninja generation not functionally changed but ensure command escaping/validation unchanged after value-type change.
-
Tests & expected behavior changes:
- Unit and BDD tests updated to use serde_saphyr and ManifestValue; several diagnostic messages/positions adjusted (line/column changes and different error text). Review test diffs for intended behavioral differences in error reporting.
-
Code quality note / refactor suggestion already surfaced in PR comments:
- CodeScene flagged "String Heavy Function Arguments" in src/manifest/diagnostics.rs; suggested newtype wrappers ManifestSource and ManifestName (constructors, AsRef/From impls) to reduce raw &str parameters — PR already added those types according to PR objectives, so verify consistent usage at call sites and that this resolves the CodeScene warning.
-
Key review action items
- Verify diagnostics: confirm location → byte-index/span calculations (new Location API handling) produce correct line/column hints (tests adjusted accordingly).
- API surface: confirm intentionality of new public exports (ManifestValue/ManifestMap/ManifestSource/ManifestName/map_data_error) and update downstream crates/docs if needed.
- Template behavior: ensure templating logic (foreach/when, Jinja context) preserves semantics under serde_json::Value (types/truthiness differences between YAML-specific Value vs JSON Value).
- Tests: run CI/test matrix (make test) and inspect changed expectations in yaml_error_tests.rs, manifest_jinja_tests.rs and BDD/test snapshots for any unintended regressions.
⏰ 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: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (22)
src/manifest/expand.rs (3)
2-5: LGTM—imports correctly use ManifestValue and serde_json types.The migration to serde_json-based types is clean and consistent.
7-28: Function correctly handles foreach expansion with new types.The logic is clear and error handling is appropriate.
111-186: Tests correctly verify foreach expansion with new types.The test coverage is appropriate and uses serde_saphyr parsing correctly.
tests/steps/manifest_steps.rs (2)
1-12: Module doc and imports are appropriate.The step definitions module is well documented and imports are correct.
294-303: Correctly migrated to use ManifestValue::as_u64().The type migration is accurate and maintains the same functionality.
tests/manifest_jinja_tests.rs (1)
445-457: Test correctly validates error message for JSON-based types.The update from "mapping" to "object" aligns with serde_json terminology, and the error chain traversal is appropriate.
tests/ast_tests.rs (3)
84-104: Excellent addition—validates vars section type constraint.The test correctly verifies that the vars section must be an object and checks the error message via the error chain.
223-245: Serialisation correctly migrated to serde_saphyr.The macro serialisation test now uses the correct parser and verifies output consistency.
247-267: Well-done—tests special character round-trip serialisation.This test addresses the past review suggestion and ensures robust handling of special characters (quotes, newlines, Unicode) in macro definitions.
src/manifest/render.rs (2)
1-99: Rendering logic correctly uses ManifestValue types.The migration is clean, and the rendering pipeline remains intact with the new types.
101-205: Tests correctly construct fixtures with ManifestValue.The test scaffolding properly uses the new types and verifies rendering behaviour.
tests/yaml_error_tests.rs (1)
14-98: Test expectations correctly updated for serde_saphyr error reporting.The updated line/column coordinates and error messages align with the new parser's diagnostic output. The coverage of edge cases (empty input, comments, nested structures) is appropriate.
src/manifest/jinja_macros.rs (2)
1-21: Module documentation is comprehensive and well structured.The module-level doc clearly explains the purpose and provides context for the macro registration system.
100-113: Function signature correctly updated to use ManifestValue.The migration from YAML-based to JSON-based value types is properly reflected in the public API, and the deserialisation path using
serde_json::from_valueis correct.src/manifest/tests.rs (4)
1-24: Module doc and test utilities are well structured.The test infrastructure is clear and follows best practices.
122-136: Test correctly validates macro shape with ManifestValue types.The test appropriately constructs invalid input and verifies the error message.
138-171: Excellent coverage—tests non-string macro values and keys.These tests address edge cases in macro parsing and ensure robust validation of the data structure.
173-195: Tests correctly exercise macro registration with new types.The test coverage for missing fields and multiple macros is appropriate and uses serde_saphyr parsing correctly.
src/manifest/diagnostics.rs (2)
96-116: Keep line/column→byte logic; serde_saphyr::Location exposes only 1‑based line()/column().Do not replace with a hypothetical byte index; the API has no index() and you already account for UTF‑8 using char_indices, with tests covering multi‑byte correctness. (docs.rs)
268-304: LGTM: UTF‑8 column→byte tests are solid.The tests validate multi‑byte handling and end‑of‑line clamping; keep them.
src/manifest.rs (2)
77-81: LGTM: YAML→serde_saphyr error mapping with ManifestSource context is correct.Error propagation preserves location information via map_yaml_error.
109-111: LGTM: Final deserialisation via serde_json::from_value mapped to structure diagnostics.Keeps parse vs structure error separation intact.
|
@coderabbitai Have the following now been resolved? |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
docs/netsuke-design.md (1)
787-793: Release Notes section—ensure library consumers are clearly informed of API surface changes.Section 8.7 documents re-exports (
ManifestValue,ManifestMap,ManifestSource,ManifestName,map_yaml_error,map_data_error) but the framing "should upgrade to these symbols" is passive. Explicitly state which symbols replace or supersede older APIs, if any exist, to prevent confusion during consumer migration.Clarify the migration path in the Release Notes:
### 8.7 Release Notes Netsuke's manifest loader now re-exports the `ManifestValue` and `ManifestMap` type aliases, alongside the `ManifestSource`, `ManifestName`, and diagnostic mapping functions `map_yaml_error` and `map_data_error`. External consumers interacting with manifest data or producing diagnostics must import these symbols from the library root; the API surface has stabilised and must be highlighted in release notes. See [Migration Guide](#migration-guide) for details.This signals breaking changes (if any) and directs readers to migration guidance.
📜 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 (4)
Cargo.toml(2 hunks)docs/netsuke-design.md(12 hunks)src/manifest/diagnostics.rs(4 hunks)src/manifest/expand.rs(9 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 / -yse / -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/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 / -yse / -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...
Files:
src/manifest/diagnostics.rssrc/manifest/expand.rs
🪛 GitHub Actions: Release Dry Run
Cargo.toml
[error] 1-1: CI step failed: python3 .github/workflows/scripts/read_manifest.py --manifest-path "$manifest_path" name 2>/tmp/name.err" produced a Cargo.toml parse failure: Could not read
🔇 Additional comments (14)
docs/netsuke-design.md (4)
403-429: Comprehensive serde_saphyr migration rationale—excellent.Section 3.1 clearly articulates the architectural shift to
serde_saphyrand explains the intermediateserde_json::Valuedesign elegantly. The rationale for omitting a bespoke YAMLValuetype and leveraging JSON's well-established data model improves maintainability and clarity.Ensure the reference to "Architecture Decision Record" (line 427–429) is documented in the repository; readers should be able to locate it from the design document footnotes or a dedicated ADR index.
Verify the ADR reference exists in the repository at a documented location (e.g., `docs/adr/` or similar). If it does not yet exist, create a placeholder or update line 427–429 to reference where it will be published.
46-61: Six-stage pipeline clearly updated—confirm Stage 2 and Stage 3 descriptions reflect implementation.Lines 46–50 and 54–61 accurately reflect the new two-pass approach: YAML parsing into
serde_json::Value, followed by template expansion in-place. The language is precise ("walks the parsedValue", "Jinja must not modify the YAML structure directly").Cross-reference with the actual implementation in
src/manifest.rsandsrc/expansion.rs(or equivalent modules) to confirm the pipeline stages match documented behaviour, especially regarding the "forbidding structural Jinja blocks" constraint.Verify that Stages 2 and 3 implementation align with the documented pipeline:
- Stage 2 parses YAML →
serde_json::Value- Stage 3 expands
foreachandwhenkeys in-place; structural{% %}blocks are rejected- Jinja rendering occurs only in Stage 4 (after deserialisation)
Run integration tests (
make test) to confirm the pipeline behaviour matches documentation.
817-818: Roadmap phases—verify Phase 1 and Phase 2 task descriptions remain actionable.Lines 817–818 (Phase 1, Task 2) now references
serde_saphyrinstead of the prior YAML parser. Lines 842 (Phase 2, Task 2) similarly updates the rendering order. Both are consistent with the migration but should be verified against the actual implementation's task breakdown to ensure the phasing is realistic.Confirm with the development team that:
- Phase 1, Task 2 (YAML parsing with
serde_saphyr) is achievable within the phase's success criterion (static manifest compilation to Ninja).- Phase 2, Task 2 (two-pass rendering with
minijinja+serde_saphyr) does not introduce dependencies or complexity that conflict with Phase 1 deliverables.- The success criteria for each phase remain achievable given the updated tooling.
Also applies to: 842-842
885-885: Technology Summary table—confirm serde_saphyr rationale is complete.Line 885 updates the technology summary table to list
serde_saphyrwith the rationale: "Maintained, panic-free YAML 1.2 parser with a serde-compatible API."This is concise but omits why
serde_saphyrwas chosen over remaining alternatives (e.g.,yaml-rust2,serde-yamldescendants). Readers reviewing the roadmap may wonder whether the rationale is sufficient. Cross-reference with Section 3.1 (the ADR) to ensure consistency.Verify that the ADR mentioned in Section 3.1 (lines 427–429) provides the detailed comparison and decision-making process. If the ADR is not yet finalised, add a footnote to line 885 referencing it or linking to the discussion thread.
Cargo.toml (1)
22-22: LGTM: dependency migration aligns with PR objectives.The replacement of
serde_ymlwithserde-saphyrand the addition ofpreserve_ordertoserde_jsonare both appropriate. Thepreserve_orderfeature is justified by the test insrc/manifest/expand.rs(lines 194–223) that validates key order stability.Note that
serde-saphyr = "0.0.6"locks to exactly version 0.0.6 under semver rules (0.0.x series treats each patch as potentially breaking). Monitor for API stability as the library matures.Also applies to: 40-40
src/manifest/diagnostics.rs (5)
6-98: LGTM: wrapper types are well-designed.The
ManifestSourceandManifestNamenewtypes provide clear domain modelling and reduce the string-heavy function signature issue flagged by CodeScene. Both types implement appropriate traits (Display,From,AsRef,Eq,Hash) and include rustdoc with examples, adhering to the coding guidelines.
135-153: LGTM: span calculation handles edge cases correctly.The
to_spanfunction correctly handles newlines (both\nand\r), EOF, and single-character spans. The use of#[expect(clippy::useless_conversion)]with a clear reason is appropriate for future-proofing.
211-241: LGTM: YAML error mapping is well-documented and correct.The function includes clear rustdoc explaining its purpose and span availability semantics, following the coding guidelines. The implementation correctly uses the
ManifestSourceandManifestNamewrappers and handles optional locations gracefully.
243-269: LGTM: data error mapping provides clear diagnostics.The function includes rustdoc explaining the absence of source spans for
serde_jsonstructural errors. The diagnostic message correctly includes the[netsuke::manifest::structure]prefix for consistency, addressing previous feedback.
271-375: LGTM: comprehensive test coverage.The test suite thoroughly covers edge cases including tab indentation hints, missing locations, CRLF handling, and UTF-8 multi-byte characters. The tests correctly use
.expect()for error messages and validate both happy paths and boundary conditions.src/manifest/expand.rs (4)
13-47: LGTM: foreach expansion logic is clear and correct.The expansion correctly handles both array-literal and expression-based
foreachvalues, removes theforeachkey from expanded targets, and applieswhenfiltering. The use ofManifestValue::Objectwrapping is consistent throughout.
76-102: LGTM: iteration variable injection is robust.The function correctly uses
EntryAPI to handle existing or absentvars, validates thatvarsis an object (with a clear error message), and injects bothitemandindex. The castindex as u64on line 99 is appropriate for tier-1 targets.
49-115: LGTM: helper functions are clear and focused.The helper functions correctly use
ManifestValueaccessors and provide descriptive error context. The separation of concerns (parsing, filtering, evaluation) aids readability.
193-223: LGTM: key order preservation test validates Cargo.toml change.This test confirms that the
preserve_orderfeature added toserde_jsoninCargo.toml(line 40) works as intended. The validation of key order["name", "vars", "after"]afterforeachandwhenremoval demonstrates stable insertion order, which is important for predictable manifest rendering.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
codex/migrate-from-serde-yml-to-serde-saphyr
branch
from
c994e71 to
659d5a9
Compare
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1080 @leynos: You're absolutely correct on both counts, and thank you for the precise correction. 1. Import paths: The types are indeed exported as use netsuke::manifest::{ManifestValue, ManifestMap, ManifestSource, ManifestName, map_yaml_error, map_data_error};2. Migration Guide link: The link to I apologise for the inaccuracies in my earlier review comment. These details matter for accurate guidance. ✏️ Learnings added
|
- Spell out the `E0001` diagnostic emitted for non-string `vars` keys so
consumers understand the parse-time failure mode.
- Introduce the explicit `ManifestMap` alias snippet and connect it to
`serde_json`'s `preserve_order` guarantee for determinism.
- Reflow the surrounding narrative to keep the type alias and target flags
discussion cohesive.
Tests: make fmt
make check-fmt
make lint
make markdownlint
make nixie (with sandbox disabled)
make test (fails: cucumber suite reports "missing build file: $2")
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f25faa850c8322ba80963d2ef8050a
Summary by Sourcery
Migrate the manifest parsing pipeline from the archived serde_yml crate to the maintained serde_saphyr crate, switching to serde_json::Value for intermediate representation and updating error diagnostics, code, tests, and documentation accordingly.
Enhancements:
Build:
Documentation:
Tests: