Allow not persistent changes for task before run

[mmorhun]

Signed-off-by: Mykola Morhun [email protected]

Description

This PR adds ability to run existing task with some temporary (one-time) changes.
It might be useful in some cases, especially when a parameter of task depends on some environment or UI state. For example, when task console command is the same, but path in which it should be run depends on current file opened in the editor, etc.

Depends on

#5313 (toTask method patch in type-converters.ts)

[mmorhun]

This pull request is ready for review again.

[akosyakov]

@mmorhun please rebase it, if you can now please let us know, maybe @elaihau can rebase it then and land

@elaihau does this PR pass all checks listed here: #5616 I cannot say anything from approves right now. If so, please merge it.

[benoitf]

FYI @mmorhun is on PTO for a couple of weeks

[akosyakov]

@benoitf thanks for letting now, @elaihau could you take care of rebasing this PR, do final checks and merging. You can just continue working on the same branch, but don't modify authorship of existing commits, only rebase them and add new if it is required.

[elaihau]

@benoitf @RomanNikitenko @akosyakov
as per the author described at the top of this thread, this PR depends on #5313 which has not been merged.

I will keep this one on hold for now.

[RomanNikitenko]

@mmorhun @elaihau @benoitf
As I mentioned here, to speed up the process I separated my PR, so I believe that this PR depends on #5591. If I understand correctly we can merge #5591 and then this PR.

[elaihau]

@RomanNikitenko thank you for the explanation

[elaihau]

i don't know the use case to thouroughly test this one.
therefore I tested the changes in TaskService.run() with a unit test.
@RomanNikitenko @benoitf could you please kindly advise what I could do to test the changes in other files, such as task-provider.ts & task-frontend-contribution.ts ?

[RomanNikitenko]

@RomanNikitenko @benoitf could you please kindly advise what I could do to test the changes in other files, such as task-provider.ts & task-frontend-contribution.ts ?

I think @mmorhun tested it for che infrastructure.
But let's try to get along how we can test it locally.

To test changes for TaskProviderAdapter we need register task provider and use resolveTask() method.
The changes for task-frontend-contribution.ts provide ability to run task with overridden configuration.
So for testing we can use a custom plugin, provide some task using plugin API, pass overridden configuration to 'run task' command and try to execute it. As result - overridden task should be executed, not the original configuration.

I provided 'How to test' section for my PR #5591 with reference for sample. It contains registered task, but execution of 'run task' command is absent. So it can be modified a little to test this PR.

I'm going to test this PR as I described above as well. So please let me know if I'm wrong or you have any questions.

btw: we need to update this branch to have this changes

[RomanNikitenko]

I created the sample to test this changes locally.
I registered 'Execute overridden task' command which runs task:run command and pass overridden arguments.
Please see code

It works well for me when I pass empty array with args for command for example:

But the configuration looks strange when args for command exist and I try to override them:

So for first case I have: command: 'yarn', args: [] before merge
and command: 'yarn', args: ['run', 'watch'] after merge

For second case I have: command: 'yarn', args: ['run', 'build'] before merge
and command: 'yarn', args: ['run', 'build', run', 'watch'] after merge

@mmorhun @evidolob
Could someone test this changes and please correct me if I'm doing something wrong.
thank you!

[westbury]

For second case I have: command: 'yarn', args: ['run', 'build'] before merge
and command: 'yarn', args: ['run', 'build', run', 'watch'] after merge

That's deepmerge. It's handling of arrays does not seem to be well-defined. See for example TehShrike/deepmerge#14

I would suggest avoiding deepmerge. Instead of providing overrides, provide a callback that is passed the task and modifies it. The override function would know whether the run should appear just once in the array or should occur before each of build and watch. This will not help if the overrides come directly from a json file, but presumably in the use-case for this PR the overrides are provided programatically.

[akosyakov]

@mmorhun @RomanNikitenko could someone rebase it? @elaihau @vince-fugnitto @evidolob Could someone test according to #5312 (comment) afterwards?

[mmorhun]

I will take a look in a few days

[RomanNikitenko]

@mmorhun @RomanNikitenko could someone rebase it?

I tried to rebase it, but the resolving conflict for task-service is not easy.
The PR provides ability to run task with overridden configuration, but in another PR the customization object was added to override the task config.

So, I think it would be better to wait for @mmorhun to review the approach

[mmorhun]

While my work on this PR was delayed, some parts for running tasks with customizations was added. So, I think, I should align my future steps in this PR with you @akosyakov @elaihau @RomanNikitenko @azatsarynnyy.
The problem which I want to solve in this PR is to be able to run a task of custom type with customization from within a plugin.
To reach my goal I need to do at least two improvements:

• Add ability to pass the customization as task:run command argument. I did this in current version of the PR, but now I need to consider existing mechanism of tasks customization. So my proposal is to add the optional third argument overrides into run method and merge it into already defined customizationObject.
• Sometimes from within a plugin is needed to replace some subfield of task configuration without knowing the full configuration. For example, I have a task config:

{
  "label": "some-task",
  "type": "my-custom",
  "propx": {
    "a": 1,
    "b": "2",
    "c": false
  }
}

and I want to set propx.b to, say, 9 without knowing a and c.
As for now, reaching that is possible via deepmerge on customization apply (probably here). I saw the problem with deepmerge which was mentioned above, but I believe we may set array strategy as replace and then new arrays will replace old ones if any.
Another, I think possible, way is to retrieve full task configuration, replace needed field and pass as amend. But the API isn't finished, so I have to wait for #5348

Would like to hear you opinion, concerns, suggestions.

[westbury]

I believe we may set array strategy as replace and then new arrays will replace old ones if any

Using 'replace' strategy would result in args disappearing in unintuitive ways.

Another, I think possible, way is to retrieve full task configuration, replace needed field and pass as amend

That is the only way to go. Full flexibility and a simple API with no scope for confusion.

[akosyakov]

@theia-ide/task-extension please help to move it forward

[elaihau]

@akosyakov
As Mykola and Nigel said, #5348 was needed to retrieve the full task configuration so that the plugin has the capability to amend a task.

#5348 was created by me. Right now it is blocked by #5975 not getting merged (changes in 5975 are needed to remove duplicated task configs in fetchTasks()).

I rewrote the fetchTasks() API based on the latest master and tested it last week.
Once #5975 gets merged I will update #5348 or create a new PR.

@mmorhun @RomanNikitenko if you have time, could you please review #5348, so that this PR and #5348 can be unblocked? Thank you !

[RomanNikitenko]

@elaihau

@mmorhun @RomanNikitenko if you have time, could you please review #5348, so that this PR and #5348 can be unblocked? Thank you !

Do you mean #5975 here instead of #5348?

Once #5975 gets merged I will update #5348 or create a new PR.

I reviewed #5975, please take a look.

[mmorhun]

Thank you all who took part in this PR, but the functionality proposed here was implemented meanwhile in this PR. So closing this one...