Skip to content

Better handling of date-time and other value classes - #1439

Merged
VisLab merged 3 commits into
hed-standard:mainfrom
VisLab:validation_cleanup
Oct 1, 2026
Merged

VisLab merged 3 commits into
hed-standard:mainfrom
VisLab:validation_cleanup

Conversation

@VisLab

@VisLab VisLab commented Oct 1, 2026

Copy link
Copy Markdown
Member

No description provided.

VisLab added 2 commits October 1, 2026 06:15
dateTimeClass values are matched with the BIDS Datetime regex (RFC 3339
with an optional offset), the rule hed-specification publishes in
character_sets.json: fractions of one to six digits, hour 00-23, seconds
00-60, offset Z or +hh:mm. Hour 24, longer fractions and out-of-range
fields no longer pass. numericClass uses ASCII digits only.

class_util.py loses the second, unreachable date-time rule
(fromisoformat, rejecting any zone) and everything that existed only to
reach it: the per-class validator map, the value_validators parameter,
the is_* helpers, the clock-face check and find_invalid_positions. The
tests of those functions become whole-value tests.
hed-tests PR hed-standard#67: the value-invalid-date-time-format case.

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.

Copilot review overview

🟡 Changes recommended

The date-time regex accepts impossible dates, and the constructor change breaks a documented extension API.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

This PR centralizes value-class validation around regex rules and tightens date-time and numeric formats.

Changes:

  • Adds stricter RFC 3339-style date-time validation.
  • Restricts numeric values to ASCII digits.
  • Removes legacy per-class validation functions.
File Description
hed/​validator/​data/​class_regex.json Tightens whole-value regex rules.
hed/​validator/​util/​class_util.py Consolidates validation through CharRexValidator.
tests/​validator/​test_tag_validator_util.py Adds date-time and numeric rule tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread hed/validator/data/class_regex.json
Comment thread hed/validator/util/class_util.py Outdated
Copilot review of PR hed-standard#1439: UnitValueValidator(value_validators=...)
was a documented parameter, so it stays accepted until 2.0.0 with a
DeprecationWarning. It has no effect, as before in practice: the only
code that consulted the injected functions was never reached by
validation. Registered in tests/test_deprecations.py with the other
2.0.0 removals.

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.

Copilot review overview

🔵 Needs a closer look

A deprecated helper is removed without the repository's compatibility and scheduled-removal handling.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Retain deprecated is_clock_face_time wrapper and schedule its removal

hed/​validator/​util/​class_util.py:28

This compatibility shim leaves another removed API behind: is_clock_face_time was explicitly marked deprecated, but this PR deletes it without retaining a warning wrapper or adding it to SCHEDULED_REMOVALS, whose stated policy is to track every deprecated item. Direct callers now fail immediately while the similarly deprecated constructor parameter remains available until 2.0.0. Keep the helper as a warning wrapper and schedule its removal for the same major release.

@VisLab
VisLab merged commit 7995d9d into hed-standard:main Oct 1, 2026
18 checks passed
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