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

search detected tasks by scope #6718

Merged
merged 1 commit into from
Dec 10, 2019
Merged

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Dec 8, 2019

This pull request adds the support of detected tasks that have

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

How to test

  1. Prepare a multi root workspace. Make sure two or more root folders contribute detected tasks that have the same name. In the GIF below, both of my root folders (i.e., test-dir and test-resource_B have npm: myWatch defined in their package.json files)

  2. Run npm: myWatch from root folder test-dir.

  3. Check which npm: myWatch was started. In the GIF below, I started npm: myWatch in test-dir, and the task history and populated messages showed that the task was started in the right root folder.

Peek 2019-12-07 23-45

Review checklist

Reminder for reviewers

@elaihau
Copy link
Contributor Author

elaihau commented Dec 8, 2019

This one is created as a draft PR, because the solution I proposed here includes a breaking change, and I am not sure if there is a better way to fix the bug.

Also, I suspect this change would break CHE badly.

Thank you in advance for the feedback !

@RomanNikitenko
Copy link
Contributor

@elaihau
I didn't investigate your changes deeply, but tested for che tasks and it works well.

@elaihau
Copy link
Contributor Author

elaihau commented Dec 9, 2019

Thank you Roman, I will do more testing later today and finalize the change.

@elaihau elaihau force-pushed the Liang/same_label_detected_task branch from f42682d to eaeb26a Compare December 10, 2019 03:50
@elaihau elaihau marked this pull request as ready for review December 10, 2019 03:51
This pull request adds the support of detected tasks that have
- same label, and
- different scopes
in a multi-root workspace. This change fixes #6715.

Signed-off-by: Liang Huang <[email protected]>
@elaihau elaihau force-pushed the Liang/same_label_detected_task branch from eaeb26a to eea3332 Compare December 10, 2019 04:40
@elaihau
Copy link
Contributor Author

elaihau commented Dec 10, 2019

Ready for review.

@vince-fugnitto vince-fugnitto added multi-root issues related to multi-root support tasks issues related to the task system labels Dec 10, 2019
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.

Tested on Ubuntu 16.04
Use Electron and Chrome to test
There are breaking changes in code, but with the Theia extensions, it works fine
DON"T know if any side-effect with some VSCode plugins, I don't have any VSCode plugins using any detected TASKS ?

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Tested in a multi-root workspace for tsc tasks: the bug is fixed for me!
che tasks work well.

@elaihau thank you!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

It looks and works fine to me 👍

@elaihau
Copy link
Contributor Author

elaihau commented Dec 10, 2019

Thank you for your reviews !

@elaihau elaihau merged commit d5f0fcf into master Dec 10, 2019
@elaihau elaihau deleted the Liang/same_label_detected_task branch December 10, 2019 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-root issues related to multi-root support tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theia could possibly start wrong detected task in a multi root workspace
4 participants