Add Move to Schema command for database project files - #22705
Add Move to Schema command for database project files#22705Sai Avishkar Sreerama (ssreerama) wants to merge 5 commits into
Conversation
Sai Avishkar Sreerama (ssreerama)
linked an issue
There was a problem hiding this comment.
Pull request overview
Adds a “Move to Schema…” context-menu command for SQL Database Projects object files, delegating the action to the existing MSSQL Move to Schema refactor flow by passing a file path and resolving an appropriate cursor position in the MSSQL provider.
Changes:
- Contribute and register
sqlDatabaseProjects.moveToSchemaand surface it in the Database Projects tree context menu. - Forward SQL object script file paths from sql-database-projects to the MSSQL
mssql.moveToSchemacommand. - Extend the MSSQL Move-to-Schema provider to accept a file path entry point and add unit coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| localization/xliff/sql-database-projects.xlf | Adds localized string entry for “Move to Schema…”. |
| extensions/sql-database-projects/src/controllers/projectController.ts | Adds handler that validates a tree node and forwards the file path to mssql.moveToSchema. |
| extensions/sql-database-projects/src/controllers/mainController.ts | Registers the new sqlDatabaseProjects.moveToSchema command. |
| extensions/sql-database-projects/src/common/constants.ts | Adds constant for the forwarded MSSQL command ID (mssql.moveToSchema). |
| extensions/sql-database-projects/package.nls.json | Adds display name string for the new command. |
| extensions/sql-database-projects/package.json | Contributes the new command and adds it to the tree item context menu. |
| extensions/mssql/test/unit/sqlSymbolRenameProvider.test.ts | Adds unit test for file-path entry point resolving position. |
| extensions/mssql/src/languageservice/sqlMoveToSchemaProvider.ts | Accepts path-based invocation and adds local SQL token parsing to determine the symbol position. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
PR Changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22705 +/- ##
===========================================
- Coverage 88.74% 77.76% -10.99%
===========================================
Files 340 423 +83
Lines 119800 135008 +15208
Branches 552 8497 +7945
===========================================
- Hits 106317 104987 -1330
- Misses 13483 30021 +16538
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
extensions/mssql/src/languageservice/sqlMoveToSchemaProvider.ts:216
- For the file-path entry point, showing
loc.noMovableSymbolAtCursoris misleading because the user didn’t invoke the action from a cursor position. Consider introducing a dedicated localized message for the file-based flow (e.g. "No object that can be moved to another schema was found in this file.") and using it here.
const position = this.findMoveToSchemaSymbolPosition(document);
if (!position) {
void vscode.window.showInformationMessage(loc.noMovableSymbolAtCursor);
return;
}
extensions/mssql/test/unit/sqlSymbolRenameProvider.test.ts:72
- The stubbed TextDocument.getText(range) implementation returns an incorrect substring when the range spans multiple lines (
lineText.slice(0, range.end.character)ignores the start position and all full lines). This can make tests pass/fail for the wrong reasons if any code under test requests multi-line text.
return lineText.slice(0, range.end.character);
| await this.applyMove(document, position, selected.label, schemas); | ||
| } | ||
|
|
||
| //#region Tree Entry Local Parsing (replaceable) |
There was a problem hiding this comment.
the whole region is replaceable if STS can give the sql object name from the parser, which script Dom already have. But, needed STS changes which can be done in another release cycle not at the ask mode time.
There was a problem hiding this comment.
This is just to provide a quality-of-life improvement, correct? Might be better just to hold this fix back from this release so that we can do it correctly (using STS/ScriptDom)
There was a problem hiding this comment.
Had a chat with Drew and this can be moved to other cycle as is not very urgent one.
have moved the issue to next cycle, updating the PR to draft (will close it and open a new one if required)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
extensions/mssql/src/languageservice/sqlMoveToSchemaProvider.ts:109
- The command handler now silently no-ops when invoked without a position (or with no args). Since this command is contributed publicly and can be invoked from the command palette / other extensions, it should fall back to the active editor selection (when available) rather than returning
undefinedwith no feedback.
typeof documentOrPath === "string"
? provider.runMoveToSchemaFromFilePath(documentOrPath)
: position
? provider.runMoveToSchema(documentOrPath, position)
: undefined,
extensions/mssql/test/unit/sqlSymbolRenameProvider.test.ts:73
makeMoveDocument.getText(range)returns an incorrect substring whenrangespans multiple lines (lineText.slice(0, range.end.character)ignores the start line/character). This can make tests pass with an unrealistic document stub and hide bugs in range handling.
return lineText.slice(0, range.end.character);
}),
extensions/sql-database-projects/src/controllers/projectController.ts:2376
- This error telemetry uses
TelemetryActions.move, which is already used for file/folder moves in the project tree (seemoveFilebelow). Reusing the same action for “Move to Schema” will conflate unrelated scenarios and make telemetry harder to interpret. Consider adding a dedicated telemetry action (e.g.moveToSchema) and emitting an action event on invocation (similar toconfigureCodeAnalysisSettings).
void vscode.window.showErrorMessage(utils.getErrorMessage(err));
TelemetryReporter.sendErrorEvent(
TelemetryViews.ProjectController,
TelemetryActions.move,
err,
);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
extensions/mssql/src/languageservice/sqlMoveToSchemaProvider.ts:216
runMoveToSchemaFromFilePathshowsnoMovableSymbolAtCursor, which is phrased specifically for a cursor-based invocation. When invoked from the Database Projects tree there is no cursor, so this message is misleading.
const document = await vscode.workspace.openTextDocument(vscode.Uri.file(filePath));
const position = this.findMoveToSchemaSymbolPosition(document);
if (!position) {
void vscode.window.showInformationMessage(loc.noMovableSymbolAtCursor);
return;
extensions/mssql/src/languageservice/sqlMoveToSchemaProvider.ts:266
findMoveToSchemaSymbolPositionreturnsundefinedimmediately when it finds a CREATE/ALTER + object type but the next token is missing. Returning inside the loop prevents scanning later statements in the file (e.g., if the first match is incomplete or parsing didn't line up with tokens).
const firstNameToken = tokens[objectTypeIndex + 1];
if (!firstNameToken) {
return undefined;
}
extensions/mssql/test/unit/sqlSymbolRenameProvider.test.ts:68
- The
TextDocument.getText(range)stub returns an incorrect substring when the range spans multiple lines (lineText.slice(0, range.end.character)). Even if current tests mostly use single-line ranges, this makes the helper fragile and can cause misleading failures if additional tests start requesting multi-line ranges.
getText: sandbox.stub().callsFake((range?: vscode.Range) => {
if (!range) return lineText;
if (range.start.line === range.end.line) {
return (lines[range.start.line] ?? "").slice(
range.start.character,
| * Tokenizes SQL text into identifiers and separators while skipping whitespace, comments, | ||
| * and string literals so statement detection ignores non-executable text. | ||
| */ | ||
| private tokenizeMoveToSchemaSql(text: string): MoveToSchemaSqlToken[] { |
There was a problem hiding this comment.
I don't see any unit tests that cover this method. Please add several, especially since we're (temporarily) relying on a complicated REGEX.
There was a problem hiding this comment.
Right, Added the test coverage for the regex method and also created a task for the scriptDom replacement and added as a todo comment in the code.
This PR: