refactor: move require_int/require_float to hflow._field_guards - #464
Open
7487 wants to merge 1 commit into
Open
refactor: move require_int/require_float to hflow._field_guards#4647487 wants to merge 1 commit into
7487 wants to merge 1 commit into
Conversation
testing.py carried a byte-identical copy of the two type guards that _video_measurements/_field_guards.py already defined. Move the module to the package root, point _frame_statistics, _camera_motion and testing at it, and delete the copy. Same functions, same exception types and message strings; no behavior change. tests/test_module_boundaries.py allowlists hflow._field_guards for the two boundaries the shared import now crosses (the video-measurements package and hflow.testing) and keeps every other hflow.* import red. Fixes Hebbian-Robotics#463 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
👋 Hi @7487 — 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! 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
require_int/require_floatnow have one definition,src/hflow/_field_guards.py, imported by_frame_statistics.py,_camera_motion.pyandtesting.py. The byte-identical private copy intesting.pyis gone. Same exception types, same message strings, same public names. No behavior change; the other ~50 inlineisinstancesites are untouched, per the issue.Fixes #463
Why
The move is what the issue asks for. One thing did not go as the issue's definition of done predicted, and it is worth being explicit about:
One test edit was required.
tests/test_module_boundaries.pyenforces two import contracts that any single-definition layout has to cross:_video_measurements/*may import nothing fromhflow.*(empty allowlist), andhflow.testingmay import onlyhflow.ffmpegandhflow.format.With the layout from the issue and no test change, the guard fails with exactly the three imports this PR adds:
The AST guard skips relative imports, so
from .._field_guards import ...inside_video_measurementswould have passed it silently. I did not do that: it is the same dependency, just hidden from the test. Instead both boundaries allowlisthflow._field_guardsby name, with the rule text updated to say so._field_guardshas no imports of its own, so extracting_video_measurementslater means vendoring 14 lines. I mutation-checked that afrom hflow.storage import ...inside_camera_motion.pyis still red under the new rule.If you would rather keep the
_video_measurementsallowlist empty, the alternative is to leave_field_guards.pywhere it was and havetesting.pyimport it from there (only thehflow.testingallowlist changes). Happy to flip; it is a one-line direction change.Also removed the
testing.pymodule-docstring bullet that explained why the copy existed, since it no longer does.Validation
Ran on macOS (CI is Linux-only). The 27 failures are all
tests/test_packaging*.pyraisingCythonOverlayBuildError: native overlay builds currently require CPython on Linux; found cpython on darwin; the identical 27 fail on pristineupstream/main(7df224a) in the same environment, so they are platform-only and CI covers them.Additional checks:
uv run pytest -q tests/test_module_boundaries.pywith the source change and the unmodified test: 1 failed with the three violations quoted above, then 2 passed with the allowlist.from hflow.storage import _load_obstoreto_camera_motion.py, guard went red naming that line; restored, green.tests/test_testing.py,tests/test_video_measurement_settings_types.py,tests/test_video_measurement_settings_ranges.py: 103 passed, unmodified. These already pin everymust be an int[, got ...]/must be an int or float, got ...message through the public constructors, which is why no new test is added for a pure move.git diff --checkclean.Checklist
_field_guards.uv run ruff check --fix,uv run ruff format, anduv run ty check.🤖 Generated with Claude Code