-
Notifications
You must be signed in to change notification settings - Fork 145
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
Adding completion extension loading and execution logic #833
Conversation
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.Hosting.v2/Extensibility/ExtensionServiceProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.Hosting.v2/Extensibility/ExtensionServiceProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/LanguageServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Contracts/Completion.cs
Show resolved
Hide resolved
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.
Overall it's looking good! Some changes I'd like to see made though and I'd like @kevcunnane to also do a quick run-over to see if the MEF stuff looks good but I think we're definitely getting there.
Note that the CI build is failing - looks like it can't build your CompletionExtSample project. Make sure you investigate and get that fixed before checkin. #Closed |
@kevcunnane , did you get a chance to review the latest code? @Charles-Gagnon , let me know when I can merge the change to master? #Closed |
test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/LanguageServiceTests.cs
Outdated
Show resolved
Hide resolved
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.
Looks good to me. I'll wait for @Charles-Gagnon to sign-off prior to merging since he has a number of comments pending.
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Contracts/Completion.cs
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
…ded at the first time or updated since last load.
...icrosoft.SqlTools.ServiceLayer/LanguageServices/Completion/Extension/ICompletionExtension.cs
Outdated
Show resolved
Hide resolved
...icrosoft.SqlTools.ServiceLayer/LanguageServices/Completion/Extension/ICompletionExtension.cs
Outdated
Show resolved
Hide resolved
...icrosoft.SqlTools.ServiceLayer/LanguageServices/Completion/Extension/ICompletionExtension.cs
Outdated
Show resolved
Hide resolved
...icrosoft.SqlTools.ServiceLayer/LanguageServices/Completion/Extension/ICompletionExtension.cs
Outdated
Show resolved
Hide resolved
...oft.SqlTools.ServiceLayer/LanguageServices/Completion/Extension/CompletionExtensionParams.cs
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
Outdated
Show resolved
Hide resolved
@kburtram, I added a list of TSQL functions into the default completion list, please review. |
"xact_abort", | ||
"xml", | ||
{ | ||
"abs", |
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.
I think adding built-in SQL functions is fine. Though I'm not sure how effective it will be. The way the IntelliSense works is that we first try to use the T-SQL parser to get suggestions. The parser will only provide schema-based suggestions (unfortunately it doesn't support keyword suggestions). Only if there are no schema-based suggestions then we return this default keyword list.
I suspect that most the code locations where a user would want to call a function they will likely have schema suggestions available (e.g. in column lists, where clauses, having clauses, etc). So the default completion list would not be provided.
There are couple exceptions, such as when the editor is not connected or when no tables have been listed in the from clause yet. But I suspect in the most common use-cases the functions still won't be suggested. Overall, I'm still fine adding them though in case they help in some cases.
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.
I see, the TSQL functions should merge with the schema-based suggestions after a space or comma.
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.
Overall looks good but I'd still like to see you add more tests - especially for the assembly loading logic.
No description provided.