Skip to content

🐛 Keep leading-zero sexagesimal values as strings under YAML 1.1#174

Merged
frenck merged 1 commit into
mainfrom
frenck/resolver-sexagesimal-leading-zero
Jul 2, 2026
Merged

🐛 Keep leading-zero sexagesimal values as strings under YAML 1.1#174
frenck merged 1 commit into
mainfrom
frenck/resolver-sexagesimal-leading-zero

Conversation

@frenck

@frenck frenck commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Breaking change

Under OPT_YAML_1_1 and OPT_PYYAML_COMPAT, a leading-zero sexagesimal value like 03:30 now resolves to the string "03:30" instead of the base-60 integer 210. This matches PyYAML and ruamel. Anyone who (incorrectly) relied on 03:30 becoming 210 will see the string instead, but that was silent data corruption of a common time-of-day value, so the change is a fix. The default YAML 1.2 schema never resolved sexagesimal and is unaffected.

Proposed change

The YAML 1.1 integer sexagesimal production is [-+]?[1-9][0-9_]*(:[0-5]?[0-9])+: the leading segment must start 1-9, and every later segment is one or two base-60 digits. parse_sexagesimal_int only rejected a per-segment sign, so 03:30 parsed as 210, and 1:030 as 60. A time of day in a Home Assistant / ESPHome / Ansible config silently became an integer.

This enforces the spec production: a leading-zero first segment (03:30, 0:30, 00:00, 023:30) and an oversized later segment (1:030) now stay strings. Genuine base-60 values (1:30, 90:00, 1:2:3) still resolve. A shared 1.1 dump test that used 01:02:03 as a sexagesimal example now uses 10:20:30, since 01:02:03 is correctly a string.

Type of change

  • Dependency or tooling upgrade
  • Bugfix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Deprecation (replaces or removes a feature, with a migration path)
  • Breaking change (a fix or feature that changes existing behavior)
  • Code quality, refactor, or test-only change
  • Documentation only

Additional information

  • This PR fixes or closes issue: None
  • This PR is related to: None
  • Link to a separate documentation pull request: None

Checklist

  • I have read the AI Policy, and this pull request was not created by an autonomous agent.
  • I fully understand the code in this pull request and can explain every line, including any AI-assisted changes.
  • The change is covered by tests, and uv run pytest passes locally. A pull request cannot be merged unless CI is green.
  • uv run ruff check . and uv run ruff format --check . pass.
  • cargo fmt --check and cargo clippy --all-targets -- -D warnings pass.
  • Round-trip fidelity is preserved: an unmodified document still re-emits byte-for-byte.
  • No commented-out or dead code is left in the pull request.

Copilot AI review requested due to automatic review settings July 2, 2026 22:33
@frenck frenck added the bugfix Inconsistencies or issues which will cause a problem for users or implementers. label Jul 2, 2026
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@frenck, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 80dc5d2b-5644-4a5b-96dc-1eceb3611cc2

📥 Commits

Reviewing files that changed from the base of the PR and between e7132bc and 5347b09.

📒 Files selected for processing (6)
  • docs/src/content/docs/reference/options.md
  • src/encode/mod.rs
  • src/resolver/yaml11.rs
  • tests/core/test_dumps.py
  • tests/core/test_yaml_1_1.py
  • tests/roundtrip/test_edge_cases.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 16 untouched benchmarks


Comparing frenck/resolver-sexagesimal-leading-zero (5347b09) with main (e7132bc)

Open in CodSpeed

@frenck frenck force-pushed the frenck/resolver-sexagesimal-leading-zero branch from df23bb3 to a72cb04 Compare July 2, 2026 22:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes YAML 1.1 scalar resolution so a leading-zero sexagesimal value such as 03:30 (a common time-of-day) stays the string "03:30" instead of being silently converted to the base-60 integer 210. It enforces the YAML 1.1 integer sexagesimal production [-+]?[1-9][0-9_]*(:[0-5]?[0-9])+ in parse_sexagesimal_int, rejecting a leading-zero first segment and an oversized/out-of-range later segment. This aligns YAMLRocks with PyYAML and ruamel and affects both OPT_YAML_1_1 and OPT_PYYAML_COMPAT (float sexagesimal parsing is intentionally left untouched, since its spec production allows a leading zero).

