chore(extensions): clean up backend entrypoints and file globs#38360
chore(extensions): clean up backend entrypoints and file globs#38360villebro merged 4 commits intoapache:masterfrom
Conversation
villebro
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina and
rusackas
as code owners
dosubot
bot
added
change:backend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (96.03%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #38360 +/- ##
==========================================
+ Coverage 64.29% 65.09% +0.79%
==========================================
Files 1811 2489 +678
Lines 71491 123865 +52374
Branches 22775 28647 +5872
==========================================
+ Hits 45965 80625 +34660
- Misses 25526 41843 +16317
- Partials 0 1397 +1397
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| Extensions use standardized entry point locations: | ||
|
|
||
| - **Backend**: `backend/src/superset_extensions/{publisher}/{name}/entrypoint.py` |
There was a problem hiding this comment.
This structure would be a bit weird when working on an extension, each extension would have backend/src/superset_extensions directory right? Having this superset_extensions directory inside an extension seems wrong to me. Is my understanding flawed here?
There was a problem hiding this comment.
This convention is already part of the framework. Without this namespacing, an extension with the publisher pandas would be able to override functionality in the pandas dependency. This just ensures that all extensions are collision free, both among external libraries, but also other extensions.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #2c389c
Actionable Suggestions - 4
-
superset-extensions-cli/tests/test_templates.py - 1
- Parametrize decorator argument type error · Line 90-90
-
superset-extensions-cli/src/superset_extensions_cli/cli.py - 3
- Use of assert statement detected · Line 225-228
- Bug in file copying logic · Line 235-239
- Blind exception catch without specific type · Line 303-306
Review Details
-
Files reviewed - 8 · Commit Range:
825af7f..825af7f- superset-core/src/superset_core/extensions/types.py
- superset-extensions-cli/src/superset_extensions_cli/cli.py
- superset-extensions-cli/src/superset_extensions_cli/templates/backend/pyproject.toml.j2
- superset-extensions-cli/src/superset_extensions_cli/templates/extension.json.j2
- superset-extensions-cli/tests/test_cli_build.py
- superset-extensions-cli/tests/test_cli_init.py
- superset-extensions-cli/tests/test_cli_validate.py
- superset-extensions-cli/tests/test_templates.py
-
Files skipped - 1
- docs/developer_docs/extensions/development.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
|
|
||
| @pytest.mark.unit | ||
| @pytest.mark.parametrize( | ||
| "include_frontend,include_backend,expected_sections", |
There was a problem hiding this comment.
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.
Code suggestion
Check the AI-generated fix before applying
| "include_frontend,include_backend,expected_sections", | |
| ("include_frontend", "include_backend", "expected_sections"), |
Code Review Run #2c389c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| """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" | ||
|
|
There was a problem hiding this comment.
Line 227 uses assert which should be replaced with proper error handling. Use explicit validation with if statement and sys.exit() instead.
Code suggestion
Check the AI-generated fix before applying
| """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" | |
| # 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 = ( |
Code Review Run #2c389c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Same as previous reply - we have a validation step that ensures all the requirements here are satisfied.
There was a problem hiding this comment.
The suggestion replaces a bare assert pyproject with an explicit check that provides a clear error message and exits gracefully if pyproject.toml can't be read. This improves error handling in the copy_backend_files function, preventing unhandled AssertionErrors and ensuring consistent CLI behavior.
Code Review Agent Run #996f9dActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@villebro Previously, we accepted multiple entry points and now it's a single one. Is this intended? I thought entry points would just be another contribution type like tools, APIs, etc and you would just need to annotate a file instead of a convention. |
@michael-s-molina Yes, this is intentional. The single entrypoint mirrors our frontend module federation pattern where Initially, I did consider adding entrypoints as a contribution type. However, thinking about it more closely, an entrypoint isn't really a contribution type like APIs or tools - it's infrastructure that simply registers contributions or triggers arbitrary code you wish to execute during bootstrap time. The frontend doesn't have "entrypoint contributions" either, and multiple entrypoints felt redundant since you can simply import multiple modules within a single entrypoint. If the need does come up for these types of "eager code execution containers" we can definitely consider it, but in that case we should probably consider it on both the frontend and backend. TL;DR: the proposed design here harmonizes backend/frontend architecture while maintaining full flexibility through imports, and aims to minimize developer overhead. |
michael-s-molina
left a comment
michael-s-molina
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the additional context @villebro!
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
Simplifies extension development by establishing convention-over-configuration for backend entry points. Eliminates the need to configure
entryPointsarrays by requiring standard file locations instead.Before:
{ "backend": { "entryPoints": ["my_extension.entrypoint"], "files": ["backend/src/**/*.py"] } }After:
backendsection inextension.jsonbackend/src/superset_extensions/{publisher}/{name}/entrypoint.pyincludeandexcludeglob arrays (convention):backend/pyproject.tomlBreaking Changes
backendsection fromextension.jsonbackend/src/superset_extensions/{publisher}/{name}/entrypoint.pybackend/pyproject.toml:ADDITIONAL INFORMATION