Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

align "configure task" and "task quick open" with vs code #5777

Merged
merged 1 commit into from
Aug 1, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Jul 23, 2019

Signed-off-by: Liang Huang [email protected]

@elaihau
Copy link
Contributor Author

elaihau commented Jul 23, 2019

Configure tasks in different root folders of a multi root workspace:

Peek 2019-07-23 00-47

Once a detected task is configured, it is displayed as a configured task:

Peek 2019-07-23 00-48

The configured detected task cannot be configured for more than once:

Peek 2019-07-23 00-50

Lastly, as the GIFs shown above, with this change, "configure task" does not write the entire task object into the tasks.json. It only writes the properties that are used to defined the task, and problemMatcher.

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems ok with minor comments
Can the 3 commits be squashed into one !
Not tested the runtime exec yet

@elaihau elaihau force-pushed the Liang/multi-root-ws-config-task branch from 477f7e8 to 396b329 Compare July 24, 2019 00:24
@elaihau elaihau changed the title align "configure task" with vs code align "configure task" and "task quick open" with vs code Jul 24, 2019
@elaihau
Copy link
Contributor Author

elaihau commented Jul 24, 2019

Code seems ok with minor comments
Can the 3 commits be squashed into one !
Not tested the runtime exec yet

Thank you for the review @lmcbout @vince-fugnitto !
I will also do more tests on my side.

@vince-fugnitto
Copy link
Member

@elaihau I don't have any issues codewise :)
Do you mind sharing on how we can successfully test the PR (mainly how to get detected tasks)?

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jul 24, 2019

I tested the changes locally using npm extension and can confirm that:

  • task after configuring is displayed as configured
  • no redundant fields are present after configuring a detected task
  • it works well for a multi-root workspace
  • user can configure a task only once

so, thank you @elaihau for the fix - well done!

I would like test the running a configuration from recently used section after configuring and so on.
Do you know some vs code extension which provides task configurations with args available for editing by user? I mean it's hard to reproduce the described case above for npm, because this one has only script and path fields. Maybe you know some extension with tasks which allow to edit command filed or args and can be good example for testing to check - is configured configuration is used for execution or detected one from recently used section.

thank you!

@elaihau
Copy link
Contributor Author

elaihau commented Jul 25, 2019

Maybe you know some extension with tasks which allow to edit command filed or args and can be good example for testing to check - is configured configuration is used for execution or detected one from recently used section.

thank you!

@RomanNikitenko Thank you for testing, really appreciated !

from my understanding in vs code the command property can not be customized. And this patch does not support customizing the command either.

As per our discussion in #5679 (comment), I should add support to allow users to override all properties such as command. Please give me two days (sorry I am not working full time on this) and I will make a patch to this PR.

@elaihau
Copy link
Contributor Author

elaihau commented Jul 25, 2019

@elaihau I don't have any issues codewise :)
Do you mind sharing on how we can successfully test the PR (mainly how to get detected tasks)?

We could install vs code extensions such as npm and grunt as plugins in theia (Jacques has a lot of experiences on that :)). the npm scripts and grunt tasks defined in your project would be detected by those extension and get displayed in the task quick open.

@elaihau elaihau force-pushed the Liang/multi-root-ws-config-task branch 2 times, most recently from 14b89f3 to 29dbe89 Compare July 28, 2019 02:22
@elaihau
Copy link
Contributor Author

elaihau commented Jul 28, 2019

@RomanNikitenko with the latest change in this patch you would be able to override any properties, except for type, and those that are used to define your detected task.

take npm tasks for example, from its task definition

      {
        "type": "npm",
        "required": [
          "script"
        ],
        "properties": {
          "script": {
            "type": "string",
            "description": "%taskdef.script%"
          },
          "path": {
            "type": "string",
            "description": "%taskdef.path%"
          }
        }
      }

we know script and path are used to define tasks, and therefore we could override any properties other than script, path, and type.

Please see this GIF for example

Peek 2019-07-27 22-21

npm lintAAA executes eslint . on the first root folder of the workspace. with users overrridding command, args, and options, it runs ./task script from the other root folder.

Please let me know if this supports what you needed. Thank you !

@elaihau elaihau force-pushed the Liang/multi-root-ws-config-task branch from 29dbe89 to 0fb0b45 Compare July 28, 2019 02:36
@RomanNikitenko
Copy link
Contributor

@elaihau
I'm going to test your changes today
Thank you very much!

@RomanNikitenko
Copy link
Contributor

@elaihau
I tested again cases mentioned here after PR is updated, also tested this case. It works well for me!

For our assembly we need some adaptation, but looks like it's on our side, so @elaihau thank you for giving me time for testing and analyzing!

- edit the right task.json when clicking "configure task" in multi-root
workspace (fixed #4919)

- in the current Theia, when users configure a detected task, the entire
task config is written into tasks.json, which introduces redundancy.
With this change, only properties that define the detected task, plus `problemMatcher`, are
written into tasks.json. (fixed #5679)

- allow users to override any task properties other than `type`, and those that are used to define the in its task definition.

- `TaskConfigurations.taskCustomizations` is a flat array, and the user can only customize one type of detected task in one way.
With this change Theia supports having different ways of task customization in different root folders.

- The detected tasks, once customized, should be displayed as configured tasks in the quick open. (fixed #5747)

- The same task shouldn’t have more than one customization. Otherwise it would cause ambiguities and duplication in tasks.json (fixed #5719)

Signed-off-by: Liang Huang <[email protected]>
@elaihau elaihau force-pushed the Liang/multi-root-ws-config-task branch from 0fb0b45 to 2fe7262 Compare August 1, 2019 03:04
@elaihau
Copy link
Contributor Author

elaihau commented Aug 1, 2019

Thank you for the review and testing @RomanNikitenko , really appreciated !

@elaihau elaihau merged commit e0d38ee into master Aug 1, 2019
@elaihau elaihau deleted the Liang/multi-root-ws-config-task branch August 1, 2019 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants