bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3270352189
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -407,7 +408,7 @@ def validate_parameters(
try:
url = url.replace('"', '""')
- results = conn.execute(f'SELECT * FROM "{url}" LIMIT 1') #
noqa: S608
+ results = conn.execute(text(f'SELECT * FROM "{url}" LIMIT 1'))
# noqa: S608
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Loop variable overwritten by assignment</b></div>
<div id="fix">
The loop variable `url` is overwritten at line 410 within the loop body. Use
a different variable name (e.g., `escaped_url`) to avoid shadowing the loop
variable.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
try:
escaped_url = url.replace('"', '""')
results = conn.execute(text(f'SELECT * FROM "{escaped_url}"
LIMIT 1')) # noqa: S608
````
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</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
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned
`filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to
prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>filterwarnings order prevents always directive</b></div>
<div id="fix">
The bare `ignore` directive on line 29 will suppress all warnings before the
`always::sqlalchemy.exc.RemovedIn20Warning` directive on line 30 can match
them. Per pytest's filterwarnings behavior, the first matching directive wins.
This means the 'always' directive is effectively dead code — RemovedIn20Warning
messages will be silently ignored instead of being printed.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- pytest.ini (lines 25-31) ---
25: +# `ignore` virtually reproduces to `-p no:warnings`.
26: +# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
27: +# Additionally, raise errors for refactored RemovedIn20Warning cases
to prevent regression.
28: +filterwarnings =
29: + always::sqlalchemy.exc.RemovedIn20Warning
30: + ignore
31: + error:Passing a string to Connection.execute\(\) is
deprecated:sqlalchemy.exc.RemovedIn20Warning
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</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/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
The `reset_postgres_id_sequence` function is dead code — it is defined but
never called. The `text` import added at line 32 is only used by this unused
function. Both should be removed to avoid maintenance confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -29,7 +29,7 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import select, text
+from sqlalchemy import select
from sqlalchemy.exc import NoSuchModuleError
from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy.orm import backref, relationship, Session
@@ -889,16 +889,6 @@
from sqlalchemy.orm import aliased
-def reset_postgres_id_sequence(table: str) -> None:
- """Reset PostgreSQL sequence ID for a table's id column."""
- op.execute(
- text("""
- SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</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]