Changes:

  • Enforce the leading [1-9] first-segment and [0-5]?[0-9] later-segment rules in parse_sexagesimal_int, keeping leading-zero/oversized forms as strings.
  • Update the Rust emitter unit test to use a genuine sexagesimal value (10:20:30) since 01:02:03 is now a string.
  • Add Python coverage for leading-zero and out-of-range sexagesimal values staying strings under both 1.1 schemas.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/resolver/yaml11.rs Core fix: enforce the YAML 1.1 int sexagesimal production so leading-zero/oversized segments resolve as strings; updated unit-test expectations.
src/encode/mod.rs Updated emitter test values from 01:02:03 to 10:20:30; a nearby explanatory comment still references the old value.
tests/core/test_yaml_1_1.py Adds parametrized coverage for leading-zero and out-of-range sexagesimal values staying strings under OPT_YAML_1_1 and OPT_PYYAML_COMPAT.

The main concern is that the behavior change also affects emitter quoting (resolves_to_number), so existing assertions in tests/core/test_dumps.py, tests/roundtrip/test_edge_cases.py, and the verified doc example in docs/reference/options.md that treat 01:02:03 as a quoted 1.1 sexagesimal number are now incorrect and would fail CI, but were not updated here.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/resolver/yaml11.rs
Comment thread src/encode/mod.rs Outdated
Copilot AI review requested due to automatic review settings July 2, 2026 22:37
@frenck frenck force-pushed the frenck/resolver-sexagesimal-leading-zero branch from a72cb04 to 3918c95 Compare July 2, 2026 22:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread src/resolver/yaml11.rs
Comment thread src/encode/mod.rs Outdated
Copilot AI review requested due to automatic review settings July 2, 2026 22:48
@frenck frenck force-pushed the frenck/resolver-sexagesimal-leading-zero branch from 3918c95 to 0ef56d2 Compare July 2, 2026 23:00
The YAML 1.1 integer sexagesimal production is `[-+]?[1-9][0-9_]*(:[0-5]?[0-9])+`:
the leading segment must start 1-9, and every later segment is one or two
base-60 digits. `parse_sexagesimal_int` only rejected a per-segment sign, so a
time of day like 03:30 parsed as the base-60 integer 210 under OPT_YAML_1_1 and
OPT_PYYAML_COMPAT, silently corrupting a common Home Assistant/ESPHome/Ansible
value. PyYAML and ruamel keep it a string.

Enforce the spec production: reject a leading-zero first segment (03:30, 0:30,
00:00, 023:30) and an oversized later segment (1:030), so all stay strings.
1:30, 90:00, and 1:2:3 still resolve as base-60. A shared dump test that used
01:02:03 as a sexagesimal example now uses 10:20:30, since 01:02:03 is
correctly a string.
@frenck frenck force-pushed the frenck/resolver-sexagesimal-leading-zero branch from 0ef56d2 to 5347b09 Compare July 2, 2026 23:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/encode/mod.rs Outdated
Copilot AI review requested due to automatic review settings July 2, 2026 23:08
@codecov-commenter

codecov-commenter commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.53%. Comparing base (98592c0) to head (5347b09).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   92.37%   92.53%   +0.15%     
==========================================
  Files          40       40              
  Lines       11201    11441     +240     
==========================================
+ Hits        10347    10587     +240     
  Misses        854      854              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@frenck frenck merged commit d1b2b48 into main Jul 2, 2026
74 checks passed
@frenck frenck deleted the frenck/resolver-sexagesimal-leading-zero branch July 2, 2026 23:29
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix Inconsistencies or issues which will cause a problem for users or implementers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants