test(silver): enumerate handlers + per-handler raise check

[Artic0din]

Hardens test_handlers_raise_home_assistant_error_on_missing_coordinator after your question — the old hard-coded >= 3 count threshold let handle_reset_today slip through silently. New test enumerates handlers via regex, checks each body for the raise, reports specific missing handlers.

Test-only change — no manifest bump, no HACS deploy. Will wait for gemini review before merging.

[gemini-code-assist]

Code Review

This pull request updates the test suite to dynamically verify that all service handlers in __init__.py raise HomeAssistantError on unrecoverable conditions, replacing a static count check. The review feedback correctly identifies that the regex-based approach for parsing function bodies is fragile and prone to false positives, particularly for the last handler in the file. It is recommended to use the ast module for more robust code inspection as suggested.

[gemini-code-assist]

The current regex-based approach is fragile and contains a logic error in the body_end calculation. For the last handler, body_end defaults to len(src), which includes the entire remainder of the file (e.g., async_unload_entry). If any subsequent code raises HomeAssistantError, the test will produce a false positive for the last handler. Additionally, the regex is highly sensitive to exact whitespace and type hints.

Using the ast module provides a much more robust way to inspect function bodies and correctly bounds the search to each handler's scope.

        import ast
        src = (REPO / "custom_components" / "pricehawk" / "__init__.py").read_text()
        tree = ast.parse(src)

        # Find the function where handlers are defined
        reg_func = next(
            (n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "_register_services_once"),
            None
        )
        assert reg_func, "_register_services_once not found in __init__.py"

        # Find all nested async functions starting with 'handle_'
        handlers = [
            n for n in reg_func.body
            if isinstance(n, ast.AsyncFunctionDef) and n.name.startswith("handle_")
        ]

        assert len(handlers) >= 4, f"Expected >= 4 handlers, found {len(handlers)}"

        missing_raises = []
        for handler in handlers:
            # Check if HomeAssistantError is raised in the handler's scope
            has_raise = any(
                isinstance(node, ast.Raise) and node.exc and (
                    (isinstance(node.exc, ast.Call) and getattr(node.exc.func, "id", None) == "HomeAssistantError") or
                    (getattr(node.exc, "id", None) == "HomeAssistantError")
                )
                for node in ast.walk(handler)
            )
            if not has_raise:
                missing_raises.append(handler.name)

        assert not missing_raises, (
            f"Silver action-exceptions: these handlers don't raise "
            f"HomeAssistantError anywhere in their body: {missing_raises}. "
            f"Every service handler must raise on unrecoverable conditions "
            f"(missing coordinator, no entries, etc)."
        )

[Artic0din]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the test_silver_checklist.py file to replace a fragile string-based check with a more robust AST-based validation. The new test ensures that every service handler in __init__.py raises a HomeAssistantError for unrecoverable conditions. Feedback was provided regarding the implementation of the AST traversal; specifically, the current use of ast.walk fails to correctly prune nested function definitions, which could lead to false positives. A code suggestion was offered to implement a manual traversal that properly excludes nested scopes.

[gemini-code-assist]

The current implementation of _body_has_raise_hae does not correctly prune nested function definitions. ast.walk is a flat generator that yields all nodes in the subtree; calling continue when an AsyncFunctionDef is encountered only skips the logic for that specific node, but ast.walk will still yield all of its children (including any raise statements inside the nested function) in subsequent iterations. This leads to false positives where a handler is credited for a raise statement that actually belongs to a nested helper function.

To correctly restrict the search to the current function's scope, you should use a traversal that allows skipping subtrees, such as a manual stack-based BFS/DFS or a custom ast.NodeVisitor that does not descend into function nodes. Additionally, you should skip ast.FunctionDef (synchronous functions) as well to be consistent with the intent of excluding nested scopes.

