Allow multiple languages in settings key

[CEbbinghaus]

This PR fixes #51935. Allowing for a single definition for multiple different languages is going to eliminate unnecessary config duplication.

{
    "[javascript, typescript]": {
       "editor.defaultFormatter": "esbenp.prettier-vscode"
    }
}

vs

{
    "[javascript]": {
       "editor.defaultFormatter": "esbenp.prettier-vscode"
    },
    "[typescript]": {
       "editor.defaultFormatter": "esbenp.prettier-vscode"
    }
}

Unfortunately, I haven't been able to do extensive testing due to having troubles getting a dev environment up and running but the unit tests in place right now are still succeeding.

A few items that still need doing:

• Update Schema definition for settings.json to accommodate these changes
• Add additional unit tests for these new possibilities.
• Possibly provide a warning if the same value is specified twice, once in a joined definition and once in a separate one.

Any feedback is welcome 🙂

[ghost]

All CLA requirements met.

[anderium]

Based on the test already used for checking language specific settings I suggest adding this test to check if the latest:

	test('parse multiple resource languages settings override', () => {
		const testObject = new ConfigurationModelParser('settings');

		testObject.parse(JSON.stringify({ '[json, javascript]': { 'FolderSettingsModelParser.resource': 'resource', 'FolderSettingsModelParser.resourceLanguage': 'resourceLanguage' }, '[json]': { 'FolderSettingsModelParser.resourceLanguage': 'overrideResourceLanguage' } }), { scopes: [ConfigurationScope.RESOURCE, ConfigurationScope.LANGUAGE_OVERRIDABLE] });

		const expected_json = Object.create(null);
		expected_json['FolderSettingsModelParser'] = Object.create(null);
		expected_json['FolderSettingsModelParser']['resource'] = 'resource';
		expected_json['FolderSettingsModelParser']['resourceLanguage'] = 'overrideResourceLanguage';

		const expected_javascript = Object.create(null);
		expected_javascript['FolderSettingsModelParser'] = Object.create(null);
		expected_javascript['FolderSettingsModelParser']['resource'] = 'resource';
		expected_javascript['FolderSettingsModelParser']['resourceLanguage'] = 'resourceLanguage';

		assert.deepStrictEqual(testObject.configurationModel.overrides, [{ 'contents': expected_json, 'identifiers': ['json'], 'keys': ['FolderSettingsModelParser.resource', 'FolderSettingsModelParser.resourceLanguage'] }, { 'contents': expected_javascript, 'identifiers': ['javascript'], 'keys': ['FolderSettingsModelParser.resource', 'FolderSettingsModelParser.resourceLanguage'] }]);
	});

This does probably require that [json, javascript] creates the settings for json first. I believe that's current behaviour, but perhaps it should not be a requirement. It doesn't test if a specific setting not occurring in the shared settings is added to contents. If the test succeeds FolderSettingsModelParser.resourceLanguage changed, this should only happen if extra settings would also be added. In case it should be tested specifically either another test is needed or an extra setting with one of the used scopes needs to added, and defined in the ConfigurationRegistry. (Perhaps using the already defined scopes and settings could be used for this, but I'm not 100% sure if that wouldn't give problems assuming only the RESOURCE and LANGUAGE_OVERRIDABLE scopes occur naturally where language settings occur.)

Also note that it seems the data structure was built to accommodate multiple language identifiers per override. (testObject.configurationModel.overrides[0]['identifiers'] is an array.) However, the current implementation makes it so that each language identifier has its own object with overrides. In case this is not desired behaviour the configurationModel class will have to be modified. (E.g. getKeysForOverride assumes identifiers only occur once.)

(Test based on:)

vscode/src/vs/workbench/services/configuration/test/common/configurationModels.test.ts

Lines 74 to 84 in 423f89b

	test('parse resource and resource language settings', () => {
	const testObject = new ConfigurationModelParser('settings');
	testObject.parse(JSON.stringify({ '[json]': { 'FolderSettingsModelParser.window': 'window', 'FolderSettingsModelParser.resource': 'resource', 'FolderSettingsModelParser.resourceLanguage': 'resourceLanguage', 'FolderSettingsModelParser.application': 'application', 'FolderSettingsModelParser.machine': 'executable' } }), { scopes: [ConfigurationScope.RESOURCE, ConfigurationScope.LANGUAGE_OVERRIDABLE] });
	const expected = Object.create(null);
	expected['FolderSettingsModelParser'] = Object.create(null);
	expected['FolderSettingsModelParser']['resource'] = 'resource';
	expected['FolderSettingsModelParser']['resourceLanguage'] = 'resourceLanguage';
	assert.deepStrictEqual(testObject.configurationModel.overrides, [{ 'contents': expected, 'identifiers': ['json'], 'keys': ['FolderSettingsModelParser.resource', 'FolderSettingsModelParser.resourceLanguage'] }]);
	});

[Wouterr0]

I know nothing about the inner workings of VScode and I was only looking at this PR because it seams like an awesome feature. But isn't the variable overrideIdentifiesr at line 159 and 161 of configurationModels.ts a misspelling? Anyway, great work 👍

[Wouterr0]

I know nothing about the inner workings of VScode and I was only looking at this PR because it seams like an awesome feature. But isn't the variable overrideIdentifiesr at line 159 and 161 of configurationModels.ts a misspelling? Anyway, great work 👍

[CEbbinghaus]

But isn't the variable overrideIdentifiesr at line 159 and 161 of configurationModels.ts a misspelling?

Indeed it is. I fixed one such misspelling already but this one has seemed to have escaped me. This is the largest project I have ever contributed to and its a little confusing finding what I'm looking for. It Will be fixed next commit

[sandy081]

Thanks for your interest in supporting this feature and providing a PR. I see that it is not completely implemented and there are some open questions like how to support writing into multiple languages key programatically. I assume it is not as trivial as changing a string type to string array type and adopting it every where.

I am planning to support this feature in this milestone (November 2021). Since it is not simple it might be time taking to get this feature contributed. So, I would like to implement this by myself so that things will move faster.

Edit: So, I would build on top of your PR and finish this feature.

Thanks for understanding.

[CEbbinghaus]

Thanks for your interest in supporting this feature and providing a PR. I see that it is not completely implemented and there are some open questions like how to support writing into multiple languages key programatically. I assume it is not as trivial as changing a string type to string array type and adopting it every where.

I am planning to support this feature in this milestone (November 2021). Since it is not simple it might be time taking to get this feature contributed. So, I would like to implement this by myself so that things will move faster.

Sorry i have been a little inactive. Between crunch at work and very little free time in general it has had to take a back seat. some of the changes have already been added and the unit test was added although my logic was flawed and I decided to figure out what the ideal implementation would be. Since this has taken too long I understand if you want to just take over from here. However I am still very keen to keep working on it even if its not full time.

Hoping I can help a little more than I already have

[sandy081]

Thanks for offering help. I have already pushed some changes/refactorings which make it easy to implement this feature. I will be finishing the implementation today with tests and also taking care of most of the other gaps. Since changes are going orthogonal, I would like to close this PR. Again, thanks for the interest and help.