Fix converting of task with custom type #5591
Conversation
| @@ -651,7 +651,8 @@ export function toTask(taskDto: TaskDto): theia.Task { | |||
| result.execution = getProcessExecution(taskDto as ProcessTaskDto); | |||
There was a problem hiding this comment.
just wondering: do we need to use types.ProcessExecution.is(execution) in the if-statement in line 650, since you added a "customType" ?
There was a problem hiding this comment.
thank you, I'll review it today
There was a problem hiding this comment.
@elaihau I updated the PR, please review.
about your question - we use types.ProcessExecution.is(execution) when we do converting from theia.Task to TaskDto, but we don't need the same when we converting from TaskDto to theia.Task. For this case we use check by type only.
It's because at converting theia.Task of 'process' type 'process' filed is converted to 'command' filed of TaskDto. ProcessTaskRunner recognizes task by type and expects command field at the moment.
RomanNikitenko
force-pushed
the
fixTaskConverting
branch
from
241d924 to
e9d530d
Compare
Signed-off-by: Roman Nikitenko <[email protected]>
RomanNikitenko
force-pushed
the
fixTaskConverting
branch
from
e9d530d to
3fb6500
Compare
|
@akosyakov @benoitf @evidolob |
|
@RomanNikitenko how do you test it? If @azatsarynnyy @elaihau already somehow verified changes i am fine with merging. No need to wait for others approves. One approve with a good explanation covering all concerns from https://github.com/theia-ide/theia/pull/5616/files is enough. |
I added 'How to test' section for my PR. |
| @@ -651,7 +651,8 @@ export function toTask(taskDto: TaskDto): theia.Task { | |||
| result.execution = getProcessExecution(taskDto as ProcessTaskDto); | |||
| } | |||
|
|
|||
| if (taskType === 'shell') { | |||
| const execution = { command, args, options }; | |||
| if (taskType === 'shell' || types.ShellExecution.is(execution)) { | |||
There was a problem hiding this comment.
I wonder whether we need similar check for taskType === 'process' above?
There was a problem hiding this comment.
I think - no, the reasons I wrote here. But let me know if I should to reconsider that.
There was a problem hiding this comment.
OK, thanks for testing changes and review!
What it does
The PR allows to fix converting of a task configuration with custom type which has 'execution' field.
How to test
Tests was provided within the PR.
toTask method is used by TaskProviderAdapter, so I provided
the simple sample to have ability to test my changes.
So you can:
The sample provides logs in 'Test Channel' of 'Output' tab.
So you can see a task configuration which is provided within
=== PLUGIN === provide task:It contains
executionsection.After task running you can see configuration for resolving within
=== PLUGIN === resolve task:It does not contain
executionsection for master branch but contains this section for branch with my changes.Signed-off-by: Roman Nikitenko [email protected]