Expand targets names while searching for matching build task #3009
Conversation
|
@elahehrashedi as requested I tried implementing expansion capability while searching for matching task. |
src/cmakeTaskProvider.ts
Outdated
|
|
||
| let taskTargets: string[]; | ||
| if (cmakeDriver) { | ||
| taskTargets = await expand.expandStrings(task.definition.targets, cmakeDriver.expansionOptions); |
There was a problem hiding this comment.
the function findBuildTask is being called from the CMkae Driver, it's better if we pass the expansion options as the input to findBuildTask, instead of fetching the active project and getting the driver instance.
There was a problem hiding this comment.
Done.
FYI - i defined expansionOptions as optional parameter (and code checks if it was passed before calling expandString) - just in case when expansion is not required at all.
|
@piomis thank you for your contribution. I tested the branch and it seems to be working. Please apply the changes to simplify the code. |
src/cmakeTaskProvider.ts
Outdated
| let taskTargets: string[]; | ||
| if (expansionOptions) { | ||
| taskTargets = await expand.expandStrings(task.definition.targets, expansionOptions); | ||
| if (task.definition.options?.cwd){ |
There was a problem hiding this comment.
I added cwd to be expanded too
src/cmakeTaskProvider.ts
Outdated
| @@ -237,12 +250,12 @@ export class CMakeTaskProvider implements vscode.TaskProvider { | |||
| } else { | |||
| // Search for the matching default task. | |||
| const defaultTask: CMakeTask[] = matchingTargetTasks.filter(task => task.isDefault); | |||
| if (defaultTask.length === 1) { | |||
| if (defaultTask.length >= 1) { | |||
| return defaultTask[0]; | |||
| } else { | |||
| // Search for the matching existing task. | |||
| const existingTask: CMakeTask[] = matchingTargetTasks.filter(task => !task.isTemplate); | |||
There was a problem hiding this comment.
I don't see a task.isTemplate in tasks after you fetch tasks using vscode.tasks.fetchTasks. You have to distinguish between the tasks that are fetched from the tasks.json file and the tasks that the task provider provides as a template by setting this value to true/false.
There was a problem hiding this comment.
@elahehrashedi
Yes. I mentioned this problem already.
#2936 (comment)
#2780 (comment)
But it was like that in your refactor.
There was a problem hiding this comment.
INstead of using vscode.tasks.fetchTasks we can write our own function to read the tasks in tasks.json to fetch the existing tasks, and use the function provideTasks to get the template tasks.
Here is a sample:
If you need help implementing this let me know and I will modify your branch.
There was a problem hiding this comment.
@elahehrashedi
I'm not sure if manual parsing of Json is good solution (honestly speaking I am not convinced that ad hoc task creation is good solution either).
First of all this will be very fragile in regards to format of Json.
Like I rised in comments - you have added isTemplate property previously but your code never sets it to true. If you point me what place was intended to have set then I will add it.
By the way I found one comment from maintainers of VS code which suggests not to use ad hock tasks (microsoft/vscode#59337 (comment)) maybe an Original proposal with 'target' retrieval during task execution (instead of having target set by default when task is created). Potentially this could also allownto use 'shell' task type.
By the way I have just discovered that ProblemMatchers stopped working. And this is also related to fact that it is not possible to programicaly define non standard problem matcher in Task constructor.
|
@bobbrow this can be checked in. |
return matchingTargetTasks[matchingTargetTasks.length];Should it be length - 1? Arrays are 0-indexed, so I would expect this to be out of bounds. |
This change fixes #2970