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

add registered ProblemMatchers to tasks schema #6422

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Oct 21, 2019

Signed-off-by: Liang Huang liang.huang@ericsson.com

How to test

  • Open tasks.json and define a task.
  • Add "problemMatcher" to the defined task, and check if we are able to see the names of registered problem matchers.

Peek 2019-10-20 22-43

Review checklist

const toDispose = new DisposableCollection(Disposable.create(() => {/* mark as not disposed */ }));
this.doRegister(matcher, toDispose);
const toDispose = new DisposableCollection(Disposable.create(() => {
/* mark as not disposed */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akosyakov this is the part i am not sure about.
could you please give me more information on what /* mark as not disposed */ means here? Thank you !

Copy link
Member

@akosyakov akosyakov Oct 22, 2019

Choose a reason for hiding this comment

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

isDisposed returns true if a collection is empty. If we don't add anything on the initialization when checking it returns false, so we push an empty disposable object to indicate that it was not disposed yet.

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.

I verified if we now get content assistance when editing the tasks.json manually and it works correctly 👍
I also correctly get warned when attempting to provide unknown problem matchers.

return JSON.stringify(taskSchema);
}

private updateProblemMatcherNames(): void {
Copy link
Member

Choose a reason for hiding this comment

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

@elaihau do you mind documenting this method?
I see that we getAll named problem matchers, reset the current list, and finally push the new list.
I just wanted to understand why it's required and in what scenario.

Copy link
Contributor Author

@elaihau elaihau Oct 22, 2019

Choose a reason for hiding this comment

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

Yep i will document this function.

This function is called in the callback of this.problemMatcherRegistry.onDidChangeProblemMatcher( ). In other words, every time a problem matcher is registered or disposed, we want to update the task schema to ensure the users get the most up-to-date when they are manually entering the problem matcher(s).

I cannot simply do problemMatcherNames = matcherNames; instead of resetting the length of array, as the reference of problemMatcherNames is assigned to the task schema when theia starts.

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves tasks issues related to the task system labels Oct 21, 2019
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 the changes:

  • the names of registered problem matchers are displayed when I edit tasks.json file manually
  • warning is displayed when I try to use unregistered problem matcher for a configuration

@vince-fugnitto
Copy link
Member

@elaihau I noticed something, when executing a task (ex: test task under tasks/test-resources), I selected the builtin problem matcher eslint-stylish. When looking at the 'tasks.json', the editor complains that the problem matcher does not exist, likely due to the **$**.

image

@elaihau
Copy link
Contributor Author

elaihau commented Oct 22, 2019

@elaihau I noticed something, when executing a task (ex: test task under tasks/test-resources), I selected the builtin problem matcher eslint-stylish. When looking at the 'tasks.json', the editor complains that the problem matcher does not exist, likely due to the **$**.

image

wow nice catch !
should we put both $eslint-stylish and eslint-stylish in the list? it is a dilemma ... as looks a little redundent.
Maybe ... instead we could do something to make sure the problem matcher auto-added by theia always starts with "$"? what do you think? Thanks!
@vince-fugnitto

@vince-fugnitto
Copy link
Member

wow nice catch !
should we put both $eslint-stylish and eslint-stylish in the list? it is a dilemma ... as looks a little redundent.
Maybe ... instead we could do something to make sure the problem matcher auto-added by theia always starts with "$"? what do you think? Thanks!
@vince-fugnitto

From looking at VS Code, it looks like they register the builtin problem matchers with the $ included.
Perhaps, we should add to the schema only one instance for each with the $ included as well.

image

- With this pull request, task schemas are updated on problem matchers
being added / disposed, and users are benefited from having content assist while specifying the name(s) of problem matcher(s) in tasks.json.
- resolves #6134

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
@elaihau
Copy link
Contributor Author

elaihau commented Oct 22, 2019

From looking at VS Code, it looks like they register the builtin problem matchers with the $ included.
Perhaps, we should add to the schema only one instance for each with the $ included as well.

Yes you are right. I updated the code to match the vs code behavior. 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 works much better now thank you! 👍

@elaihau elaihau merged commit cdccf8c into master Oct 23, 2019
@elaihau elaihau deleted the Liang/assist_matcher_name branch October 23, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tasks] add registered 'ProblemMatchers' to the 'tasks.json' schema
4 participants