aminghadersohi commented on code in PR #40060:
URL: https://github.com/apache/superset/pull/40060#discussion_r3235855896
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1212,6 +1212,19 @@ def _resolve_viz_type(config: Any) -> str:
return "unknown"
+CHART_TYPE_LABELS = {
Review Comment:
**NIT:** `CHART_TYPE_LABELS` implies a general registry for all chart types,
but the PR scopes this deliberately to table-family only (and
`get_table_chart_type_label` makes that intent clear in the function name).
Future contributors seeing this dict may add `"echarts_timeseries_bar": "bar
chart"` etc., which isn't the intended design. Consider `TABLE_VIZ_TYPE_LABELS`
to match the function name and prevent scope creep.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1212,6 +1212,19 @@ def _resolve_viz_type(config: Any) -> str:
return "unknown"
+CHART_TYPE_LABELS = {
+ "table": "table chart",
+ "ag-grid-table": "interactive table chart",
+}
+
+
+def get_table_chart_type_label(viz_type: str | None) -> str | None:
+ """Return a user-facing label for table-family Superset viz types."""
+ if viz_type is None:
Review Comment:
**NIT:** The `if viz_type is None` guard is redundant — `dict.get(None)`
already returns `None` when `None` is not a key. Can simplify the body to a
single line: `return CHART_TYPE_LABELS.get(viz_type)`.
##########
tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:
##########
@@ -45,6 +46,58 @@
class TestGenerateChart:
"""Tests for generate_chart MCP tool."""
+ @pytest.mark.asyncio
+ async def test_generate_chart_returns_table_chart_type_label(self):
Review Comment:
**NIT:** Missing `-> None` return type annotation on this async test method.
All test methods should have `-> None` per the project style (Rule 12). The
other new tests in this PR do have it (e.g., `test_chart_utils.py`'s
`TestGetTableChartTypeLabel`).
--
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]
