aminghadersohi commented on PR #41895:
URL: https://github.com/apache/superset/pull/41895#issuecomment-4938817579
Thanks for the thorough review, @richardfogaca — addressed in 21233a56c9:
**BLOCKER — module scope vs. local `DatasetDAO`/`ColumnSpec` imports**
Kept `DatasetDAO` local, but documented it: this mirrors the established
convention across `mcp_service` (every DAO/command import in this module — and
in most of `mcp_service`, e.g. `chart_helpers.py:64,82,100`,
`chart_helpers.py:498,659`) is a function-local import annotated `# avoid
circular import`, so `superset.daos.*` never gets pulled in at `mcp_service`
module-import time. Added the same `# avoid circular import` comment to
`_find_dataset_by_id_or_uuid` for consistency (`chart_utils.py:281`).
`ColumnSpec` in the test file had no such constraint (tests already import
plenty of `superset.*` at module scope), so that one moved to the top-level
import and the two local re-imports were dropped (`test_chart_utils.py`).
**MEDIUM — `schemas.py:1405` `temporal_column` description**
Updated to reflect that the field now also drives dashboard time-filter
binding for `big_number_total`, not just the trendline x-axis.
**MEDIUM — `chart_utils.py:269` concrete type instead of `Any`**
Added a `TYPE_CHECKING`-guarded `SqlaTable` import and switched
`_find_dataset_by_id_or_uuid`'s return type, `is_column_truly_temporal`'s
`dataset` param, and `_resolve_default_x_axis`'s tuple return to `SqlaTable |
None` — no runtime import added.
**NIT — `test_big_number_chart.py:385`**
Removed the redundant `all(...call_args_list)` assertion;
`assert_called_once_with("42")` already covers it.
`ruff format`/`ruff check` clean, pre-commit (incl. mypy) clean, and `pytest
tests/unit_tests/mcp_service/chart/test_big_number_chart.py
tests/unit_tests/mcp_service/chart/test_chart_utils.py -q` → 207 passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
