refactor: Remove unimplemented APIs from @apache-superset/core #36952
Conversation
michael-s-molina
requested review from
eschutho,
geido,
kgabryje,
rusackas and
villebro
as code owners
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #4751c7Actionable Suggestions - 0Review 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
michael-s-molina
force-pushed
the
remove-core-apis
branch
from
a5f8319 to
b830268
Compare
![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 #9878bf
Actionable Suggestions - 1
-
superset-frontend/packages/superset-core/src/api/sqlLab.ts - 1
- Missing Documentation Update · Line 257-269
Additional Suggestions - 14
-
superset-frontend/src/extensions/ExtensionsStartup.test.tsx - 1
-
Unnecessary `any` type assertion · Line 100-100The test assertion uses `(window as any).superset.extensions`, but since the global `Window` interface is extended in ExtensionsStartup.tsx to include `superset`, the `any` cast is unnecessary and violates the project's TypeScript standards that prohibit `any` types.
Code suggestion
@@ -96,6 +96,6 @@ - expect((window as any).superset).toBeDefined(); - expect((window as any).superset.authentication).toBeDefined(); - expect((window as any).superset.core).toBeDefined(); - expect((window as any).superset.commands).toBeDefined(); - expect((window as any).superset.extensions).toBeDefined(); - expect((window as any).superset.sqlLab).toBeDefined(); + expect(window.superset).toBeDefined(); + expect(window.superset.authentication).toBeDefined(); + expect(window.superset.core).toBeDefined(); + expect(window.superset.commands).toBeDefined(); + expect(window.superset.extensions).toBeDefined(); + expect(window.superset.sqlLab).toBeDefined();
-
-
superset-frontend/src/core/sqlLab/index.ts - 11
-
Import Rename · Line 19-19The import alias rename from 'sqlLabType' to 'sqlLabApi' improves code clarity without changing behavior.
-
Destructuring Update · Line 50-50Destructuring update to use new alias 'sqlLabApi' instead of 'sqlLabType'.
-
Function Implementation · Line 104-106Replaced 'notImplemented' stub with 'extractBaseData' function implementation.
-
API Implementation · Line 215-216Implemented getCurrentTab function matching the API declaration.
-
API Implementation · Line 218-222Implemented getTabs function as per API.
-
Event Implementation · Line 273-282Implemented onDidQueryRun event handler.
-
Event Implementation · Line 284-294Implemented onDidQuerySuccess event.
-
Event Implementation · Line 296-305Implemented onDidQueryStop event.
-
Event Implementation · Line 307-318Implemented onDidQueryFail event.
-
Event Implementation · Line 321-331Implemented onDidChangeEditorDatabase event.
-
Event Implementation · Line 333-343Implemented onDidCloseTab event.
-
-
superset-frontend/packages/superset-core/src/api/index.ts - 2
-
No Issue · Line 31-31The comment update replaces a reference to a non-existent 'environment' module with 'extensions', which accurately describes the extensions API for managing extension lifecycle and metadata. This change maintains consistency with the available modules.
-
No Issue · Line 40-40The export update removes a reference to a non-existent './environment' module and adds './sqlLab', which exists and provides SQL Lab integration APIs. This prevents potential import errors and aligns exports with available modules.
-
Review Details
-
Files reviewed - 11 · Commit Range:
b830268..b830268- superset-frontend/packages/superset-core/src/api/environment.ts
- superset-frontend/packages/superset-core/src/api/index.ts
- superset-frontend/packages/superset-core/src/api/sqlLab.ts
- superset-frontend/src/core/authentication/index.ts
- superset-frontend/src/core/commands/index.ts
- superset-frontend/src/core/environment/index.ts
- superset-frontend/src/core/extensions/index.ts
- superset-frontend/src/core/index.ts
- superset-frontend/src/core/sqlLab/index.ts
- superset-frontend/src/extensions/ExtensionsStartup.test.tsx
- superset-frontend/src/extensions/ExtensionsStartup.tsx
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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 [email protected].
Documentation & Help
AI Code Review powered by
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
I'm not closely familiar with the stability/usefulness of SQL Lab APIs, but I agree we should start light and expand as needed, as opposed to starting heavy at the risk of breaking changes down the road.
2 participants
SUMMARY
To avoid exposing and committing to API definitions before they are implemented, this PR removes stub functions from
@apache-superset/core. Defining APIs prematurely locks us into contracts that may change once actual implementation begins.TESTING INSTRUCTIONS
CI should be sufficient.
ADDITIONAL INFORMATION