Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1520 +/- ##
==========================================
- Coverage 92.02% 91.93% -0.09%
==========================================
Files 89 89
Lines 18500 18288 -212
==========================================
- Hits 17024 16813 -211
+ Misses 1476 1475 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
serde-saphyr has way better error message, and it is written in safe Rust.
📦 Cargo Bloat ComparisonBinary size change: +3.10% (22.6 MiB → 23.3 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
There was a problem hiding this comment.
Pull request overview
This PR switches prek’s YAML parsing from serde_yaml to serde-saphyr to improve user-facing YAML error messages (including richer spans/snippets), and updates tests/schema accordingly.
Changes:
- Replace
serde_yamlwithserde-saphyrfor config/manifest parsing and thecheck-yamlbuiltin hook. - Update integration/unit test snapshots to match the new error formatting.
- Regenerate/update
prek.schema.jsonand dependency manifests/lockfile for the new YAML stack.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| prek.schema.json | Regenerated schema reflecting updated config model/serialization details. |
| crates/prek/src/config.rs | Switch config/manifest parsing to serde-saphyr and update related unit tests/snapshots. |
| crates/prek/src/hooks/pre_commit_hooks/check_yaml.rs | Use serde-saphyr to validate YAML files (single/multi-doc) and adjust error output. |
| crates/prek/src/yaml.rs | Remove prior merge-key processing code, leaving scalar serialization logic. |
| crates/prek/src/cli/run/filter.rs | Update tests to construct glob patterns without serde_yaml. |
| crates/prek/tests/workspace.rs | Snapshot updates for new YAML parse errors. |
| crates/prek/tests/validate.rs | Snapshot updates + new test coverage for invalid-config errors with new formatting. |
| crates/prek/tests/run.rs | Snapshot updates for new YAML parse errors. |
| crates/prek/tests/meta_hooks.rs | Snapshot updates for new YAML parse errors. |
| crates/prek/tests/install.rs | Snapshot updates for new YAML parse errors in warnings. |
| crates/prek/tests/builtin_hooks.rs | Snapshot updates for check-yaml output + naming tweaks in test display strings. |
| crates/prek/tests/auto_update.rs | Snapshot updates for new YAML parse errors. |
| crates/prek/Cargo.toml | Swap dependency from serde_yaml to serde-saphyr. |
| Cargo.toml | Add workspace dependency for serde-saphyr and adjust clippy lint settings. |
| Cargo.lock | Lockfile updates for serde-saphyr and its transitive dependencies. |
| let options = serde_saphyr::Options { | ||
| ignore_binary_tag_for_string: true, | ||
| ..Default::default() | ||
| }; | ||
| if allow_multi_docs { | ||
| for doc in deserializer { | ||
| if let Err(e) = serde_yaml::Value::deserialize(doc) { | ||
| let error_message = | ||
| format!("{}: Failed to yaml decode ({e})\n", filename.display()); | ||
| return Ok((1, error_message.into_bytes())); | ||
| } | ||
| if let Err(e) = serde_saphyr::from_slice_multiple_with_options::<serde_json::Value>( | ||
| &content, | ||
| options.clone(), | ||
| ) { | ||
| let error_message = format!("{}: Failed to yaml decode ({e})\n", filename.display()); | ||
| return Ok((1, error_message.into_bytes())); | ||
| } | ||
| Ok((0, Vec::new())) | ||
| } else { | ||
| match serde_yaml::from_slice::<serde_yaml::Value>(&content) { | ||
| match serde_saphyr::from_slice_with_options::<serde_json::Value>(&content, options) { | ||
| Ok(_) => Ok((0, Vec::new())), |
There was a problem hiding this comment.
check-yaml now deserializes YAML into serde_json::Value, which only supports JSON-compatible YAML (e.g., mapping keys must be strings). This can cause valid YAML files (such as those with non-string keys or other YAML-only constructs) to be reported as invalid. Consider parsing/deserializing into a YAML value type (or using a parser-only API) so this hook validates YAML syntax rather than JSON compatibility.
| let config = serde_saphyr::from_str(&content) | ||
| .map_err(|e| Error::Yaml(path.user_display().to_string(), Box::new(e)))?; |
There was a problem hiding this comment.
The serde_saphyr error string already includes a leading "error:"; wrapping it as the source for Failed to parse ... results in user output like "caused by: error: ..." / "Failed to parse ...: error: ...". Consider normalizing the source message (e.g., stripping the leading prefix) before boxing/storing it so the final output isn’t redundant.
| }, | ||
| "writeOnly": true |
There was a problem hiding this comment.
writeOnly: true on RemoteRepo.hooks is likely incorrect for a configuration-file JSON schema (it may hide the property in editors/UI tooling even though it’s required for configs). Consider removing writeOnly here unless there’s a specific schema-consumer reason for it.
| }, | |
| "writeOnly": true | |
| } |
serde-saphyr has way better error message, and it is written in safe Rust.
Previous try: #1087
Previous revert: #1106 and #1112
TODO:
Replace serialize_yaml_scalar with serde-saphyr as well:
prek/crates/prek/src/yaml.rs
Lines 12 to 13 in 0a4be22