Skip to content

fix: resolve RFC 9557 compliance findings from expert review - #26

Merged
8beeeaaat merged 4 commits into
mainfrom
fix/rfc9557-review-findings
Jul 7, 2026
Merged

fix: resolve RFC 9557 compliance findings from expert review#26
8beeeaaat merged 4 commits into
mainfrom
fix/rfc9557-review-findings

Conversation

@8beeeaaat

Copy link
Copy Markdown
Owner

Summary

Fixes all findings from the v0.4.0 pre-release RFC 9557 compliance review (blocking PR #25):

Finding Fix
Major-1: Format emitted offset zones as [+0900] (RFC grammar violation, round-trip loss) Keep +09:00 as the FixedZone name (RFC 9557 §1.2 serialization); formatOffsetName removed
Major-2: duplicate critical suffix keys not rejected (§3.3 MUST) Duplicate key errors with ErrCriticalExtension when either occurrence is critical
Major-3: unknown critical keys accepted even in strict mode Strict mode rejects critical keys other than u-ca; non-strict delegation via Critical map documented in README
Minor-4: second time-zone annotation slipped through after an ignored unknown zone Parse state (seenTimezone) decoupled from ext.Location
Minor-5: suffix = [time-zone] *suffix-tag ordering unenforced Time-zone annotation after a suffix tag → ErrInvalidSuffix in both modes
Minor-6: Format rejected CriticalLocation even when the timestamp's own zone would be emitted Critical flag now applies to the fallback zone; error only when no zone is available
Code quality: ParseError lacked Unwrap Added; sentinel errors now match with errors.Is

Spec-conformance tests added

  • RFC §3.3 example strings: [!u-ca=chinese][u-ca=japanese], [u-ca=chinese][!u-ca=japanese], [!knort=blargel]
  • RFC §1.2 offset example: 2022-07-08T00:14:07+08:45[+08:45]
  • Parse → Format → Parse round trips for offset annotations incl. critical variant
  • Grammar-ordering and ignored-zone duplicate cases
  • errors.Is matching through ParseError.Unwrap

Verification

  • go test -race ./... passes (all packages)
  • golangci-lint run: 0 issues
  • go vet, gofmt: clean

🤖 Generated with Claude Code

Address all findings from the v0.4.0 pre-release spec review:

- Serialize offset time zones in the RFC 3339 form: keep "+09:00" as
  the FixedZone name instead of Go's "+0900" convention, so
  Parse -> Format -> Parse round-trips offset annotations per
  RFC 9557 Section 1.2 and the Section 4.1 time-numoffset grammar.
  Previously Format emitted "[+0900]", which strict re-parsing
  rejected and non-strict re-parsing silently dropped.
- Reject duplicate suffix keys when either occurrence is critical
  (ErrCriticalExtension) per the Section 3.3 MUST; elective duplicates
  still keep the first occurrence. Previously the second occurrence
  was silently ignored and its "!" flag lost.
- Reject unrecognized critical suffix keys (only "u-ca" is
  understood) in strict mode per Section 3.3; non-strict mode keeps
  delegating to the caller via the Critical map, now documented in
  the README.
- Enforce the Section 4.1 grammar "suffix = [time-zone] *suffix-tag":
  a second time-zone annotation is rejected even when the first was
  an unknown zone ignored by a non-strict parse, and a time-zone
  annotation after a suffix tag is rejected in both modes. Tracking
  moved from ext.Location (nil for ignored zones) to explicit parse
  state.
- Apply the critical "!" flag to the fallback zone taken from the
  timestamp when ext.Location is unset, instead of rejecting; Format
  still errors when no zone is available at all.
- Add ParseError.Unwrap so sentinel errors match with errors.Is.

Tests now cover the RFC's own Section 3.3 example strings
([!u-ca=chinese][u-ca=japanese], [u-ca=chinese][!u-ca=japanese],
[!knort=blargel]), the Section 1.2 offset example (+08:45[+08:45]),
and offset-annotation round trips including the critical variant.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.45%. Comparing base (56017bb) to head (ebd7fe8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ixdtf.go 95.45% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
- Coverage   94.76%   94.45%   -0.31%     
==========================================
  Files           1        1              
  Lines         382      397      +15     
==========================================
+ Hits          362      375      +13     
- Misses         12       13       +1     
- Partials        8        9       +1     

☔ 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.

@8beeeaaat

Copy link
Copy Markdown
Owner Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 addresses RFC 9557 compliance findings for IXDTF parsing/formatting, including stricter grammar enforcement for suffix ordering/duplication rules, correct offset time-zone serialization, and improved error interoperability via ParseError.Unwrap.

Changes:

  • Fix offset time-zone annotation round-tripping by preserving RFC 3339 +HH:MM form as the FixedZone name and updating offset-name detection accordingly.
  • Enforce RFC 9557 §4.1 suffix grammar (single time-zone annotation, must precede tags) and RFC 9557 §3.3 handling for critical duplicate/unknown keys.
  • Add spec-conformance tests (round trips, ordering violations, errors.Is via ParseError.Unwrap) and document strict vs non-strict critical-tag responsibilities in the README.

Reviewed changes

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

File Description
README.md Documents strict/non-strict responsibility for unknown critical tags and clarifies offset-annotation behavior and suffix ordering constraints.
ixdtf.go Implements RFC 9557 compliance fixes: offset zone naming, stricter suffix parsing state, critical-key duplicate/unknown handling, and ParseError.Unwrap.
ixdtf_test.go Adds/updates tests for offset annotation round-trips, suffix ordering errors, critical duplicates/unknown critical keys, and errors.Is behavior.
ixdtf_internal_test.go Removes tests tied to the deleted formatOffsetName and updates offset-name detection expectations and parser helper callsites.

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

Comment thread ixdtf.go
Comment thread ixdtf_test.go
Comment thread ixdtf.go Outdated
8beeeaaat and others added 3 commits July 7, 2026 10:24
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@8beeeaaat
8beeeaaat merged commit c69b6ca into main Jul 7, 2026
8 checks passed
@8beeeaaat
8beeeaaat deleted the fix/rfc9557-review-findings branch July 7, 2026 04:27
8beeeaaat added a commit that referenced this pull request Jul 7, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@8beeeaaat 8beeeaaat mentioned this pull request Jul 7, 2026
8beeeaaat added a commit that referenced this pull request Jul 7, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants