✨ Add a represent emitter protocol to dumps#247
Open
frenck wants to merge 18 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will degrade performance by 5.07%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_dumps[medium] |
102.6 µs | 108.1 µs | -5.07% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing frenck/represent-protocol (3c1d8df) with main (0191dd4)
frenck
force-pushed
the
frenck/represent-protocol
branch
from
8adeb78 to
1eb0287
Compare
![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
🧹 Nitpick comments (1)
src/roundtrip/value.rs (1)
544-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc now that this helper is crate-public.
assigned_string_stylewas widened topub(crate), but it only has internal comments. Add a short///summary above the function. As per coding guidelines,src/**/*.rs: “Keep public Rust items documented with///rustdoc”.Proposed fix
+/// Choose the scalar style that preserves a string under the active schema. pub(crate) fn assigned_string_style(Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3e082f93-73aa-42b0-ba4d-7fc7f76e85ef
📒 Files selected for processing (13)
adr/021-emitter-represent-protocol-for-dumps.mdadr/README.mddocs/src/content/docs/guides/dumping.mdpysrc/yamlrocks/__init__.pypysrc/yamlrocks/__init__.pyisrc/emit_util.rssrc/encode/mod.rssrc/ffi/mod.rssrc/ffi/represent.rssrc/lib.rssrc/roundtrip/emit.rssrc/roundtrip/value.rstests/features/test_represent.py
There was a problem hiding this comment.
Pull request overview
This PR adds an optional represent= callback to dumps, giving hosts full, per-value control over how any value (builtins included) is emitted — an explicit tag, a scalar style, or block/flow layout — closing the gap that keeps downstreams (notably ESPHome's dumper) on import yaml. The callback returns one of three new node descriptors (YAMLRocksScalar/YAMLRocksSequence/YAMLRocksMapping) carrying the original host objects, or None to defer to the built-in rendering. When represent= is present, dumps lowers the object into a synthetic round-trip YamlNode tree and emits through the round-trip emitter (which already speaks per-node style/tag/flow), leaving the plain-dumps fast path untouched. Shared objects become anchors/aliases, OPT_SORT_KEYS applies, and deferred values are pinned byte-for-byte to a plain dumps.
Changes:
- New
src/ffi/represent.rslowering (descriptors →YamlNodetree, PyYAML-faithful auto styling, id-based anchor detection) plusrepresent=wired intodumps. - Round-trip emitter gains a
DumpConfig(sort keys, indent synthetic sequences) and anemit_roundtrip_dumpentry point;use_literal_blockis hoisted to sharedemit_util, andassigned_string_styleis madepub(crate). - Public surface: three new PyO3 classes exported from Rust/
__init__/stubs, plus docs (guides/dumping.md) and ADR-021.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ffi/represent.rs |
New descriptors and lowering; error message names nonexistent classes; name_anchors recurses without a stack guard |
src/ffi/mod.rs |
Adds represent= param and the represent branch in dumps |
src/roundtrip/emit.rs |
DumpConfig, emit_roundtrip_dump, key sorting, synthetic-sequence indent |
src/roundtrip/value.rs |
Exposes assigned_string_style as pub(crate) |
src/emit_util.rs |
Shared use_literal_block for fast and represent paths |
src/encode/mod.rs |
Delegates use_literal_block to the shared helper |
src/lib.rs |
Registers the three new pyclasses |
pysrc/yamlrocks/__init__.py / .pyi |
Re-exports and typing for descriptors and represent= |
docs/.../guides/dumping.md |
New represent guide; overstates which options apply on the path |
adr/021-...md, adr/README.md |
Records the design rationale |
tests/features/test_represent.py |
Behavioral coverage for the new protocol |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`dumps` could shape unknown types (`serializers=`, `default=`) but not control how a builtin renders. `represent=` is a callback the emitter invokes for every value it is about to emit, builtins included. It returns a node descriptor (`YAMLRocksScalar`/`YAMLRocksSequence`/`YAMLRocksMapping`) saying how to render that value, with an explicit tag and scalar style, or `None` to defer to the built-in rendering. Descriptors carry the original host objects, not pre-rendered nodes, so the emitter drives the recursion and keeps owning indentation, flow, `sort_keys`, and anchor detection. The path lowers to the round-trip node tree and emits through the round-trip emitter (which already speaks per-node style, tags, and flow) rather than the fast `Value` emitter, so plain `dumps` is untouched. The represent path reproduces PyYAML's dump behavior so a host's representers port verbatim: block sequences indent under their key, `OPT_SORT_KEYS` applies, a standard tag the value already resolves to is elided while a custom tag is kept and force-quoted, multi-line strings default to a `|` literal block, and a shared object emits once with an anchor and aliases the repeats. Deferred values render byte-for-byte like a plain `dumps`. Adds the ADR (021), the descriptor types and stubs, a docs section, and tests.
frenck
force-pushed
the
frenck/represent-protocol
branch
from
1eb0287 to
0c4b6f2
Compare
A value the `represent` callback defers on (`None`) previously went through a narrow node builder that only handled basic builtins, so `default`/`serializers` were ignored and a deferred datetime/dataclass raised where a plain `dumps` succeeds. Route deferred non-container leaves through the same `python_to_value` pipeline a plain `dumps` uses, then bridge the resulting `Value` to a node, so `default`, `serializers`, and datetime/dataclass/numpy handling all compose and deferred output stays byte-for-byte identical to a plain `dumps`. Move `sort_keys` into the lowering (before values are lowered) so it orders keys by the fast path's typed comparator (numbers numerically, not lexically), matching plain `dumps`, and so anchor detection follows the final emission order rather than risking an alias emitted before its anchor.
Address the review of the `represent` deferred path. A value the callback defers on used to go through a narrow node builder, so nested values inside a set/dataclass/enum/numpy value or a `default` result never reached `represent`, `default`/`serializers` were bypassed, and several emit options were dropped. - Every value: a deferred compound decomposes into its child objects, which recurse back through `represent`; only a scalar leaf goes through the shared `python_to_value` pipeline, so `default`/`serializers` and datetime/dataclass/ numpy handling compose and deferred output matches a plain `dumps`. - Aliasing now matches PyYAML's `ignore_aliases` (everything except None/bool/int/float/str/bytes), so a shared set, dataclass, or custom object represented as a collection is deduped, and a cycle through one resolves to an alias instead of the depth error. - `sort_keys` orders keys by the fast path's typed comparator (numbers numerically), derived directly from the key so `default`/`serializers` are not run twice, and with a total order so the sort cannot misbehave. - Descriptor tags are validated with the emit-side tag rules. - A block scalar inside a flow collection is downgraded to a quoted style (block scalars are invalid there). - `OPT_FLOW_STYLE`, `OPT_EXPLICIT_START`, and `OPT_EXPLICIT_END` apply on the represent path. Updates ADR-021, the docs, and the tests to match.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ffi/convert/encode.rs (1)
525-554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate duplication between
numpy_childandnumpy_to_value.
numpy_child(lines 531-554) andnumpy_to_value(lines 564-588) share identicalNUMPY_TYPESlookup andis_instancechecks. If the numpy detection logic changes, it must be updated in two places. Refactornumpy_to_valueto delegate tonumpy_child:♻️ Proposed refactor
fn numpy_to_value( py: Python<'_>, obj: &Bound<'_, PyAny>, ctx: EncodeCtx<'_>, ) -> PyResult<Option<Value<'static>>> { - let types = NUMPY_TYPES.get_or_init(|| { - let numpy = py.import("numpy").ok()?; - let ndarray = numpy.getattr("ndarray").ok()?.unbind(); - let generic = numpy.getattr("generic").ok()?.unbind(); - Some((ndarray, generic)) - }); - let Some((ndarray, generic)) = types else { - return Ok(None); - }; - if !(obj.is_instance(ndarray.bind(py))? || obj.is_instance(generic.bind(py))?) { - return Ok(None); - } - if let Ok(list) = obj.call_method0("tolist") { - return Ok(Some(python_to_value(py, &list, ctx)?)); - } - if let Ok(item) = obj.call_method0("item") { - return Ok(Some(python_to_value(py, &item, ctx)?)); - } - Ok(None) + if let Some(child) = numpy_child(py, obj)? { + return Ok(Some(python_to_value(py, &child, ctx)?)); + } + Ok(None) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b53e5f5-e3e4-47f2-a25a-94f2abbd01c9
📒 Files selected for processing (8)
adr/021-emitter-represent-protocol-for-dumps.mddocs/src/content/docs/guides/dumping.mdsrc/ffi/convert/encode.rssrc/ffi/convert/mod.rssrc/ffi/mod.rssrc/ffi/represent.rssrc/roundtrip/emit.rstests/features/test_represent.py
✅ Files skipped from review due to trivial changes (1)
- docs/src/content/docs/guides/dumping.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ffi/mod.rs
- src/roundtrip/emit.rs
Follow-up review fixes on the `represent` path: - A transparent re-dispatch (an enum's value, a numpy value, or a `default` result) now goes through `lower` at the next depth, so it is depth-bounded and stack-guarded. A `default` that returns its argument resolves to a self-alias instead of recursing into a native stack overflow. - Only an unrecognized-type error falls back to `default`; a genuine encode error (non-UTF-8 bytes, a lone surrogate) propagates unchanged rather than being masked by `default`, matching plain `dumps`. - The round-trip emitter now writes the anchor/tag on a nested block-sequence item, so a shared list nested in a sequence emits `- &id001` before the alias instead of dropping the anchor (also fixes a latent round-trip gap). Documents that an explicit scalar style is honored verbatim (the host picks a representable style) and that a block style inside a flow collection is downgraded to a quoted one.
Further review fixes on the `represent` path: - Never stamp an anchor onto an alias node (that produced the malformed `&id001 *id001`). A value that resolves only to itself (a `default` or serializer returning its own input) now raises cleanly instead. - `tagged_node` lowers its inner through the depth-bounded `lower`, so a serializer that tags its own input aliases and is rejected rather than recursing into a native stack overflow. - Canonical `tag:yaml.org,2002:*` tags are normalized to `!!*` before validation, so a callback ported from a PyYAML representer (which uses the canonical URI tags) passes validation and gets the standard-tag elision. - Every object recorded for aliasing is retained for the whole lowering, so a freed temporary's reused address cannot be misread as an alias to an earlier value. - Integer sort keys keep their exact value (i128), so two large i64 keys no longer collide as they would under f64.
An integer key beyond `i64` was compared with `extract::<f64>()`, which raises `OverflowError` once the value exceeds `f64`'s range, dropping the key to the `Other` rank (sorted after strings). Parse the decimal repr instead, matching the fast path's `compare_keys` (which parses a `BigInt` to `±inf`): the key stays numeric and two that saturate to the same infinity tie on insertion order, so a sorted `represent` dump matches plain `dumps`.
A deferred tagged null now emits as a bare `key: !x` / `- !x` in both mapping and sequence position instead of falling through to `!x null`, which reloads as the string "null" rather than an empty value (the mapping branch also used to drop the tag entirely). This matches the fast path byte-for-byte and is verified against the YAML test suite for round-trip fidelity. A synthetic non-first block-mapping key carrying a tag or anchor now uses the explicit `? key` form. An inline `!tag key:` after a previous entry has its property read as the preceding value's node property, a reparse error when that value was empty, so the `?` indicator opens a fresh key instead, matching the fast emitter. Loaded keys are never synthetic, so their fidelity is untouched. The synthetic AST is dismantled iteratively with `drop_node_tree` (GIL detached) after emission, so a deeply nested represent tree cannot overflow the native stack on its recursive drop, mirroring the loaded-document teardown.
The deferred `represent` path (a callback returning None for every value) is contractually a plain `dumps`, but it emits through the round-trip emitter rather than the fast `Value` encoder, so the two drifted on several shaping and dispatch decisions. An exhaustive parity sweep (a corpus of every scalar type, collection, datetime, dataclass, enum, and tagged value, in every structural position, across the option flags) found six divergence classes; all are fixed: - Block-scalar body indent in a sequence item now lands at `dash + 2` (was `dash + 4`) by making the round-trip emitter take an absolute body column instead of always adding a step, matching the fast encoder. - A multi-line string mapping key is emitted inline double-quoted, and a collection key inline as a flow collection, rather than an explicit `?` block key (which the fast path has no equivalent of). - `OPT_INDENT_2/4` and `OPT_INDENTLESS_SEQUENCES` are honored on the dump path. - A datetime-like object (anything exposing `isoformat()`) is dispatched before Enum/dataclass, matching the fast path's `special_type_to_value` order. - A non-primitive mapping key (datetime/UUID/Path) sorts under `OPT_SORT_KEYS` by the scalar the fast path renders, not dropped to input order. Aliasing stays the one accepted divergence: `represent` keeps PyYAML-style anchors for shared objects while plain `dumps` never aliases (ADR-021). A parametrized parity test now sweeps the corpus and asserts deferred output equals plain `dumps` byte-for-byte, so the two paths cannot silently drift again. The deep-nesting represent test is bounded to a depth safely under the interpreter's C recursion budget (the callback runs at every level).
…tale indent docs Sorting a mapping under OPT_SORT_KEYS converted each non-primitive key through the pipeline with serializers still active, so a registered serializer ran once during the sort and again when the key was lowered, and a real conversion error was swallowed as an 'other' key. The sort conversion now disables both `default` and `serializers`: a key only those could render is not a plain scalar and ranks in input order (as `compare_keys` ranks the tagged node it lowers to), a datetime/UUID/Path key still sorts by its rendered string, and a genuine error (a non-UTF-8 bytes key) propagates instead of being swallowed. Also correct the docs, ADR-021, and emitter comments: OPT_INDENT_4 and OPT_INDENTLESS_SEQUENCES do apply on the represent path (they were wired into the dump config in this PR); only line width remains unimplemented there.
…e, int subclass - Dismantle the temporary Value built to rank a non-primitive sort key with drop_value_tree, so a deeply nested key cannot overflow the stack on its recursive drop. - Rank a key the built-in conversion cannot render as Other (input order) rather than raising during the sort pass: represent runs first when the key is lowered and may rescue it (a non-UTF-8 bytes key, a custom object), and if it does not, lowering still raises. Refines the previous error-propagation, which raised before represent got its documented first look at the key. - Reduce a beyond-i64 int-subclass key through a base int before parsing its f64 sort rank, so an overridden __str__ cannot make it sort as NaN while plain dumps sorts by the real numeric value.
…positions An adversarial pass over the diff plus a widened parity sweep (now covering tagged values) surfaced several byte-for-byte gaps for tagged values, all fixed against the fast encoder: - A tagged block collection at the document root indents its body one step under the tag (a tagged root sequence stays flush under OPT_INDENTLESS_SEQUENCES), instead of emitting the body flush against the tag line. - A tagged empty null at the root keeps just its tag (`!t`) rather than `!t null`, which would reload as the string "null". - A tagged block mapping as a sequence item indents by the configured step; a tagged nested sequence keeps the fixed two-column offset. - A tagged sequence in mapping-value position always indents under its tag, even in indentless mode, so the tag binds to it on reload. Also reject `width` together with `represent` explicitly (line folding is not implemented on that path) rather than silently ignoring it, and record that in the docs and ADR. The parity sweep now exercises tagged values across positions and options; the two accepted edges (a shared tagged value cannot alias with its tag; a tagged value as a mapping key renders as a valid explicit key) are skipped with comments.
| // transparently to a child (enum value, numpy, a `default`/serializer | ||
| // result), returning the child's node, which already carries the child's | ||
| // anchor or is an alias. | ||
| let delegated = matches!(node.kind, YamlNodeKind::Alias(_)) || node.anchor.is_some(); |
Comment on lines
+51
to
+52
| it. The fast emitter cannot emit a literal `|` block at all, has no per-node style | ||
| or per-node flow, and always shows a tag; adding all of that to the hot emitter is |
…lock styles - If lowering a later mapping entry or sequence item fails after an earlier deeply nested one was already built, dismantle the accumulated nodes iteratively (drop_node_pairs / drop_node_tree) before propagating the error, so their derived recursive Drop cannot overflow the native stack. Mirrors the success-path teardown. - Reject an explicit literal/folded scalar style that cannot round-trip the value instead of silently emitting lossy YAML: folding rewrites interior line breaks, a literal auto-detects indentation from a leading-whitespace first line, and neither can hold a carriage return or non-printable control. A lossless literal (single or multi-line, no leading whitespace) still works, so the !lambda literal case is unaffected. A flow context still downgrades block styles to double-quoted as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking change
None.
represent=is a new optional keyword; omit it anddumpsbehaves exactly as before, on the same fast path. All existing options,default=, andserializers=are unchanged.Proposed change
dumpscould shape unknown types (serializers=maps a type to a custom!tag,default=fires for types nothing else handles), but it could not control how a builtin renders: mask astr, format afloata specific way, force a scalar's style, or emit a specific tag. PyYAML can, because its representers dispatch on any type and return a node with an explicit tag and style. That gap is the one thing keepingimport yamlin downstreams that ship a hand-rolledDumper(the driver is ESPHome, whose parser already moved to yamlrocks).This adds a
represent=callback the emitter invokes for every value it is about to emit, builtins included. It returns one of three node descriptors, orNoneto defer to the built-in rendering:The descriptors carry the original host objects, not pre-rendered nodes, so the emitter drives the recursion and keeps owning indentation, flow,
sort_keys, and anchor detection; the host only ever describes one level. The path lowers to the round-trip node tree and emits through the round-trip emitter (which already speaks per-node style, tags, and flow) rather than the fastValueemitter, so plaindumpsis untouched (seeadr/021).The represent path reproduces PyYAML's dump behavior so a host's representers port verbatim, with auto style and their existing tags:
OPT_SORT_KEYSapplies,!!bool,!!float,!!str) is elided, while a custom tag (!extend) is kept and force-quoted (a plain form would lose the tag),|literal block,Deferred values (where
representreturnsNone) render byte-for-byte like a plaindumps.Line-wrap (
width) composition on the represent path is intentionally deferred; it is the lowest-priority item for the reference consumer.Type of change
Additional information
guides/dumping.md)Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.