Skip to content

testing.py carries a byte-identical copy of _field_guards.require_int/require_float #463

Description

@kstonekuan

src/hflow/testing.py:60-67 defines two helpers that are byte-for-byte identical to src/hflow/_video_measurements/_field_guards.py:17-30, message strings included.

# testing.py
def _require_int(value: object, name: str) -> None:
    if isinstance(value, bool) or not isinstance(value, int):
        raise ValueError(f"{name} must be an int, got {type(value).__name__}")

# _field_guards.py
def require_int(value: object, name: str) -> None:
    """Refuse anything but a plain ``int``, ``bool`` included."""
    if isinstance(value, bool) or not isinstance(value, int):
        raise ValueError(f"{name} must be an int, got {type(value).__name__}")

Same for _require_float / require_float. Only the docstring differs.

Why it is worth one small change

_field_guards is imported by exactly two modules today (_frame_statistics.py:17, _camera_motion.py:10), both inside _video_measurements. Its docstring says it is "for the video-measurement settings dataclasses", so the next person who needs this check does what testing.py did and writes it again. That is how a shared helper stops being shared.

testing.py uses its copy six times, so this is a real import, not a dead definition.

What to do

  1. Move the two functions to a home that is honest about being general. _field_guards.py sits under _video_measurements and nothing about require_int is video-specific, so a top-level private module (src/hflow/_field_guards.py) fits better than importing across package boundaries.
  2. Re-point _frame_statistics.py, _camera_motion.py, and testing.py at it. Delete testing.py's copy.
  3. Keep both public names as they are. This is a move, not a rename.

Explicitly not in scope

Do not sweep the other 50 call sites. There are about fifty places in src/ that write isinstance(x, bool) inline, and most of them should stay exactly as they are. They combine the type check with a finiteness and range check in one condition, and their message names the parameter the caller typed:

if (
    isinstance(horizontal_field_of_view_degrees, bool)
    or not np.isfinite(horizontal_field_of_view_degrees)
    or not 0 < horizontal_field_of_view_degrees <= 360
):
    raise ValueError("horizontal_field_of_view_degrees must be finite and in (0, 360], got ...")

src/hflow/checks.py:1004-1010. That shape is deliberate, and #434, #445 and #447 exist precisely because the alternative reported an internal field name the caller never typed. Replacing those with a shared helper would undo work merged this week. If a call site does nothing but the type check, folding it in is fine; if it carries a range or a caller-facing message, leave it.

packaging.py:1998-2056 has its own _require_integer family that returns the parsed value rather than returning None. Different contract, different module, out of scope.

Definition of done

  1. One definition of each helper, imported by all three current users.
  2. No behavior change anywhere: same exceptions, same message strings.
  3. The full suite passes with no test edits. If a test needs changing, the move changed behavior and something is wrong.
  4. Deleting the moved definition breaks the import rather than silently falling back to a copy.

Validation

uv sync --locked --all-extras
uv run ruff check
uv run ruff format --check
uv run ty check
uv run pytest -q

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions