[vscode] fix fetchTasks to no longer filter modified configured tasks

[allaVolkov]

related to: #7681

Signed-off-by: Alla Volkov [email protected]

What it does

fetchTasks filters out some configured tasks from the tasks.json file.
It happens when an auto detected task is configured and some of its properties are then changed in the tasks.json file.

Management of the tasks from the tasks.json file separates them into two groups:

• customized tasks
• configured tasks

When fetchTasks is executed, the heuristic looks for matched original auto detected task for each customized task. The search is strict and compares each property from the task definition. In the case of a modified task, the match fails, and the task is filtered out. The PR resolves this problem by assigning these tasks to the group of configured tasks instead.

How to test

1. Clone https://github.com/allaVolkov/test-echo-task-provider which is a VS Code extension that registers echo task provider.
2. npm install
3. npm run compile
4. vsce package
5. Deploy the result vsix file to theia
6. Run theia
7. Select command RunTasks… from the command palette
   Observe 2 auto detected tasks:

• echo: task 1
• echo: task 2

8. Configure task 1
9. Update created tasks.json file:

• change label to task 3
• change text to Configured text

10. Select again command RunTasks… from the command palette
11. Check the list of tasks:  

• task 3
• echo: task 1
• echo: task 2

Prior to this PR task 3 was missing in the list of tasks. In VS Code it was listed.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[allaVolkov]

@akosyakov
@RomanNikitenko
@tsmaeder

Whom please I can contact to start reviewing this PR?

Thank you

[vince-fugnitto]

I reviewed the changes superficially for the moment, please address the test since we should not be disabling the rules (it leaves us with incorrect formatting, suppressed potential warnings and errors). Also, please squash your commits (into a single commit) when you are ready for another round of review.

[tsmaeder]

How can we be sure that the label field is not undefined here?

[allaVolkov]

I agree with your concern and maybe I have to add a check here.
The reason of such usage of label field is caused by the following:

PR modifies method getTasks.
This method processes 2 groups of tasks:

• taskCustomizationMap
• tasksMap

These groups are created in the method reorganizeTasks. As you can see, the existing code:

theia/packages/task/src/browser/task-configurations.ts

Line 407 in d7f9163

addConfiguredTask(scopeKey, transformedTask['label'] as string, transformedTask);

uses the field label without additional check.
If task is not detected it's assigned to tasksMap group assuming it has field label.
Don't we miss the check already here?

[tsmaeder]

When fixing a bug, please add a link to the issue for easy reference.

[allaVolkov]

done

[tsmaeder]

As I understand the code, this will still remove any tasks of type "shell", etc (which don't have a task definition) that have the same label (since we're using a map by label). Is the behaviour after this change aligned with VS Code as described in #7672 (comment) ? If I add the following to a tasks.json file:

        {
			"label": "My Task",
			"type": "shell",
			"command": "echo Hello",
			"problemMatcher": []
		},
		{
			"label": "My Task",
			"type": "shell",
			"command": "echo Hello",
			"problemMatcher": []
		}
}

and then do "run tasks", I see two tasks, as I read the code here, we would only get one task.

[allaVolkov]

You are right, the purpose of this PR is not to cover all delta of the fetchTasks between Theia and VSCode.
It covers only specific case that we run into, implementing VSCode extension with task provider.

[lidortal]

Any update on this PR?

[tsmaeder]

I'm not sure this PR does the right thing: according to https://code.visualstudio.com/docs/editor/tasks#_customizing-autodetected-tasks, an entry in a tasks.json file must be either of type shell or process or otherwise customize an auto-detected task. So having a task in task.json that does not fulfill either condition could also be considered an error. I lack the time right now to work myself into the tasks system once more in order to form an opinion.

I've opened #10850 exactly because we don't really have a good handle on questions like this. I would vote for keeping this PR open unmerged. When we attack the tasks once more, we can decide for/against it.

[vince-fugnitto]

@tsmaeder the pull-request is over a year old and based on your last comment you don't believe it is going in the right direction. Is it safe to close the issue to keep the list of open pull-requests more manageable?

[tsmaeder]

As much as I hate doing this, I think the likelihood of this one getting merged any time soon is low.