Option to automatically disable language service on detected Non-TSQL files #18550
Conversation
laurennat
requested review from
kburtram,
aasimkhan30,
cssuh,
caohai,
lewis-sanchez and
Benjin
as code owners
PR Changes
|
|
Hi @laurennat just a quick comment about the prompt. Could you please update the text to say "non-SQL Server" instead of "MSSQL". Another option is to say "non-TSQL" as T-SQL is SQL Server SQL language name. |
| * Called by VS Code when a text document is changed. | ||
| * @param change The change event that was detected | ||
| */ | ||
| public async onDidChangeTextDocument( |
There was a problem hiding this comment.
Please add some unit tests for this code in a follow up PR.
| SwitchUnset = 2, | ||
| } | ||
|
|
||
| export function hasNonTSqlKeywords( |
There was a problem hiding this comment.
Good candidate for some unit tests.
| // This condition is true if there are any errors in the document | ||
| // from another sql language service | ||
| sources.length || | ||
| docErrors.length >= 50 || |
There was a problem hiding this comment.
Do we want to make this number configurable?
| * and PostGreSql documentation that are not present in the TSQL documentation | ||
| * of keywords. | ||
| */ | ||
| export const keywords: Set<string> = new Set([ |
There was a problem hiding this comment.
What if these keywords appear in object names? It's possible for keywords like 'cost' to be used as object names. With the current logic, this might trigger the prompt. I'm not sure what the best solution would be, but it's something to consider.
There was a problem hiding this comment.
Maybe we should trim some likely candidates for object names from this list?
| @@ -561,6 +561,9 @@ export let flavorDescriptionMssql = l10n.t( | |||
| export let flavorDescriptionNone = l10n.t( | |||
| "Disable intellisense and syntax error checking on current document", | |||
| ); | |||
| export let autoDisableNonTSqlLanguageServicePrompt = l10n.t( | |||
| "Possible non-SQL Server SQL file detected. Automatically disable Intellisense for all non-SQL Server SQL files?", | |||
There was a problem hiding this comment.
@croblesm, can you help refine this string to make sure the branding references are correct? Also, I feel like saying "SQL" so many times makes it a bit confusing.
There was a problem hiding this comment.
Looks like you already did in another comment; thanks!
| * and PostGreSql documentation that are not present in the TSQL documentation | ||
| * of keywords. | ||
| */ | ||
| export const keywords: Set<string> = new Set([ |
There was a problem hiding this comment.
Maybe we should trim some likely candidates for object names from this list?
| export function hasNonTSqlKeywords( | ||
| text: string, | ||
| nonTSqlKeywords: Set<string>, | ||
| ): boolean { |
There was a problem hiding this comment.
For some reason, I envisioned this happening inside the Parser itself or within STS, rather than at the VS Code layer. The Parser code is already parsing the entire document to identifying comments/literals/object names, and the STS code (see HandleSyntaxParseRequest) is enumerating errors.
I'm also concerned about the performance of using a slew of regexes on what could be a large script file. Have you tried this with a long script (e.g. a deployment script)?
PR for giving users an option to automatically disable the language service when the extension detects non T-Sql syntax/ keywords.
The bulk of github's detected line changes come from the added non t-sql keyword list.