mirror of
https://github.com/apache/superset.git
synced 2026-07-16 11:46:09 +00:00
f840ce9813
Five structural changes — no behaviour change — applied as one commit per the tidy-first discipline. All from the clean-code review of e4070a4716. * Tidy 1 — Fix _fetch_change_records sort key. Was calling .timestamp() on issued_at with an `else 0` fallback; the fallback was dead defense (column is non-null per sc-103156 schema) and .timestamp() introduces non-determinism on tz-naive datetimes. Sort on the datetimes directly. * Tidy 2 — parse_activity_query_params now raises ActivityParamsError (subclass of ValueError) instead of returning (Optional[dict], Optional[str]). The tuple was forcing every caller into a defensive `if error or params is None: return self.response_400(message=error or "Invalid query parameters")`. The new shape — `try: params = parse(...); except ActivityParamsError as exc: return response_400(str(exc))` — is shorter, type-safe, and the contract is enforced at the boundary. * Tidy 3 — Test helper now uses Flask client's query_string= parameter instead of f-string concatenation. Handles URL-encoding correctly for the day a test passes a value containing & / = / + / etc. * Tidy 4 — get_activity pipeline collapses to a single rolling `records` variable instead of the mid-stream `raw / visible_raw / enriched / visible` naming. Each function call's name documents what the step does; no intermediate variable names needed. * Tidy 5 — Extracted four per-parameter parsers: _parse_optional_iso, _parse_include, _parse_page, _parse_page_size. The "parse one parameter" concept now has a name. Cost: four small helpers (each 10-15 lines, one job). Benefit: parse_activity_query_params is a 10-line table-driven dispatcher. Test changes: parser unit tests now use pytest.raises(ActivityParamsError) instead of unpacking the (params, error) tuple. Added one test confirming ActivityParamsError subclasses ValueError so the standard library exception hierarchy still catches it. Total unit tests: 57. Integration tests still 10/10 green. Deferred per the review: the UUID-parse + entity-find + ownership- check dance is duplicated between activity / list_versions / get_version and will grow to T028 / T033. Refactor when T028 lands — three real callers > one prospective one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>