Require timezone information for isodatetime validation

[sejalpunwatkar]

This PR adds validation to ensure that values with string_format="isodatetime" include timezone information.

Specifically:

• Added a has_timezone() helper function to detect timezone presence.

• Integrated timezone validation in both AttributeValidator and DatasetValidator.

• Raises a validation Error if an ISO datetime string is missing timezone information.

• Existing type and shape validation behavior remains unchanged.

Please let me know if enforcing timezone as a requirement aligns with the intended specification behavior.

Fix #1284

[rly]

Thank you @sejalpunwatkar . Please add tests to verify that your fix worked. The tests should fail prior to this change and succeed with this change.

[codecov]

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.84%. Comparing base (6f481ae) to head (b19c601).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
src/hdmf/validate/validator.py	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1399      +/-   ##
==========================================
+ Coverage   92.83%   92.84%   +0.01%     
==========================================
  Files          41       41              
  Lines        9958     9986      +28     
  Branches     2039     2052      +13     
==========================================
+ Hits         9245     9272      +27     
  Misses        436      436              
- Partials      277      278       +1     

☔ View full report in Codecov by Sentry.
📢 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.

[sejalpunwatkar]

Thanks for the feedback, @rly . I will add unit tests to tests/unit/test_validator.py (or the relevant test suite) that include ISO datetime strings both with and without timezones to ensure the new validation logic triggers correctly and prevents regressions.

[rly]

Thanks @sejalpunwatkar . This is looking good. Can you also add a test for validating DatasetSpec with dtype=isodatetime?

[rly]

Ideally the tests in each PR cover each change made in the PR. You can look at the codecov report above to see what lines were not touched by the tests. It's easy for a bug to appear if the code isn't tested.

[rly]

When you're done, could you please also add an entry to the CHANGELOG.md for version 5.0.0 with a brief description of the change. Follow the format of the other changelog entries. This would go under "Fixed"

[sejalpunwatkar]

Thanks for the detailed feedback @rly ! I’ll get started on these updates right away, including the 'Z' suffix test, the DatasetSpec validation, and the CHANGELOG entry. I'll also make sure to hit the missing lines to get that Codecov report back to green.
I'll have the commits pushed shortly so they're ready for your review whenever you're back. enjoy the break

[sejalpunwatkar]

Updates made:

• Enforced timezone requirement for isodatetime dtype validation.

Added tests for:

• Valid timestamp with explicit offset (+02:00)

• Missing timezone (expected validation error)

• UTC "Z" suffix case

• Added a corresponding entry in CHANGELOG.md.

All tests are passing locally.

[Copilot]

Pull request overview

This PR adds timezone validation for isodatetime values to address issue #1284, which reported that the validator doesn't raise warnings when datetime values lack required timezone information.

Changes:

• Added has_timezone() helper function to detect timezone presence in ISO datetime strings
• Integrated timezone validation into AttributeValidator and DatasetValidator
• Added test cases for scalar datetime strings with and without timezone information

Reviewed changes

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

File	Description
src/hdmf/validate/validator.py	Added has_timezone() helper function and integrated timezone checks in AttributeValidator and DatasetValidator to raise Error when isodatetime strings lack timezone information
tests/unit/validator_tests/test_validate.py	Added TestISODateTimeTimezone and TestISODateTimeDatasetTimezone test classes to verify timezone validation for attributes and datasets with scalar datetime strings
CHANGELOG.md	Added entry documenting the fix for missing timezone validation

---

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

[rly]

Thanks for the updates @sejalpunwatkar . Could you please address the issues raised by copilot? They are mostly the same issue. I do think we need to test arrays of datetimes, that checking for "+"/"-" can lead to false positives, and that the variable names could be made clearer. The arrays issue is causing a test failure in the pynwb tests. Any existing hdmf test(s) where a datetime without time zone is used should be updated because that is not valid. Thank you!

[sejalpunwatkar]

