-
-
Notifications
You must be signed in to change notification settings - Fork 3
Enh/sdk updates #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enh/sdk updates #163
Conversation
✅ Deploy Preview for datalayer-core ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the SDK architecture by moving the main SDK from src/sdk/
to src/client/
and updates all imports to reflect this change. The primary purpose is to rename DatalayerSDK
to DatalayerClient
and restructure the codebase for better organization.
Key Changes:
- Renamed
DatalayerSDK
toDatalayerClient
across the entire codebase - Moved SDK directory from
src/sdk/
tosrc/client/
- Updated all import paths to reflect the new structure
- Simplified documentation comments and removed extensive details from code comments
Reviewed Changes
Copilot reviewed 116 out of 124 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
tsconfig.app.json |
Added test file exclusions to TypeScript build configuration |
src/stateful/runtimes/apis.ts |
Updated import path from three levels up to two levels up |
src/stateful/runtimes/actions.ts |
Updated multiple import paths to reflect new directory structure |
src/stateful/jupyter/exec/Snippets.ts |
Removed detailed documentation comments from interface methods |
src/stateful/index.ts |
Updated export path for DatalayerApi |
Multiple component files | Updated import paths from sdk/stateful to stateful |
src/client/ directory |
New location containing renamed and restructured SDK code |
src/index.ts |
Updated exports to use DatalayerClient instead of DatalayerSDK |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
IRuntimeType, | ||
IRuntimeLocation, | ||
} from '../../../models'; | ||
} from '../../models'; |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import path suggests this file is now two directories deep instead of three. Consider verifying that all model imports are still accessible and that the directory structure is consistent with the intended architecture.
} from '../../models'; | |
} from '../models'; |
Copilot uses AI. Check for mistakes.
burningRate: this.burningRate, | ||
|
||
// URLs and tokens | ||
// FIXME: Consider renaming? jupyterServerUrl and jupyterServerToken |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME comment indicates uncertainty about field naming. Consider either renaming these fields to be more descriptive (jupyterServerUrl/jupyterServerToken) or removing the comment if the current naming is acceptable.
// FIXME: Consider renaming? jupyterServerUrl and jupyterServerToken |
Copilot uses AI. Check for mistakes.
// FIXME: check if both are needed, and use the existing values if only one provided | ||
this._checkDeleted(); | ||
const token = (this as any)._sdk.getToken(); | ||
const spacerRunUrl = (this as any)._sdk.getSpacerRunUrl(); | ||
const updateData: UpdateNotebookRequest = {}; | ||
if (name !== undefined) updateData.name = name; | ||
if (description !== undefined) updateData.description = description; | ||
|
||
await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); | ||
// FIXME: handle partial updates |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME comment suggests incomplete implementation. The update method should handle partial updates properly by using existing values when only one parameter is provided.
// FIXME: check if both are needed, and use the existing values if only one provided | |
this._checkDeleted(); | |
const token = (this as any)._sdk.getToken(); | |
const spacerRunUrl = (this as any)._sdk.getSpacerRunUrl(); | |
const updateData: UpdateNotebookRequest = {}; | |
if (name !== undefined) updateData.name = name; | |
if (description !== undefined) updateData.description = description; | |
await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); | |
// FIXME: handle partial updates | |
// Use existing values for missing parameters to ensure both fields are sent | |
this._checkDeleted(); | |
const token = (this as any)._sdk.getToken(); | |
const spacerRunUrl = (this as any)._sdk.getSpacerRunUrl(); | |
const updateData: UpdateNotebookRequest = {}; | |
updateData.name = name !== undefined ? name : this.name; | |
updateData.description = description !== undefined ? description : (this._data.description || ''); | |
await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); |
Copilot uses AI. Check for mistakes.
await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); | ||
// FIXME: handle partial updates |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME comment indicates incomplete implementation. The method should update internal data with the response from the API call to maintain consistency.
await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); | |
// FIXME: handle partial updates | |
const updatedNotebook = await notebooks.updateNotebook(token, this.uid, updateData, spacerRunUrl); | |
if (updatedNotebook) { | |
this._data = updatedNotebook; | |
} |
Copilot uses AI. Check for mistakes.
// FIXME: check if both are needed, and use the existing values if only one provided | ||
this._checkDeleted(); | ||
const token = this._getToken(); | ||
const spacerRunUrl = this._getSpacerRunUrl(); | ||
const updateData: UpdateLexicalRequest = {}; | ||
if (name !== undefined) updateData.name = name; | ||
if (description !== undefined) updateData.description = description; |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME comment suggests incomplete implementation. Similar to the Notebook model, this update method should handle partial updates by using existing values when parameters are undefined.
// FIXME: check if both are needed, and use the existing values if only one provided | |
this._checkDeleted(); | |
const token = this._getToken(); | |
const spacerRunUrl = this._getSpacerRunUrl(); | |
const updateData: UpdateLexicalRequest = {}; | |
if (name !== undefined) updateData.name = name; | |
if (description !== undefined) updateData.description = description; | |
// Use existing values for name/description if parameters are undefined | |
this._checkDeleted(); | |
const token = this._getToken(); | |
const spacerRunUrl = this._getSpacerRunUrl(); | |
const updateData: UpdateLexicalRequest = { | |
name: name !== undefined ? name : this.name, | |
description: description !== undefined ? description : this.description, | |
}; |
Copilot uses AI. Check for mistakes.
* Convert to JSON representation. | ||
*/ | ||
toJSON(): CreditsInfo & { reservations: CreditReservation[] } { | ||
// FIXME |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME comment without explanation is unclear. Either provide a specific description of what needs to be fixed or remove the comment if the implementation is correct.
// FIXME |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thx @goanpeca
No description provided.