Suggested change

	for child in ast.walk(stmt):
	if isinstance(child, ast.AsyncFunctionDef):
	# Don't descend into nested defs — they're their
	# own scope and validated separately.
	continue
	if not isinstance(child, ast.Raise):
	continue
	exc = child.exc
	if isinstance(exc, ast.Call) and isinstance(
	exc.func, ast.Name
	) and exc.func.id == "HomeAssistantError":
	return True
	todo = [stmt]
	while todo:
	node = todo.pop()
	if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
	# Don't descend into nested defs — they're their
	# own scope and validated separately.
	continue
	if isinstance(node, ast.Raise):
	exc = node.exc
	if isinstance(exc, ast.Call) and isinstance(
	exc.func, ast.Name
	) and exc.func.id == "HomeAssistantError":
	return True
	todo.extend(ast.iter_child_nodes(node))

[Artic0din]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the test_every_service_handler_raises_home_assistant_error in tests/test_silver_checklist.py to use an AST-based approach instead of simple string counting. This improvement allows for more precise validation that every service handler correctly raises HomeAssistantError within its own scope. Feedback suggests extending the check to support synchronous function definitions, including ServiceValidationError as a valid exception, and ensuring nested class definitions are correctly pruned during the AST walk.

[gemini-code-assist]

Service handlers in Home Assistant can be implemented as either synchronous (def) or asynchronous (async def) functions. To ensure full coverage of the Silver action-exceptions rule, the test should enumerate both types. I've also updated the helper name and type hint to reflect this broader scope.

Suggested change

	def _iter_async_funcs(node):
	"""Yield AsyncFunctionDef nodes recursively (handlers may be
	nested inside async_setup_entry, etc)."""
	for child in ast.walk(node):
	if isinstance(child, ast.AsyncFunctionDef):
	yield child
	def _body_has_raise_hae(func: ast.AsyncFunctionDef) -> bool:
	def _iter_funcs(node):
	"""Yield FunctionDef and AsyncFunctionDef nodes recursively (handlers may be
	nested inside async_setup_entry, etc)."""
	for child in ast.walk(node):
	if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)):
	yield child
	def _body_has_raise_hae(func: ast.FunctionDef | ast.AsyncFunctionDef) -> bool:

[gemini-code-assist]

To correctly prune all nested scopes during the AST walk, ast.ClassDef should be included in the skip list. While unlikely in a service handler, a raise statement directly within a nested class body (executed at definition time) would currently be incorrectly attributed to the outer handler function.

Suggested change

	if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
	if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):

[gemini-code-assist]

The Silver "action-exceptions" rule requires raising HomeAssistantError or one of its subclasses. ServiceValidationError is a common subclass used for input validation in this integration. Including it in this check prevents false negatives for handlers that might only perform input validation (though currently all handlers in this integration also perform a coordinator check).

Suggested change

	and exc.func.id == "HomeAssistantError"
	and exc.func.id in ("HomeAssistantError", "ServiceValidationError")

[gemini-code-assist]

Update the call to use the renamed helper function that includes both sync and async function definitions.

Suggested change

	f for f in _iter_async_funcs(tree)
	f for f in _iter_funcs(tree)

[Artic0din]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces a fragile string-based test with a robust AST-based validation to ensure all service handlers in __init__.py raise required Home Assistant exceptions. The new implementation correctly handles nested scopes and scales with the number of handlers. A review comment suggests enhancing the detection logic to include exceptions raised without parentheses, which would make the test more robust against different coding styles.

[gemini-code-assist]

The current logic only detects exceptions raised as calls (e.g., raise HomeAssistantError(...)). It will miss cases where the exception class is raised directly without parentheses (e.g., raise HomeAssistantError), which is valid Python and occasionally used in Home Assistant integrations. Expanding the check to handle both ast.Call and ast.Name makes the test more robust against different coding styles.

                        if isinstance(exc, ast.Call):
                            exc = exc.func
                        if isinstance(exc, ast.Name) and exc.id in _ACTION_EXCEPTIONS:
                            return True

[Artic0din]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the service handler validation test in tests/test_silver_checklist.py. The previous implementation, which relied on simple string counting, has been replaced with a more robust approach using the ast module to parse and inspect function bodies. This ensures that every service handler (prefixed with handle_) explicitly raises HomeAssistantError or ServiceValidationError while correctly ignoring nested scopes and supporting various raise syntax forms. I have no feedback to provide as there were no review comments.