Skip to content

chore: consolidate check helpers into resources/_check_helpers#87

Merged
henchaves merged 1 commit into
mainfrom
chore/centralize-check-helpers
May 4, 2026
Merged

chore: consolidate check helpers into resources/_check_helpers#87
henchaves merged 1 commit into
mainfrom
chore/centralize-check-helpers

Conversation

@henchaves
Copy link
Copy Markdown
Member

Summary

Consolidates the request-side check helpers that were spread between types/check.py and resources/evaluations/evaluations.py into a single shared resources/_check_helpers.py module. Merges the two list-builders into one parametrized function. No behavior change.

What changed

  • New resources/_check_helpers.py housing IDENTIFIER_TO_KIND, check_param_to_spec, and check_params_to_specs(..., flat: bool).
  • _check_params_to_api (test-cases shape) and _check_params_to_flat_specs (run-single shape) merged into check_params_to_specs(..., flat=False/True).
  • Underscore prefixes dropped on the exported names — they're shared across sister modules inside a package-private file, matching the convention in resources/_included.py. Removes a # pyright: ignore workaround.
  • _extract_check_params stays in types/check.py — response-side helper used by the Check / CheckConfig pydantic validators.
  • Tests/imports updated accordingly.

@henchaves henchaves self-assigned this May 4, 2026
@henchaves henchaves merged commit 465a2d8 into main May 4, 2026
5 checks passed
@henchaves henchaves deleted the chore/centralize-check-helpers branch May 4, 2026 14:38
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request centralizes the logic for converting check parameters into the wire format by introducing a new helper module, _check_helpers.py. It replaces several redundant internal implementations across the checks, evaluations, and test_cases resources and updates the corresponding tests. A bug was identified in the check_params_to_specs function where the flat=True branch fails to handle Pydantic models, which would lead to an AttributeError at runtime.

Comment on lines +56 to +61
result.append({"identifier": identifier, **{k: v for k, v in params.items() if k != "type"}})
else:
entry: Dict[str, Any] = {"identifier": identifier, "enabled": check.get("enabled", True)}
if params:
entry["spec"] = check_param_to_spec(identifier, params)
result.append(entry)
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.

high

The flat=True branch currently assumes params is a dictionary and calls .items(). However, params can also be a Pydantic BaseModel (as handled in check_param_to_spec). This will cause an AttributeError at runtime if a model is passed. Reusing check_param_to_spec ensures consistent handling of both dictionaries and models while also correctly stripping the type key.

Suggested change
result.append({"identifier": identifier, **{k: v for k, v in params.items() if k != "type"}})
else:
entry: Dict[str, Any] = {"identifier": identifier, "enabled": check.get("enabled", True)}
if params:
entry["spec"] = check_param_to_spec(identifier, params)
result.append(entry)
if flat:
spec = check_param_to_spec(identifier, params)
spec.pop("kind", None)
result.append({"identifier": identifier, **spec})
else:
entry: Dict[str, Any] = {"identifier": identifier, "enabled": check.get("enabled", True)}
if params:
entry["spec"] = check_param_to_spec(identifier, params)
result.append(entry)

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant