bito-code-review[bot] commented on code in PR #40296:
URL: https://github.com/apache/superset/pull/40296#discussion_r3287020271
##########
superset/models/core.py:
##########
@@ -1317,7 +1317,7 @@ def execute(
:param options: QueryOptions with execution settings
:returns: QueryResult with status, data, and metadata
"""
- from superset.sql.execution import SQLExecutor
+ from superset.sql.execution import SQLExecutor # noqa: PLC0415
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>PLC0415 noqa masks violation</b></div>
<div id="fix">
The `# noqa: PLC0415` comment masks a linting violation. Per pyproject.toml
lines 404-428, `superset/models/` is not in the list of directories allowed to
have function-body imports. The proper fix is to move `from
superset.sql.execution import SQLExecutor` to the top-level imports section,
not suppress the error.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/models/core.py (lines 85-86) ---
85: from superset.utils import cache as cache_util, core as utils, json
86: +from superset.sql.execution import SQLExecutor
87: from superset.utils.backports import StrEnum
--- superset/models/core.py (lines 1318-1322) ---
1318: :returns: QueryResult with status, data, and metadata
1319: """
1320: - from superset.sql.execution import SQLExecutor # noqa:
PLC0415
1321: + return SQLExecutor(self).execute(sql, options)
1322:
--- superset/models/core.py (lines 1334-1338) ---
1334: :returns: AsyncQueryHandle for tracking the query
1335: """
1336: - from superset.sql.execution import SQLExecutor # noqa:
PLC0415
1337: + return SQLExecutor(self).execute_async(sql, options)
```
</div>
</details>
</div>
<small><i>Code Review Run #86bbd8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-extensions-cli/tests/test_cli_build.py:
##########
@@ -347,7 +347,7 @@ def
test_build_manifest_exits_when_extension_json_missing(isolated_filesystem):
@pytest.mark.unit
def test_clean_dist_frontend_removes_frontend_dist(isolated_filesystem):
"""Test clean_dist_frontend removes frontend/dist directory
specifically."""
- from superset_extensions_cli.cli import clean_dist_frontend
+ from superset_extensions_cli.cli import clean_dist_frontend # noqa:
PLC0415
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>PLC0415 suppression masks fixable issue</b></div>
<div id="fix">
The `# noqa: PLC0415` comments suppress a legitimate pylint warning. These 4
functions should be added to the existing top-level import block (lines 24-31)
instead of using local imports. Having imports in two places creates
maintenance divergence risk.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset-extensions-cli/tests/test_cli_build.py (lines 24-31) ---
24: from superset_extensions_cli.cli import (
25: app,
26: build_manifest,
27: clean_dist,
28: copy_backend_files,
29: copy_frontend_dist,
30: init_frontend_deps,
31: + clean_dist_frontend,
32: + run_frontend_build,
33: + rebuild_frontend,
34: + rebuild_backend,
35: )
--- superset-extensions-cli/tests/test_cli_build.py (lines 347-353) ---
347: @pytest.mark.unit
348: def
test_clean_dist_frontend_removes_frontend_dist(isolated_filesystem):
349: """Test clean_dist_frontend removes frontend/dist directory
specifically."""
350: - from superset_extensions_cli.cli import clean_dist_frontend #
noqa: PLC0415
351: +
352: # Create dist/frontend structure
353: dist_dir = isolated_filesystem / "dist"
--- superset-extensions-cli/tests/test_cli_build.py (lines 366-372) ---
366: @pytest.mark.unit
367: def
test_clean_dist_frontend_handles_nonexistent_directory(isolated_filesystem):
368: """Test clean_dist_frontend handles case where frontend dist
doesn't exist."""
369: - from superset_extensions_cli.cli import clean_dist_frontend #
noqa: PLC0415
370: +
371: # No dist directory exists
372: clean_dist_frontend(isolated_filesystem)
--- superset-extensions-cli/tests/test_cli_build.py (lines 377-383) ---
377: @pytest.mark.unit
378: def test_run_frontend_build_with_output_messages(isolated_filesystem):
379: """Test run_frontend_build produces expected output messages."""
380: - from superset_extensions_cli.cli import run_frontend_build #
noqa: PLC0415
381: +
382: frontend_dir = isolated_filesystem / "frontend"
383: frontend_dir.mkdir()
--- superset-extensions-cli/tests/test_cli_build.py (lines 406-412) ---
406: isolated_filesystem, return_code, expected_result
407: ):
408: """Test rebuild_frontend handles different build results."""
409: - from superset_extensions_cli.cli import rebuild_frontend # noqa:
PLC0415
410: +
411: # Create frontend structure
412: frontend_dir = isolated_filesystem / "frontend"
--- superset-extensions-cli/tests/test_cli_build.py (lines 434-440) ---
434: @pytest.mark.unit
435: def
test_rebuild_backend_calls_copy_and_shows_message(isolated_filesystem):
436: """Test rebuild_backend calls copy_backend_files and shows
success message."""
437: - from superset_extensions_cli.cli import rebuild_backend # noqa:
PLC0415
438: +
439: # Create extension.json
440: extension_json = {
```
</div>
</details>
</div>
<small><i>Code Review Run #86bbd8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/utils/rls.py:
##########
@@ -84,7 +84,7 @@ def get_predicates_for_table(
table must be fully qualified, with catalog (null if the DB doesn't
support) and
schema.
"""
- from superset.connectors.sqla.models import SqlaTable
+ from superset.connectors.sqla.models import SqlaTable # noqa: PLC0415
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>PLC0415 noqa missing justification</b></div>
<div id="fix">
The pre-commit hook for PLC0415 explicitly requires a justification when
adding `# noqa: PLC0415`. The policy states: 'new code must either move the
import to the top or add the same noqa with a justification.' Without
justification, future maintainers won't understand why this import is deferred.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/utils/rls.py (lines 84-90)---
84: table must be fully qualified, with catalog (null if the DB
doesn't support) and
85: schema.
86: """
87: - from superset.connectors.sqla.models import SqlaTable # noqa:
PLC0415
87: + from superset.connectors.sqla.models import SqlaTable # noqa:
PLC0415 - avoid circular import with database models at module load time
88:
89: # if the dataset in the RLS has null catalog, match it when using
the default
90: # catalog
```
</div>
</details>
</div>
<small><i>Code Review Run #86bbd8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/utils/rls.py:
##########
@@ -158,7 +158,7 @@ def collect_rls_predicates_for_sql(
(kept consistent with what's actually applied at query time).
:return: List of RLS predicate strings that would be applied
"""
- from superset.sql.parse import SQLScript
+ from superset.sql.parse import SQLScript # noqa: PLC0415
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>PLC0415 noqa missing justification</b></div>
<div id="fix">
The pre-commit hook for PLC0415 explicitly requires a justification when
adding `# noqa: PLC0415`. The policy states: 'new code must either move the
import to the top or add the same noqa with a justification.' Without
justification, future maintainers won't understand why this import is deferred.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/utils/rls.py (lines 158-164)---
158: (kept consistent with what's actually applied at query time).
159: :return: List of RLS predicate strings that would be applied
160: """
161: - from superset.sql.parse import SQLScript # noqa: PLC0415
161: + from superset.sql.parse import SQLScript # noqa: PLC0415 -
SQLScript depends on database engine which may not be initialized at module load
162:
163: try:
164: parsed_script = SQLScript(sql,
engine=database.db_engine_spec.engine)
```
</div>
</details>
</div>
<small><i>Code Review Run #86bbd8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
