Skip to content

refactor(video): reuse field guards in VideoImportConfig (#508) - #512

Merged
kstonekuan merged 2 commits into
Hebbian-Robotics:mainfrom
Rayan-and-beyond:refactor-video-import-guards-508
Sep 13, 2026
Merged

refactor(video): reuse field guards in VideoImportConfig (#508)#512
kstonekuan merged 2 commits into
Hebbian-Robotics:mainfrom
Rayan-and-beyond:refactor-video-import-guards-508

Conversation

@Rayan-and-beyond

@Rayan-and-beyond Rayan-and-beyond commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Closes #508.

What changes

VideoImportConfig.__post_init__ now reuses the shared numeric field guards instead of duplicating type and range checks.

  • duration_s, source_start_s, and image_hz use require_finite_float
  • image_width and image_height use require_positive_int, while the H.264 even-dimension rule stays local to video.py
  • start_time_ns uses require_int_in_range with the existing timestamp bounds

The video-specific validation remains in place, including the separate even-dimension error so non-positive and odd dimensions are reported distinctly.

Validation

Ran:

uv run pytest tests/test_video_import.py -q
uv run ruff check
uv run ruff format --check
uv run ty check

Result:

  • tests/test_video_import.py: 39 passed
  • Ruff check: clean
  • Ruff format: clean
  • ty: clean

I also checked each guard independently.

The start_time_ns upper-bound case originally stayed green when that bound was removed because the later MCAP timestamp-range check rejected the same value. I added a focused assertion for the field-level error so that removing only the upper bound now fails independently instead of being masked by the downstream check.

Copilot AI lite review requested due to automatic review settings September 12, 2026 04:05
@github-actions

Copy link
Copy Markdown

👋 Hi @Rayan-and-beyond — thank you so much for your first contribution to HFlow!

A maintainer will review your pull request as soon as possible. In the meantime:

💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game.

We are excited to have you here and appreciate your help making the project better! 🙌

@Rayan-and-beyond
Rayan-and-beyond force-pushed the refactor-video-import-guards-508 branch from 9c29b9b to 58a66cc Compare September 12, 2026 04:06

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.

🟢 Approval recommended

The refactor preserves domain-specific validation and includes focused regression coverage.

Pull request overview

Refactors VideoImportConfig to reuse shared numeric validation guards while preserving video-specific even-dimension checks.

Changes:

  • Reuses finite-number, positive-integer, and bounded-integer guards.
  • Retains local H.264 evenness validation.
  • Adds regression coverage for validation behavior and messages.
File summaries
File Description
tests/test_video_import.py Adds invalid-input and validation-message coverage.
src/hflow/importers/video.py Reuses shared field guards and retains evenness validation.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

@kstonekuan kstonekuan 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.

One mutation does not bite: widening the start_time_ns upper bound passes all 38 tests, even though tests/test_video_import.py:229 passes (1 << 64). Have a look at why.

It predates your change, so "I established it is not independently reachable, here is why" is a complete answer.

Everything else is clean.

Copy link
Copy Markdown
Contributor Author

Thanks — I traced the upper-bound mutation.

The existing start_time_ns = 1 << 64 case was only proving that the value was rejected somewhere. Without the upper bound in require_int_in_range, the later MCAP timestamp-range check still raised, so the test stayed green.

I kept the field-level bound and added a focused assertion for its error. With that in place, removing only the upper bound now makes the test fail independently.

I reran the video-import tests, Ruff, formatting, and ty; everything is green.

@kstonekuan kstonekuan 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.

LGTM, merging.

Both ends of the range are held now. I checked the lower bound too, which had the same exposure.

Nit, not worth a round trip: the expected bound is derived from the test input rather than from _MAXIMUM_TIMESTAMP_NS.

@kstonekuan
kstonekuan merged commit ff201ba into Hebbian-Robotics:main Sep 13, 2026
6 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.

VideoImportConfig hand-rolls three numeric guards, and only one of them maps straight onto a helper

3 participants