Add changes for flavor selection #8419
Conversation
src/sql/workbench/services/connection/browser/connectionManagementService.ts
Show resolved
Hide resolved
src/sql/workbench/services/connection/browser/connectionManagementService.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts
Outdated
Show resolved
Hide resolved
Adding a boolean property to ConnectionProviderProperties for providers that are language flavors. When it is set to true, the provider will be part of drop down for changing SQL language flavor.
src/sql/platform/capabilities/test/common/testCapabilitiesService.ts
Outdated
Show resolved
Hide resolved
| @@ -21,6 +21,7 @@ export interface ConnectionProviderProperties { | |||
| providerId: string; | |||
| displayName: string; | |||
| azureResource?: string; | |||
| isLanguageFlavorProvider?: boolean; | |||
There was a problem hiding this comment.
Are there any connection providers that are not language flavor providers? I thought every connected editor would have the associated language dialect selected in the drop-down. Maybe we can get away without this extra property? Otherwise we will need to add it to MySQL extension too...
There was a problem hiding this comment.
Yes there is one MSSQL-CMS which is not a flavor provider. So adding this property in extension to distinguish.
|
This pull request introduces 1 alert when merging 4284f14 into ee94524 - view on LGTM.com new alerts:
|
Using Object.keys instead of entries as doing [0] can be error prone if no provider matches.
|
This pull request introduces 1 alert when merging 28cfad8 into c34869c - view on LGTM.com new alerts:
|
src/sql/workbench/services/connection/browser/connectionDialogService.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| // Remove duplicate listings (CMS uses the same display name) | ||
| let dedupMap = this._connectionManagementService.getDedupeConnectionProvidersByNameMap(this.providerNameToDisplayNameMap); | ||
| this._providerTypeSelectBox.setOptions(Object.keys(dedupMap).map(k => dedupMap[k])); |
There was a problem hiding this comment.
Hm, what's the type of dedupMap? Why do you need to map it again? Can't we just return the map from that get... method?
There was a problem hiding this comment.
dedupMap returns the object of type { [providerDisplayName: string]: string } and we only need display names in the box. So map is kinda like .select in c#
There was a problem hiding this comment.
I see, yeah, I thought it was actually generating a Map... Can't you just do Object.values if that's what you need?
There was a problem hiding this comment.
Nope, this is an object in typescript and you cannot access values directly. I am trying to see if I should return a Map instead as we will only be using this object for iteration/lookup. It will make accessing keys and values easier.
Looking into this: https://medium.com/front-end-weekly/es6-map-vs-object-what-and-when-b80621932373
src/sql/workbench/services/connection/browser/connectionManagementService.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/connection/browser/connectionManagementService.ts
Show resolved
Hide resolved
src/sql/workbench/services/connection/browser/connectionManagementService.ts
Outdated
Show resolved
Hide resolved
| @@ -672,6 +717,7 @@ export class ConnectionManagementService extends Disposable implements IConnecti | |||
| */ | |||
| public doChangeLanguageFlavor(uri: string, language: string, provider: string): void { | |||
| if (this._providers.has(provider)) { | |||
| this._uriToProvider[uri] = provider; | |||
There was a problem hiding this comment.
Hm... what is this mapping used for? I'm thinking if should be consistent with the actual connection, rather than language flavor. For example, if somebody connects an editor to MySQL, then that URI should be claimed by MySQL connection provider, no matter what language flavor user selects in the editor. What is the scenario where we need to map URI to the language flavor?
In reality this should always match connection anyway (since language flavor selection is not allowed), only for disconnected providers we should be setting this. Maybe keep the assignment in sendConnectRequest and override it here if user explicitly (somehow) changes it?
One thing I'm not sure about is what behavior do we expect, if user chooses the MySQL language flavor for disconnected editor and then connects it to SQL Server? I think it should reset to mssql, as at that time it's kind of guaranteed they will be SQL Server queries.
There was a problem hiding this comment.
Mapping is used for both connected as well as disconnected URI. It maps to the provider, so if connected it will get the provider id for the connection and set to that. Whereas if disconnected it will set to the flavor id that yo selected, which is turn is the provider id. When user does sendConnectRequest it will call this doChangeLanguageFlavor and we need to set the map to keep track of the flavor. Whereas this method will also be called when user explicitly sets it. So putting it at a common place. Yes that will be the expected behavior, if it is connected to a server, it will use that flavor.
There was a problem hiding this comment.
Yeah, that's what I thought - sendConnectRequest will call doChangeLanguageFlavor, it's just not that obvious. I was thinking maybe we should do it in both places, in case somebody changes it later and doesn't call doChangeLanguageFlavor when connecting - we should still have that URI (editor) mapped. But I see how that will be a code duplication, so not really sure how much risk is there to keep it in just one place...
Adding changes to allow user to select flavor for offline scripts.
By default the user setting is MSSQL, user can go to Preferences -> Setting -> Data -> SQL and change default engine value. If they add a provider which is not supported, it will show "Choose SQL Language" as flavor. User can then select a particular file and select the flavor value from list of providers.
If a valid value is set, it will default that as flavor for all the offline scripts. Online scripts will have flavor based on their connection. It would not allow you to change the flavor for connected scripts and give an info message.
This PR fixes #8417