Settings writer looses comments

[dbaeumer]

Have a setting defined that is an array of literal (an example is the tasks.json with the tasks property). Try to update the array using the settings API with comments used for elements inside the array or on elements in the array.

Observe: comments are lost

tasks.json

{
	// See https://go.microsoft.com/fwlink/?LinkId=733558
	// for the documentation about the tasks.json format
	"version": "2.0.0",
	"tasks": [
		{
			// a comment
			"type": "gulp",
			"task": "build",
			"problemMatcher": [
				"$tsc"
			]
		}
	]
}

configure a second task using the configuration UI (the gear icon in the task quick open). You get:

{
	// See https://go.microsoft.com/fwlink/?LinkId=733558
	// for the documentation about the tasks.json format
	"version": "2.0.0",
	"tasks": [
		{
			"type": "gulp",
			"task": "build",
			"problemMatcher": [
				"$tsc"
			]
		},
		{
			"type": "gulp",
			"task": "watch",
			"problemMatcher": []
		}
	]
}

I couldn't find a way to preserve the comment.

[sandy081]

@dbaeumer IConfigurationService.updateValue always replace the existing value with the given value. There is no smartness involved in it and I guess that smartness is not necessary for normal configurations.

Tasks to me looks like a special case. From configuration service perspective key is tasks and value is the complete tasks object. It does not know the semantics of the value object. Hence coming up with smartness is tough.

My suggestion is to directly write into the file by computing the edits using jsonEdit. This will allow you to do small edits like inserting just a single task.

It's also worth referring to jsonEditing.

[dbaeumer]

@sandy081 I have to say I disagree here. That would mean you limit the configuration to single values only. Even in a settings file I can easily define a setting that has an array of values which would be writable via our API and loose comments. So instead of me fixing this in Task land we should fix this in settings land. I agree that this is not easy but it is not easy on both sides :-)

[dbaeumer]

Just to clarify: in the given case I am not updating the while task settings. I am updating the tasks array. But there is no way for me to add, remove, ... elements from the array. I can only replace it.

[sandy081]

@dbaeumer I would say in Settings file most common way to comment is per setting. Commenting a sub portion of the value is very rare. Hence comments are retrieved while updating settings. Tasks from Configuration perspective, is just a single object with out any semantics known. But it is much more than that when you see Tasks as a feature.

Configuration writing API is simple and straight, update the existing value with given value for the given key. It does not mention about updating the sub section of the value which I think is your use case.

What I am saying is if your requirement is that you are programmatically adding a task to existing tasks, I would suggest to take a different route (as mentioned here) than using configuration service. It is not aligned with how it has to be used.

Since, this case which is very rare from Settings point of view, I am closing this.

[dbaeumer]

@sandy081 and I discussed this via Slack and I am reopening this for now. Will discuss how to continue on this face 2 face.

Just for the record: Using arrays in configs is not so uncommon. Examples are: tslint.rulesDirectory, tslint.exclude, tslint.autoFixOnSave. ESLint uses arrays as well.

[vscodebot]

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!

[dbaeumer]

@sandy081 thanks for reopening. Lets discuss this week. I have an idea how this could work . Basically we can use a path syntax that allows addressing array elements. Something like tasks.[2].label. That should not be so different than tasks.xxx.label

[dcharkes]

I've also ran into #37138. Any progress?

[weinand]

@sandy081 Loosing comments is a "user data loss" that we must avoid.
We need to fix this.

[Tarang74]

Any progress on this issue?

[arsu-leo]

I would suggest to just create a field named "description" with string content. This way the writter will write it down as it reads it, you get a warning about a non allowed property but it's non breaking

[vscodenpa]

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!