codeant-ai-for-open-source[bot] commented on code in PR #36768:
URL: https://github.com/apache/superset/pull/36768#discussion_r2635176925
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -479,6 +479,11 @@ def init(
(target_dir / "extension.json").write_text(extension_json)
click.secho("✅ Created extension.json", fg="green")
+ # Create .gitignore
+ gitignore = env.get_template(".gitignore.j2").render(ctx)
+ (target_dir / ".gitignore").write_text(gitignore)
Review Comment:
**Suggestion:** Platform-dependent encoding: using Path.write_text() without
specifying an encoding can result in inconsistent behavior across environments;
specify an explicit encoding (UTF-8) when writing the .gitignore file.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
(target_dir / ".gitignore").write_text(gitignore, encoding="utf-8")
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Specifying encoding='utf-8' when calling Path.write_text is a small, sensible
hardening change that avoids surprises on systems with non-UTF-8 locales.
It's low risk and doesn't require broader refactors.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-extensions-cli/src/superset_extensions_cli/cli.py
**Line:** 484:484
**Comment:**
*Possible Bug: Platform-dependent encoding: using Path.write_text()
without specifying an encoding can result in inconsistent behavior across
environments; specify an explicit encoding (UTF-8) when writing the .gitignore
file.
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-extensions-cli/tests/test_cli_init.py:
##########
@@ -297,11 +297,35 @@ def test_init_command_output_messages(cli_runner,
isolated_filesystem, cli_input
# Check for expected success messages
assert "✅ Created extension.json" in output
+ assert "✅ Created .gitignore" in output
Review Comment:
**Suggestion:** Fragile test assertion: checking for the exact
emoji-prefixed string "✅ Created .gitignore" will fail if the CLI output omits
the emoji, uses a different emoji, or formats the message slightly differently;
assert a stable substring instead (e.g., "Created .gitignore") to avoid flaky
failures. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
assert "Created .gitignore" in output
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good catch — asserting the raw emoji can make CI flaky across terminals or
emoji normalization. Matching a stable substring like "Created .gitignore"
makes the test more robust without changing behavior. This is a low-risk,
high-value improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-extensions-cli/tests/test_cli_init.py
**Line:** 300:300
**Comment:**
*Possible Bug: Fragile test assertion: checking for the exact
emoji-prefixed string "✅ Created .gitignore" will fail if the CLI output omits
the emoji, uses a different emoji, or formats the message slightly differently;
assert a stable substring instead (e.g., "Created .gitignore") to avoid flaky
failures.
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-extensions-cli/tests/test_cli_init.py:
##########
@@ -297,11 +297,35 @@ def test_init_command_output_messages(cli_runner,
isolated_filesystem, cli_input
# Check for expected success messages
assert "✅ Created extension.json" in output
+ assert "✅ Created .gitignore" in output
assert "✅ Created frontend folder structure" in output
assert "✅ Created backend folder structure" in output
assert "🎉 Extension Test Extension (ID: test_extension) initialized" in
output
[email protected]
+def test_gitignore_content_is_correct(cli_runner, isolated_filesystem,
cli_input_both):
+ """Test that the generated .gitignore has the correct content."""
+ result = cli_runner.invoke(app, ["init"], input=cli_input_both)
+ assert result.exit_code == 0
+
+ extension_path = isolated_filesystem / "test_extension"
+ gitignore_path = extension_path / ".gitignore"
+
+ assert_file_exists(gitignore_path, ".gitignore")
+
+ content = gitignore_path.read_text()
Review Comment:
**Suggestion:** Reading file without an explicit encoding can produce
platform-dependent failures; specify an explicit encoding (utf-8) when calling
`read_text()` to ensure consistent behavior across environments. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
content = gitignore_path.read_text(encoding="utf-8")
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Specifying encoding="utf-8" removes platform locale variability and is a
sensible defensive change for tests reading generated files. Path.read_text
accepts encoding on supported Python versions used in typical CI.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-extensions-cli/tests/test_cli_init.py
**Line:** 317:317
**Comment:**
*Possible Bug: Reading file without an explicit encoding can produce
platform-dependent failures; specify an explicit encoding (utf-8) when calling
`read_text()` to ensure consistent behavior across environments.
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-extensions-cli/tests/test_cli_init.py:
##########
@@ -297,11 +297,35 @@ def test_init_command_output_messages(cli_runner,
isolated_filesystem, cli_input
# Check for expected success messages
assert "✅ Created extension.json" in output
+ assert "✅ Created .gitignore" in output
assert "✅ Created frontend folder structure" in output
assert "✅ Created backend folder structure" in output
assert "🎉 Extension Test Extension (ID: test_extension) initialized" in
output
[email protected]
+def test_gitignore_content_is_correct(cli_runner, isolated_filesystem,
cli_input_both):
+ """Test that the generated .gitignore has the correct content."""
+ result = cli_runner.invoke(app, ["init"], input=cli_input_both)
+ assert result.exit_code == 0
+
+ extension_path = isolated_filesystem / "test_extension"
+ gitignore_path = extension_path / ".gitignore"
+
+ assert_file_exists(gitignore_path, ".gitignore")
+
+ content = gitignore_path.read_text()
+
+ # Verify key patterns are present
+ assert "node_modules/" in content
+ assert "dist/" in content
+ assert "*.supx" in content
+ assert "__pycache__/" in content
Review Comment:
**Suggestion:** The test asserts the presence of "__pycache__/" exactly, but
some .gitignore files use "__pycache__" without a trailing slash; make the
assertion accept either form to avoid false negatives. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
assert ("__pycache__/" in content) or ("__pycache__" in content)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accepting either "__pycache__/" or "__pycache__" makes the assertion
tolerant to minor template formatting differences and avoids false negatives.
It's a small robustness improvement for the test.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-extensions-cli/tests/test_cli_init.py
**Line:** 323:323
**Comment:**
*Possible Bug: The test asserts the presence of "__pycache__/" exactly,
but some .gitignore files use "__pycache__" without a trailing slash; make the
assertion accept either form to avoid false negatives.
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-extensions-cli/src/superset_extensions_cli/templates/.gitignore.j2:
##########
@@ -0,0 +1,35 @@
+# Dependencies
+node_modules/
+
+# Build outputs
+dist/
+*.supx
+
+# Python
+__pycache__/
+*.py[cod]
+*$py.class
+*.egg-info/
+.eggs/
+*.egg
+.venv/
+venv/
+ENV/
+
Review Comment:
**Suggestion:** The virtual environment list includes `.venv/`, `venv/`, and
`ENV/` but omits the common lowercase `env/` directory name; on some setups a
virtualenv named `env/` would not be ignored and could be accidentally
committed. Add `env/` to the list to cover that case. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
env/
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Adding `env/` is harmless and correct — many users name their venv `env/`
and it should be ignored. The current list misses that lowercase variant;
adding it reduces the chance of accidentally committing a virtualenv.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-extensions-cli/src/superset_extensions_cli/templates/.gitignore.j2
**Line:** 18:18
**Comment:**
*Possible Bug: The virtual environment list includes `.venv/`, `venv/`,
and `ENV/` but omits the common lowercase `env/` directory name; on some setups
a virtualenv named `env/` would not be ignored and could be accidentally
committed. Add `env/` to the list to cover that case.
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]
