aminghadersohi commented on PR #36035: URL: https://github.com/apache/superset/pull/36035#issuecomment-3499593729
## Addressed Review Feedback All 5 Korbit AI review comments have been addressed: ### 1-4: Required field validation, error handling, field validation, separation of concerns Created comprehensive `filter_model_fields()` utility function in `superset/mcp_service/utils/__init__.py` that: - Automatically includes required fields to prevent ValidationError - Validates fields exist in the model before including them - Has comprehensive error handling with graceful fallback to full model - Separates filtering logic from serialization logic - Fully documented with clear docstrings ### 5: Overly permissive type hints Replaced `Any` types with specific type hints across all three list tool serializers: **Chart serializer** (`list_charts.py`): ```python def _serialize_chart(obj: "Slice | None", cols: list[str] | None) -> ChartInfo | None ``` **Dashboard serializer** (`list_dashboards.py`): ```python def _serialize_dashboard(obj: "Dashboard | None", cols: list[str] | None) -> DashboardInfo | None ``` **Dataset serializer** (`list_datasets.py`): ```python def _serialize_dataset(obj: "SqlaTable | None", cols: list[str] | None) -> DatasetInfo | None ``` Also removed unnecessary `isinstance` checks since the type hints now make the expected types explicit. Used TYPE_CHECKING imports to avoid circular dependencies. All changes pass mypy type checking and pre-commit hooks. -- 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]
