Allow extensions to set configuration options.

[mitchdenny]

I've got an extension that I'm currently adding telemetry too so that I can better understand how people are using it, however I want to give users an easy way to opt out of this telemetry collection. My initial thoughts were that I would display a quick pick with two options (agree or disagree) and that I would then store their choice for later executions.

However, it looks like the configuration API for extensions doesn't support the option to set a configuration value. It would be great if I could do something like this:

let configuration = vscode.workspace.getConfiguration('ecdc');
configuration.set('collectTelemetry', false);

I'm sure other extensions would find this useful as well to help guide users to setting up sensitive defaults.

[jrieken]

@mitchdenny You can also use the extension state for storage (https://code.visualstudio.com/docs/extensionAPI/vscode-api#ExtensionContext)

[mitchdenny]

Thanks @jrieken. I'll look into that in the meantime. Are you considering allowing extensions to write settings or is using extension state the better approach for this kind of setting?

[jrieken]

Not decided yet. The model of today is that only the user can modify settings and there is lot of good about that, but also see some shortcomings... The extension state is meant for remembering a user choice, store some installing path etc. This also includes your use-case. Since it's a little touchy (collecting telemetry data) I can understand that you want a more prominent option to unset something like that.

[mitchdenny]

Thanks for getting back to me. Appreciate it.

[jrieken]

Closing as we believe the current story isn't too bad. User manually editor configuration options and extensions can persist settings etc globally/locally

[jrieken]

We have decided to open up that way - giving extensions the power to modify the configuration outweighs the risks.

[jrieken]

@egamma, @aeschli - I wonder if I should implement a restriction wrt setting configurations. For me it makes sense that you cannot write a setting which isn't defined anywhere, like "editor.foobar": true will throw unless some extension has contributed this using the contribution-point. What do you think?

[aeschli]

Given that our settings are just key-value pairs, why would we care about settings like 'foo.bar'.
We also have some hidden settings that aren't defined in the settings and there's always the the case that an extension is not present.
I'd rather worry about validating the type of the setting (string, number, boolean...)

[jrieken]

Given that our settings are just key-value pairs, why would we care about settings like 'foo.bar'.

Because they end up in a file which is supposed to be human readable, if an extension by accident fills it up with random stuff your configuration is effectively lost.

[jrieken]

There is a challenge with this. Generally, our configuration is flat key-value pairs. The catch is that keys can be dot-separated and that the configuration is an object-tree along those dots.

For instance, the setting search.exclude is a nested structure of JS objects { search: { exclude: ... } }. That brings in an ambiguity when the value part is also an object, like { search: { exclude: { '**/node_modules': true, '**/bar': true } } }.

For reading, we don't differentiate this. You can do getConfiguration('search').get('exclude.**/node_modules') === true or getConfiguration('search.exclude').get('**/node_modules') === true which I think is fair for reading but I am unsure how writing configurations should behave in such cases? Should we allow for partial changes of the value part when the key is actual key + value-key?

@bpasero @aeschli ideas?

[bpasero]

Btw I would find it a good thing if an extension could actually write to extensions it does not contribute because this opens the door for a whole new family of extensions that could provide added value for changing settings (e.g. toggle core settings via command palette or ask the user if he is a tabs user or not and then make all the settings changes to disable or enable tabs).

As for writing settings, not sure. I think the main use case is to write a value for a setting that shows up in user settings. If there is search.exclude you should be able to write to this value using search.exclude and not via search or search.exclude.when. There are (unfortunately) some keys that have 2 dots (e.g. html.format.indentInnerHTML) and again I would expect to write html.format.indentInnerHTML via the API to provide a value.

Now, I am not sure if you could enforce this at all given that in the end all of these things end up being one large object.

Unrelated: would this API take into consideration that there are global settings and workspace settings?

[jrieken]

Btw I would find it a good thing if an extension could actually write to extensions it does not contribute because

There is a misunderstanding. Yes, I do want to allow extensions to write any setting - given it exists. That is the foobar extension can reconfigure search/tabs/etc but it cannot introduce a new config option. When thinking of key-value-pairs, you can set the value of any key but you cannot introduce a new key. Only the static contribution point in package.json can do that.

Unrelated: would this API take into consideration that there are global settings and workspace settings?

Yes. Current thinking is that there WorkspaceConfiguration#update which takes the key, a value, and some sort of hint where to write to - which again is complex.

The problem with writing is that I'd like to know if for a given key, object is a valid value, as it is for search.exclude. That's cos generally I don't think it wise to have an object as arbitrary value, cos it would allow for config.update('search', {}) which results in an invalid config... Or on a second though, the config service (setWorkspaceConfiguration, setUserConfiguration) should maybe just reject changes that are not compliant to the schema. @aeschli Can we already do that?

[aeschli]

...reject changes that are not compliant to the schema

Right now the configuration service doesn't know anything about the configuration schemas, but it looks like it should.
It should know the real key names (a.b.c). When collecting the default values we should also collect the allowed types and enum values. There are more ways of defining restrictions with JSON schema, but that would require the full schema checking code that is now in the JSON extension.

[jrieken]

That would be deluxe ;-) Similar to other places in the API we would apply a change always locally, send it to the main side, and update the extension side according to what it did (we do the same for instance with editor selection).

[bpasero]

