bito-code-review[bot] commented on code in PR #38360:
URL: https://github.com/apache/superset/pull/38360#discussion_r2877246188
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -217,17 +222,36 @@ def copy_frontend_dist(cwd: Path) -> str:
def copy_backend_files(cwd: Path) -> None:
+ """Copy backend files based on pyproject.toml build configuration
(validation already passed)."""
dist_dir = cwd / "dist"
- extension = read_json(cwd / "extension.json")
- if not extension:
- click.secho("❌ No extension.json file found.", err=True, fg="red")
- sys.exit(1)
+ backend_dir = cwd / "backend"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Use of assert statement detected</b></div>
<div id="fix">
Line 227 uses `assert` which should be replaced with proper error handling.
Use explicit validation with `if` statement and `sys.exit()` instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
# Read build config from pyproject.toml
pyproject = read_toml(backend_dir / "pyproject.toml")
if not pyproject:
click.secho("❌ Failed to read backend pyproject.toml", err=True,
fg="red")
sys.exit(1)
build_config = (
````
</div>
</details>
</div>
<small><i>Code Review Run #2c389c</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_templates.py:
##########
@@ -81,23 +81,16 @@ def
test_extension_json_template_renders_with_both_frontend_and_backend(
# Verify frontend section is not present (contributions are code-first)
assert "frontend" not in parsed
- # Verify backend section exists
- assert "backend" in parsed
- backend = parsed["backend"]
- assert backend["entryPoints"] == [
- "superset_extensions.test_org.test_extension.entrypoint"
- ]
- assert backend["files"] == [
- "backend/src/superset_extensions/test_org/test_extension/**/*.py"
- ]
+ # Verify no backend section in extension.json (moved to pyproject.toml)
+ assert "backend" not in parsed
@pytest.mark.unit
@pytest.mark.parametrize(
"include_frontend,include_backend,expected_sections",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Parametrize decorator argument type error</b></div>
<div id="fix">
Line 90: `pytest.mark.parametrize` first argument should be a `tuple` of
parameter names, not a string. Change
`"include_frontend,include_backend,expected_sections"` to `("include_frontend",
"include_backend", "expected_sections")` or use a tuple.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
("include_frontend", "include_backend", "expected_sections"),
````
</div>
</details>
</div>
<small><i>Code Review Run #2c389c</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/src/superset_extensions_cli/cli.py:
##########
@@ -217,17 +222,36 @@ def copy_frontend_dist(cwd: Path) -> str:
def copy_backend_files(cwd: Path) -> None:
+ """Copy backend files based on pyproject.toml build configuration
(validation already passed)."""
dist_dir = cwd / "dist"
- extension = read_json(cwd / "extension.json")
- if not extension:
- click.secho("❌ No extension.json file found.", err=True, fg="red")
- sys.exit(1)
+ backend_dir = cwd / "backend"
- for pat in extension.get("backend", {}).get("files", []):
- for f in cwd.glob(pat):
+ # Read build config from pyproject.toml
+ pyproject = read_toml(backend_dir / "pyproject.toml")
+ assert pyproject
+ build_config = (
+ pyproject.get("tool", {}).get("apache_superset_extensions",
{}).get("build", {})
+ )
+ include_patterns = build_config.get("include", [])
+ exclude_patterns = build_config.get("exclude", [])
+
+ # Process include patterns
+ for pattern in include_patterns:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Bug in file copying logic</b></div>
<div id="fix">
The current implementation splits patterns on '/' and constructs glob calls
that fail for patterns without '/' or starting with '**'. Simplify to use
backend_dir.glob(pattern) directly, which handles all standard glob patterns
correctly.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
for pattern in include_patterns:
for f in backend_dir.glob(pattern):
if not f.is_file():
````
</div>
</details>
</div>
<small><i>Code Review Run #2c389c</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/src/superset_extensions_cli/cli.py:
##########
@@ -272,6 +296,89 @@ def app() -> None:
def validate() -> None:
validate_npm()
+ cwd = Path.cwd()
+
+ # Validate extension.json exists and is valid
+ extension_data = read_json(cwd / "extension.json")
+ if not extension_data:
+ click.secho("❌ extension.json not found.", err=True, fg="red")
+ sys.exit(1)
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specific type</b></div>
<div id="fix">
Line 305 catches a blind `Exception`. Catch specific exception types (e.g.,
`ValueError`, `ValidationError`) instead to improve error handling.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
try:
extension = ExtensionConfig.model_validate(extension_data)
except (ValueError, TypeError) as e:
click.secho(f"❌ Invalid extension.json: {e}", err=True, fg="red")
````
</div>
</details>
</div>
<small><i>Code Review Run #2c389c</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]
