✨ Resolve timestamps in round-trip mode under OPT_TIMESTAMPS#218
Conversation
📝 WalkthroughWalkthroughThis PR introduces a Sequence Diagram(s)sequenceDiagram
participant Caller
participant loads
participant loads_roundtrip
participant YAMLRocksDocument
participant node_to_python_with
Caller->>loads: loads(data, OPT_ROUND_TRIP | OPT_TIMESTAMPS)
loads->>loads_roundtrip: resolve_timestamps flag
loads_roundtrip->>YAMLRocksDocument: with_resolve_timestamps(flag)
Caller->>YAMLRocksDocument: to_dict() / __getitem__ / walk()
YAMLRocksDocument->>node_to_python_with: convert(node, resolve_timestamps)
node_to_python_with-->>YAMLRocksDocument: date/datetime or raw value
YAMLRocksDocument-->>Caller: resolved result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #216 that extends the standalone OPT_TIMESTAMPS flag so it also resolves plain, untagged timestamp scalars to Python date/datetime objects in round-trip mode, not just on the fast path. Previously round-trip timestamp typing was schema-driven (active only under OPT_PYYAML_COMPAT); now the fast and round-trip paths agree, while emission stays untouched so unmodified documents still re-emit byte-for-byte. A resolve_timestamps boolean is threaded through the round-trip AST read conversion and stored on YAMLRocksDocument, derived from the load options. Annotated mode deliberately remains schema-driven.
Changes:
- Threads a
resolve_timestampsflag throughnode_to_python_*/ document read helpers (to_dict, indexing,walk,keys,len, key matching), stored onYAMLRocksDocumentand set from load options. - Switches the round-trip
scalar_to_pygate fromschema == Schema::Yaml11PyYamlto the new flag; annotated-mode call sites explicitly passschema == Schema::Yaml11PyYamlto keep their schema-driven behavior. - Updates the timestamps test and the options reference documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/roundtrip/value.rs |
Adds resolve_timestamps parameter through the conversion helpers and gates timestamp resolution on it. |
src/roundtrip/document.rs |
Stores the flag on YAMLRocksDocument, adds with_resolve_timestamps, and threads it through every read helper and both view/node accessors. |
src/ffi/mod.rs |
Passes the derived resolve_timestamps into loads_roundtrip and sets it on the document. |
src/ffi/convert/annotate.rs |
Preserves annotated-mode schema-driven resolution by passing schema == Schema::Yaml11PyYaml. |
docs/src/content/docs/reference/options.md |
Updates prose to describe round-trip resolution (summary table row still stale — see comment). |
tests/features/test_timestamps.py |
Replaces the "keeps strings" test with one asserting round-trip resolution under the standalone flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| the other 1.1 footguns. It applies to `loads`/`load` and to round-trip mode, so | ||
| `to_dict()`, indexing, `walk()`, and `keys()` on a `YAMLRocksDocument` all resolve | ||
| timestamps too (the source still re-emits byte-for-byte). In annotated mode, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
==========================================
- Coverage 92.53% 92.51% -0.03%
==========================================
Files 41 41
Lines 12520 12720 +200
==========================================
+ Hits 11586 11768 +182
- Misses 934 952 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/features/test_timestamps.py (1)
118-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring spans two lines; guideline calls for one-line docstrings.
The docstring wraps across two physical lines rather than fitting on one. As per coding guidelines, "Every Python test has a one-line docstring describing what it verifies."
✏️ Proposed fix
def test_round_trip_with_standalone_flag_resolves(): - """The standalone OPT_TIMESTAMPS flag also resolves in round-trip mode, across - to_dict, indexing, and walk, while the source re-emits byte-for-byte.""" + """The standalone OPT_TIMESTAMPS flag also resolves timestamps in round-trip mode."""Aside from the docstring formatting, the assertions correctly exercise
to_dict(), indexing,walk(), and byte-for-byteto_yaml()re-emission, consistent with theresolve_timestampswiring insrc/roundtrip/document.rs.Source: Path instructions
src/roundtrip/document.rs (1)
166-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding targeted tests for the new timestamp-aware mutation/key paths.
Codecov reports 19 missing lines / 88.69% patch coverage concentrated in this file. The
resolve_timestampsthreading throughset_child/del_child/distinct_keys(key matching against a resolved date/datetime, duplicate-key dedup under timestamp resolution) is exactly the kind of edge-case logic worth covering explicitly — e.g.doc[date(2024,1,15)] = ...ordel doc[...]against a plain timestamp key whenOPT_TIMESTAMPSis set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3f65472-796c-49d7-a13d-f4035680d962
📒 Files selected for processing (6)
docs/src/content/docs/reference/options.mdsrc/ffi/convert/annotate.rssrc/ffi/mod.rssrc/roundtrip/document.rssrc/roundtrip/value.rstests/features/test_timestamps.py
Breaking change
None. Extends an existing opt-in flag to another mode; default behavior is unchanged.
Proposed change
Follow-up to #216: the standalone
OPT_TIMESTAMPSflag now resolves timestamps in round-trip mode as well as on the fast path.to_dict(), indexing,walk(), andkeys()on aYAMLRocksDocumentreturn adate/datetimewhen the flag (orOPT_PYYAML_COMPAT, which implies it) is set, while the source still re-emits byte-for-byte.Before this, round-trip typed-timestamp resolution followed the schema, so it was active only under
OPT_PYYAML_COMPAT; the standalone flag was a fast-path feature. Now the two paths agree.resolve_timestampsflag through the round-trip AST read conversion (node_to_python_*and the document read helpers), stored on theYAMLRocksDocumentand set from the load options. Only plain, untagged scalars resolve, and a quoted"2024-01-15"stays a string, matching the fast path.OPT_ANNOTATED) deliberately stays schema-driven: it resolves timestamps underOPT_PYYAML_COMPAT(to a plaindate/datetime, no source location) but not from the standalone flag. Documented in the options reference.Type of change
Additional information
OPT_TIMESTAMPSfeature)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.