michael-s-molina commented on code in PR #37817:
URL: https://github.com/apache/superset/pull/37817#discussion_r2931144047
##########
superset/daos/datasource.py:
##########
Review Comment:
Don't we need to include SemanticView here?
##########
superset/commands/explore/get.py:
##########
@@ -124,7 +124,11 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901
security_manager.raise_for_access(datasource=datasource)
viz_type = form_data.get("viz_type")
- if not viz_type and datasource and datasource.default_endpoint:
+ if (
+ not viz_type
+ and datasource
+ and getattr(datasource, "default_endpoint", None)
Review Comment:
Using `getattr` with a fallback here signals that `default_endpoint` is not
part of the `Explorable` protocol, but callers expect it. The right fix is to
add `default_endpoint` to the `Explorable` protocol (it's already in
`ExplorableData`) and have `SemanticView` implement it returning `None`. That
makes the protocol complete and removes the need for defensive attribute access.
##########
superset-frontend/src/explore/actions/saveModalActions.ts:
##########
@@ -151,11 +151,8 @@ export const getSlicePayload = async (
const [id, typeString] = formData.datasource.split('__');
datasourceId = parseInt(id, 10);
- const formattedTypeString =
- typeString.charAt(0).toUpperCase() + typeString.slice(1);
- if (formattedTypeString in DatasourceType) {
- datasourceType =
- DatasourceType[formattedTypeString as keyof typeof DatasourceType];
+ if (Object.values(DatasourceType).includes(typeString as DatasourceType)) {
Review Comment:
Great improvement with the types!
--
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]
