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

Reorganize tasks when task schema is updated #6643

Merged
merged 1 commit into from
Nov 29, 2019
Merged

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Nov 27, 2019

What it does

Rely on onDidChangeTaskSchema event to reorganize tasks configurations.
Fixes #6640

How to test

  1. Use vs code extensions with tasks (like typescript, npm).
    Tasks should be available for running from Terminal -> Run task menu.
    Try to run them, configure and run, refresh page and run again.

  2. Please use my forked repo for testing, it contains:

  • the current PR changes
  • the test extension which allows to add/remove schemes for custom types
  • tasks with custom type for testing
  1. git clone https://github.com/RomanNikitenko/theia.git
  2. cd theia/ and git checkout reorganizeTasks.
  3. Build the project and run it.
  4. The project contains prepared tasks in tasks.json file, so please open the cloned project: File=>Open... and select the project.
  5. Go to Terminal => Run Task menu:
    you can see that only SHELL task is available for running
  6. Go to Help menu and select Add CHE task schema
    you can see that now CHE task is available for running as well as SHELL task.
  7. Try the same for EXEC task
  8. You can try to remove subschema for CHE task and EXEC task using Help menu
    you can see that the corresponding tasks are not available for running after that.

Review checklist

Reminder for reviewers

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

@akosyakov akosyakov added the tasks issues related to the task system label Nov 27, 2019
@akosyakov akosyakov added the json issues related to the json language label Nov 27, 2019
@elaihau
Copy link
Contributor

elaihau commented Nov 28, 2019

from the functionality point of view the problem is fixed with this change. I will do one more round of testing after Anton's comment is addressed.

Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

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

I tested in the example browser with NPM extension and Roman's test extension. It worked well.

@@ -38,6 +38,9 @@ export class JsonSchemaStore {
protected readonly onSchemasChangedEmitter = new Emitter<void>();
readonly onSchemasChanged = this.onSchemasChangedEmitter.event;

protected readonly onDidChangeSchemaEmitter = new Emitter<string>();
Copy link
Member

Choose a reason for hiding this comment

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

let's keep URI here, clients can do toString if it is required or check other properties

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code-wise looks good me

@elaihau I have not actually tested, could you have a look please?

@elaihau
Copy link
Contributor

elaihau commented Nov 29, 2019

dumb question for Roman:
This block of code shows that this.jsonSchemaStore.registerSchema() (in the catch block) is not always called.
that means, TaskSchemaUpdater.onDidChangeTaskSchemaEmitter wouldn't always fire when the schema gets updated. Is it OK?

@akosyakov
Copy link
Member

akosyakov commented Nov 29, 2019

@elaihau It will be fired from https://github.com/eclipse-theia/theia/pull/6643/files#diff-c34063f85d0ae9acca8b8841309dc4a1R55 whenever content of schema is updated.

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@elaihau
Copy link
Contributor

elaihau commented Nov 29, 2019

@elaihau It will be fired from https://github.com/eclipse-theia/theia/pull/6643/files#diff-c34063f85d0ae9acca8b8841309dc4a1R55 whenever content of schema is updated.

Got it. thank you for explaination. I have no more questions

@RomanNikitenko
Copy link
Contributor Author

@akosyakov @elaihau
thank you very much for review and testing!

I'm going to merge the PR.

@RomanNikitenko RomanNikitenko merged commit f85acd6 into master Nov 29, 2019
@RomanNikitenko RomanNikitenko deleted the reorganizeTasks branch November 29, 2019 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
json issues related to the json language tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom tasks are not available for running
3 participants