Refactor validator return type to support warnings via ValidationResult wrapper (#1479) by sejalpunwatkar · Pull Request #1480 · hdmf-dev/hdmf

sejalpunwatkar · 2026-05-27T18:14:01Z

Motivation

Addresses #1479

This PR implements Phase 1 (the foundational plumbing) to introduce ValidationWarning infrastructure without breaking the existing error reporting model. This change ensures that downstream packages (like PyNWB and NWB Inspector) can eventually consume warning-level messages without modifying their immediate logic or incorrectly marking files as invalid.

How to test the behavior?

Added Core Types: Introduced ValidationWarning class as a sibling to Error inside errors.py, preserving the matching internal shape (.name, .reason, .location).
Built the Smart Container: Implemented the ValidationResult wrapper class in errors.py with custom Python overrides (__bool__, __len__, __iter__, __getitem__) pointing explicitly to the errors bucket to ensure backward compatibility.
Wired the Plumbing: Updated ValidatorMap.validate() inside validator.py to capture the errors_list, wrap it into the new ValidationResult container with an empty warnings container (warnings=[]), and adjusted its strict @docval return type check to rtype=(list, ValidationResult).

Checklist

Did you update CHANGELOG.md with your changes?
Does the PR clearly describe the problem and the solution?
Have you reviewed our Contributing Guide?
Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

…lt wrapper (hdmf-dev#1479)

for more information, see https://pre-commit.ci

codecov · 2026-05-27T18:17:07Z

Codecov Report

❌ Patch coverage is 70.37037% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.09%. Comparing base (29fd56d) to head (c0b0464).

Files with missing lines	Patch %	Lines
src/hdmf/validate/errors.py	68.00%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1480      +/-   ##
==========================================
- Coverage   93.21%   93.09%   -0.13%     
==========================================
  Files          41       41              
  Lines       10182    10233      +51     
  Branches     2104     2106       +2     
==========================================
+ Hits         9491     9526      +35     
- Misses        414      430      +16     
  Partials      277      277

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

rly · 2026-05-30T14:27:19Z

Thanks! Looks good overall. Could you please add some basic tests of ValidationResult:

validate() returns a ValidationResult with .warnings == [].
Backward compatibility: bool, len, iteration, indexing all reflect errors only.
Construct ValidationResult(warnings=[w]) and assert the warning survives.

Could you please also add a changelog entry that points to this PR?

rly · 2026-06-02T18:45:13Z

@sejalpunwatkar Thanks for your work on this. Let me know when it is ready for re-review.

sejalpunwatkar · 2026-06-06T09:12:26Z

Hi @rly, the PR is now ready for your re-review!

I have fully addressed your feedback from the initial code review:

Bug Fix: Fixed the initialization logic typo in ValidationResult.__init__ (warnings vs errors).
Ecosystem Safety: Added the new classes to __all__ in errors.py.
Type-Checking & Docs: Tightened @docval in validator.py to strictly enforce returning ValidationResult, and updated the corresponding docstrings and imports in src/hdmf/common/__init__.py.
Enhanced Diagnostics: Added a human-readable __repr__ method to ValidationResult.
Testing & Coverage: Added comprehensive unit tests at the bottom of test_validate.py covering the wrapper construction, magic methods, and empty state evaluations. 100% of the test suite is passing cleanly.
Changelog: Added a release tracking entry under the ## HDMF 5.0.0 (Upcoming) section in CHANGELOG.md.

rly · 2026-06-13T23:01:46Z

+    Returns:
+        ValidationResult: A ValidationResult object containing the errors and warnings found.


It's unconventional, but the return docstring is encoded in the returns argument in the @docval above, so this should be removed here.

Suggested change

Returns:

ValidationResult: A ValidationResult object containing the errors and warnings found.

rly · 2026-06-13T23:03:07Z

 - Deprecated `BaseStorageSpec.add_attribute`, `GroupSpec.add_group`, `GroupSpec.add_dataset`, and `GroupSpec.add_link`. Use `set_attribute`, `set_group`, `set_dataset`, and `set_link` instead. @rly [#1333](https://github.com/hdmf-dev/hdmf/pull/1333)
 - Deprecated unused `BaseStorageSpec.get_data_type_spec` and `BaseStorageSpec.get_namespace_spec`. @rly [#1333](https://github.com/hdmf-dev/hdmf/pull/1333)
 - Moved `test`, `docs`, and `min-reqs` from `[project.optional-dependencies]` to `[dependency-groups]` (PEP 735). `min-reqs` was renamed to `test-min-deps`. @rly [#1395](https://github.com/hdmf-dev/hdmf/pull/1395)
+- Refactored validator return type to `ValidationResult` to support upcoming validation warnings. @sejalpunwatkar [#1480](https://github.com/hdmf-dev/hdmf/pull/1480)


The changelog has been updated since you started this work. Please add this entry to a new section at the top of the file

## HDMF 6.1.0 (Upcoming)

rly · 2026-06-13T23:04:13Z

        self.assertIsInstance(results[0], DtypeError)
        self.assertEqual("Foo/data (my_foo/data): incorrect type - expected 'bytes', got 'utf'", str(results[0]))

+


This test class was inserted in the middle of TestVlenStringData so the tests after your additions here have been added to TestValidationResultWrapper which is not correct.

rly · 2026-06-13T23:04:47Z

        self.assertEqual("Foo/data (my_foo/data): incorrect type - expected 'bytes', got 'utf'", str(results[0]))

+
+class TestValidationResultWrapper:


Suggested change

class TestValidationResultWrapper:

class TestValidationResultWrapper(TestCase):

By convention, all test classes should subclass hdmf.testing.TestCase

rly · 2026-06-13T23:05:03Z

+        assert empty_result.warnings == []
+
+    def test_validate_method_returns_empty_warnings(self):
+        pass


This should be implemented

rly · 2026-06-13T23:09:29Z

 from hdmf.validate import ValidatorMap
 from hdmf.validate.errors import (DtypeError, MissingError, ExpectedArrayError, MissingDataType,
-                                  IncorrectQuantityError, IllegalLinkError, ShapeError, IncorrectDataType)
+                                  IncorrectQuantityError, IllegalLinkError, ShapeError,IncorrectDataType,


Suggested change

IncorrectQuantityError, IllegalLinkError, ShapeError,IncorrectDataType,

IncorrectQuantityError, IllegalLinkError, ShapeError, IncorrectDataType,

rly · 2026-06-13T23:14:24Z

        return hash(self) == hash(other)


+class ValidationWarning:


Since ValidationWarning is almost entirely identical to Error, it would be better to create a shared base class ValidationIssue that both extend from and that contains all the shared code. Downstream code will still be able to differentiate between Error and ValidationWarning because they are different classes.

One minor thing is that because __eq__ is defined based on the hash and I believe the hash of a ValidationWarning and the hash of an identical Error object will be the same, then Error("name", "reason") == ValidationWarning("name", "reason") will be True. This might become an issue if/when errors and warnings are placed in a dict together.

To address this, I would change __eq__ to:

def __eq__(self, other): return type(self) is type(other) and hash(self) == hash(other)

rly · 2026-06-13T23:15:12Z

Thanks @sejalpunwatkar . This is looking much better. I have a few more comments and suggestions above

for more information, see https://pre-commit.ci

sejalpunwatkar · 2026-06-17T15:36:48Z

Hi @rly, I've addressed all your feedback: fixed the changelog section, updated the docstrings, corrected the test indentation, implemented the missing test, and updated the ValidationWarning.eq check. Ready for review!

sejalpunwatkar and others added 2 commits May 27, 2026 18:08

Refactor validator return type to support warnings via ValidationResu…

885f7a1

…lt wrapper (hdmf-dev#1479)

[pre-commit.ci] auto fixes from pre-commit.com hooks

8e46bd4

for more information, see https://pre-commit.ci

rly reviewed May 30, 2026

View reviewed changes

Comment thread src/hdmf/validate/errors.py Outdated

rly reviewed May 30, 2026

View reviewed changes

Comment thread src/hdmf/validate/errors.py

rly reviewed May 30, 2026

View reviewed changes

Comment thread src/hdmf/validate/validator.py Outdated

Merge branch 'dev' into feature/1479-validation-result

3ee3ea3

rly reviewed May 30, 2026

View reviewed changes

Comment thread src/hdmf/validate/errors.py

sejalpunwatkar added 3 commits June 2, 2026 10:38

Update docstring and import for ValidationResult in common init

c1225d4

Resolve merge conflicts and clean up validator tests

39499cf

style: resolve ruff line length check on test imports

669261e

sejalpunwatkar force-pushed the feature/1479-validation-result branch 2 times, most recently from 3a072bb to 669261e Compare June 2, 2026 11:09

style: fix ruff F811 duplicate import redefinition error

88bb8e9

sejalpunwatkar force-pushed the feature/1479-validation-result branch from 9d3ea64 to 88bb8e9 Compare June 2, 2026 11:13

rly reviewed Jun 13, 2026

View reviewed changes

Comment thread src/hdmf/common/__init__.py

rly reviewed Jun 13, 2026

View reviewed changes

Fix PR feedback, update ValidationWarning equality, and test structure

964acbf

sejalpunwatkar force-pushed the feature/1479-validation-result branch from 5e33150 to 964acbf Compare June 17, 2026 11:12

Merge branch 'dev' into feature/1479-validation-result

6670324

pre-commit-ci Bot and others added 2 commits June 17, 2026 11:15

[pre-commit.ci] auto fixes from pre-commit.com hooks

6b88ba9

for more information, see https://pre-commit.ci

Add data type attribute to test builder setup

c0b0464

		Returns:
		ValidationResult: A ValidationResult object containing the errors and warnings found.

		self.assertIsInstance(results[0], DtypeError)
		self.assertEqual("Foo/data (my_foo/data): incorrect type - expected 'bytes', got 'utf'", str(results[0]))

		self.assertEqual("Foo/data (my_foo/data): incorrect type - expected 'bytes', got 'utf'", str(results[0]))


		class TestValidationResultWrapper:

	class TestValidationResultWrapper:
	class TestValidationResultWrapper(TestCase):

	IncorrectQuantityError, IllegalLinkError, ShapeError,IncorrectDataType,
	IncorrectQuantityError, IllegalLinkError, ShapeError, IncorrectDataType,

Conversation

sejalpunwatkar commented May 27, 2026

Motivation

How to test the behavior?

Checklist

Uh oh!

codecov Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rly commented May 30, 2026

Uh oh!

Uh oh!

rly commented Jun 2, 2026

Uh oh!

sejalpunwatkar commented Jun 6, 2026

Uh oh!

Uh oh!

rly Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

rly commented Jun 13, 2026

Uh oh!

sejalpunwatkar commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 27, 2026 •

edited

Loading

rly Jun 13, 2026 •

edited

Loading