codeant-ai-for-open-source[bot] commented on code in PR #36367:
URL: https://github.com/apache/superset/pull/36367#discussion_r2590134882
##########
superset/commands/explore/get.py:
##########
@@ -136,7 +136,7 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901
utils.merge_extra_filters(form_data)
utils.merge_request_params(form_data, request.args)
- datasource_data: BaseDatasourceData | QueryData = {
+ datasource_data: ExplorableData = {
Review Comment:
**Suggestion:** After moving the type import under TYPE_CHECKING, the
annotation `datasource_data: ExplorableData = {...}` will reference a name that
does not exist at runtime and will raise a NameError on module import; annotate
using a string literal instead so the annotation is not evaluated at runtime
while still being usable by static type checkers. [type error]
**Severity Level:** Minor ⚠️
```suggestion
datasource_data: "ExplorableData" = {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Changing the annotation to a string literal ("ExplorableData") is the
correct complement to moving the TypedDict import under TYPE_CHECKING: it
avoids runtime name lookup while remaining usable for static type checkers.
It's safe even if the import remains runtime, and it prevents the NameError
that would otherwise occur.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/explore/get.py
**Line:** 139:139
**Comment:**
*Type Error: After moving the type import under TYPE_CHECKING, the
annotation `datasource_data: ExplorableData = {...}` will reference a name that
does not exist at runtime and will raise a NameError on module import; annotate
using a string literal instead so the annotation is not evaluated at runtime
while still being usable by static type checkers.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/connectors/sqla/models.py:
##########
@@ -105,7 +106,7 @@
from superset.superset_typing import (
AdhocColumn,
AdhocMetric,
- BaseDatasourceData,
+ ExplorableData,
Metric,
QueryObjectDict,
ResultSetColumnType,
Review Comment:
**Suggestion:** Avoid importing `ExplorableData` at runtime: it's only used
for type annotations. Importing it at module level can create unnecessary
import costs or circular imports. Remove it from the runtime import list and
import it under TYPE_CHECKING so the annotation remains available to type
checkers but does not execute the module at import time. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
Metric,
QueryObjectDict,
ResultSetColumnType,
)
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from superset.superset_typing import ExplorableData
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
ExplorableData is a TypedDict defined in superset/superset_typing.py. The
models module only uses it in return/type annotations
(e.g., the data property). Since the module already uses "from __future__
import annotations", these annotations are stored
as strings and do not require the actual TypedDict at import time. Moving
ExplorableData into a TYPE_CHECKING-only import
avoids unnecessary runtime imports and reduces the risk of
import-order/circularity. This is an actionable, low-risk improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 109:112
**Comment:**
*Possible Bug: Avoid importing `ExplorableData` at runtime: it's only
used for type annotations. Importing it at module level can create unnecessary
import costs or circular imports. Remove it from the runtime import list and
import it under TYPE_CHECKING so the annotation remains available to type
checkers but does not execute the module at import time.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/connectors/sqla/models.py:
##########
@@ -85,6 +85,7 @@
SupersetSecurityException,
SupersetSyntaxErrorException,
)
+from superset.explorables.base import TimeGrainDict
Review Comment:
**Suggestion:** Runtime import/circular-import risk: importing
`TimeGrainDict` from `superset.explorables.base` at module import time can
create import-order/circular-import issues (or unnecessary work) because this
type is only needed for type annotations. Guard the import with TYPE_CHECKING
and provide a lightweight runtime fallback to avoid executing the other module
at import time. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from superset.explorables.base import TimeGrainDict
else:
# lightweight runtime fallback used only to ensure the name exists at
runtime;
# actual structure is only needed for typing.
TimeGrainDict = dict
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
TimeGrainDict is a TypedDict defined in superset/explorables/base.py (see
TimeGrainDict in that module).
Importing it at module import time is unnecessary because this file has
"from __future__ import annotations",
so annotations are deferred to runtime and the name does not need to be
available at import time.
Moving the import under TYPE_CHECKING prevents potential
import-order/circular-import problems and
slightly reduces import-time coupling. This is a small safety/maintenance
improvement, not a breaking change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 88:88
**Comment:**
*Possible Bug: Runtime import/circular-import risk: importing
`TimeGrainDict` from `superset.explorables.base` at module import time can
create import-order/circular-import issues (or unnecessary work) because this
type is only needed for type annotations. Guard the import with TYPE_CHECKING
and provide a lightweight runtime fallback to avoid executing the other module
at import time.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