Yes, +1 for not introducing new keys to the setting. I also see the issue with validation so for sure, the API needs to run the same validation checks we see in the editor and reject invalid updates.

There is another complexity: today we have settings.json, tasks.json and launch.json that all go through the configuration service. We end up merging tasks.json and launch.json (actually any *.json) into the global config by taking the name of the file as the first part (so that debug can do getConfiguration("launch") and getting all the configured settings). If we allow to write, what happens if I use a key from task or debug land? It would need to end up in the right file. Alternatively we would have to prevent and reject writing to launch or debug at all.

The global vs workspace configuration change is tricky because an extension might only write to the global config but the user might have workspace config that actually overwrites this setting.

[jrieken]

The global vs workspace configuration change is tricky because an extension might only write to the global config but the user might have workspace config that actually overwrites this setting.

I knew that but the task.json and launch.json is new to my. My initial reaction is to not allow editing those via the API.

[bpasero]

@jrieken also ugly, but less hardcoded: this all happens in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/configuration/common/model.ts#L54 and the idea is that any JSON file in the .vscode folder is being looked at. Contents with settings.json are being put top level, any other file contents is being added under a key that matches the name of the file. If you have tasks.json with a key foo it ends up to be tasks.foo in the getConfiguration call.

[bpasero]

Rethinking how this could work for task and debug I think these are new ConfigurationTargets for the writeConfiguration call and should not be done in an implicit way: Debug and task should be able to tell the configuration editing service that there are new targets for configuration values by providing the full path to this file. When calling into writeConfiguration you can use the contributed targets as well as the built in ones (USER, WORKSPACE).

Now, from an extension point of view I also think that this should be made very explicit in the API: If an extension can change launch configurations, there should be a namespace in the API to do so (same for tasks). Imho we should not implicitly write to tasks.json or launch.json if the settings key starts with task. or launch.

@isidorn @weinand fyi

[isidorn]

Keys in the launch.json are contributed by each extension, vscode is aware of them by reading the package.json of that extension. So it does not make a lot of sense that VSCode should verify them since the extension contributes those keys in the first place.

[DanTup]

Quick question!

There are several comments in this thread that suggest we shouldn't be having settings that don't exist in the package.json. Currently we assumed this was a feature, eg. having "hidden" settings - we have a couple of settings that we (extension devs) turn on (such as logging traffic to the language service to a file, and I was thinking about feature toggles we don't want to show to users in the settings file/completion yet).

Are we just taking advantage of a lack of validation here and we should stop using settings that aren't declared in the package.json?

[bpasero]

@jrieken I added support to write to launch.json and tasks.json. Any configuration key that starts with launch.* or tasks.* ends up in those files in the workspace. There are some new behaviours:

• I throw a new error ERROR_INVALID_TARGET if someone tries to write to the global settings file when using launch.* or tasks.* as key
• there is no validation happening when using launch.* or tasks.* keys because those are not in our collection of configured schemas and default values
• I currently do not allow to replace the contents of the file (e.g. you cannot use launch as key and provide a rich object as the contents), so you always have to provide a key within the file. not sure if we need to revisit this based on the scenario from debug and tasks (ping @dbaeumer @weinand @isidorn). the workaround is to just call the write-API for each key you want to have in the file (though that does not allow you to remove any user defined setting)
• I currently also do not write to any other standalone settings file besides tasks.json and launch.json because this is very dangerous: an extension could decide to use other JSON files in .vscode folder to store data and we do not want to simply write to that file without knowing how the file is being used

Let me know if more is needed from the workbench.

[jrieken]

Any configuration key that starts with launch.* or tasks.* ends up in those files in the workspace.

Does it mean it's asymmetric? So when I have 'foo: true' in launch.json and launch.foo: false in settings.json. From my understanding the latter value (false) will be the effective value but it seems now that writing will change the other value. Is that correct/desired?

[bpasero]

@jrieken today it seems random who is winning in such a case. in my testing the 'launch.foo': false in settings.json is actually winning over the 'foo: true' in launch.json which seems somewhat unexpected.

I think when merging settings in our config model we should first take all settings from settings.json and then overwrite the ones found in any other JSON file.

[bpasero]

Pushed a change to ensure that any value in tasks.json or launch.json overwrites anything in settings.json under the same key.

[bpasero]

@weinand @isidorn @dbaeumer I pushed a change so that you can actually replace the entire contents when you use the tasks or launch key. This is only possible in those cases 👍

[isidorn]

@bpasero I just tried this and seems to work great! It would be lovely if we could format the launch.json / tasks.json / settings.json after editing it. Here is a small gif where I am adding a new configuration using the API after the hello world command

[bpasero]

@isidorn is that changing an existing launch.json or creating a brand new one?

[isidorn]

@bpasero that is changing an existing launch.json. When I create a brand new launch.json it is nicely formated.

[bpasero]

@isidorn actually I can reproduce and this seems to happen when you pass in a complex object to @aeschli method applyEdits https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/jsonFormatter.ts#L34

The formatting seems to be ok'ish preserved if you just change simple key-value pairs, but not for such larger objects.

As a workaround you could just get the configuration and set it fully because then I am applying it using JSON.stringify().

[isidorn]

@bpasero thanks for the workaround!

[leonardovilarinho]

Hi @bpasero or @isidorn ,

You can show me how you've solved the problem. In my extension I want to get all the keys from settings.json and replace where there is a flag of my extension. It is possible?