Updated validator.py to enforce that isodatetime datasets include timezone information.

• Modified AttributeValidator and DatasetValidator to check timezone for scalar and array data (lists, tuples, numpy arrays).

• Added comprehensive unit tests in test_validate.py to cover:

• Scalar datetimes with/without timezone

• Arrays with mixed timezone presence

• Empty arrays

• Numpy arrays

• Python datetime objects

• Ensures validation raises an Error when timezone is missing and passes when present.

[oruebel]

@sejalpunwatkar thanks for the updates! We are in a bit of crunch this week so it'll likely be later next week before we get back to this on our end. Feel free to re-ping at the end of next week if you have not seen activity here.

[sejalpunwatkar]

No problem at all, @oruebel . I appreciate the update! I'll use this time to verify that every test runs smoothly and keep the PR ready for your review. I'll re-ping you at the end of next week as suggested. Hope the crunch goes well!

[rly]

Suggested change

	class TestISODateTimeTimezone(ValidatorTestBase):
	"""Test that isodatetime dtype requires timezone information."""
	class TestISODateTimeAttributeTimezone(ValidatorTestBase):
	"""Test that an AttributeSpec with isodatetime dtype requires timezone information."""

[rly]

Suggested change

	"""Test that isodatetime dtype in DatasetSpec requires timezone information."""
	"""Test that a DatasetSpec with isodatetime spec requires timezone information."""

[sejalpunwatkar]

Hi @rly , I've addressed all the feedback and pushed the updates:
Refactored Validation: Extracted the timezone check into a standalone utility function _is_missing_timezone to keep the code DRY and reusable.
Restored Original Logic: Re-inserted the spec.required validation and the EmptyArrayError catch/comments that were previously removed.
Updated Error Handling: Used the specific Error constructors with correct location information for both attributes and datasets.
Tests Updated: Renamed the test classes to TestISODateTimeAttributeTimezone and TestISODateTimeDatasetTimezone and updated the docstrings as suggested.
All 1663 local tests are passing. Ready for your final review!

[sejalpunwatkar]

No worries at all, @rly ! I really appreciate the detailed feedback and the opportunity to make the code cleaner and more robust. It’s been a great learning process.
I’ve addressed all the latest comments:

1. Integrated Validation: Moved the timezone check directly into AttributeValidator and DatasetValidator and removed the standalone helper method for better consistency.
2. Simplified has_timezone: Refactored the function to focus strictly on scalar strings, as the container iteration is now handled upstream.
3. Multi-dimensional Support: Updated _is_missing_timezone to handle multi-dimensional numpy arrays (using np.nditer) and nested lists/tuples.
4. Test Cleanup: Consolidated the redundant test cases into the original project tests and added the 'timezone' error message verification as suggested.
   All 1663 local tests are passing. Ready for your final review whenever you have a moment

[rly]

Looks good @sejalpunwatkar ! I made a couple minor changes.

One last comment: TestISODateTimeAttributeTimezone only tests a couple cases, whereas TestISODateTimeDatasetTimezone is thorough. We should test the same cases for attributes as for datasets. However, I do not want all of that code to be simply copied and pasted with slight modifications for testing attributes vs datasets. Can you refactor the boilerplate code for these tests and add the same tests for attributes that you have for datasets? It might help to have getSpecs return a GroupSpec with an optional DatasetSpec (quantity='?') and an optional AttributeSpec (required=False) so that you can use that for both attribute testing and dataset testing.

[sejalpunwatkar]

Hi @rly , I've finished the refactor! I ran into some merge conflicts earlier which have now been resolved. The tests are passing locally (1659 passed), and the code is now unified into a single class with the shared helper as requested. Ready for another look!

[rly]

Looks good. Thank you for your sustained effort on this @sejalpunwatkar !

[sejalpunwatkar]

Thank you, @rly ! I appreciate the support in getting this across the finish line. Looking forward to helping out with more issues soon.