handles invalid task configurations #6515

elaihau · 2019-11-08T04:03:38Z

not display the invalid task configurations in the list of tasks.
displays a warning message to inform users if Theia finds invalid task
configuration(s) in tasks.json.
fixes task config parsing errors are not handled properly #6482

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

How to test

As the GIF shows,

enter an invalid task configuration into tasks.json.
FYI, it would be helpful to keep the Problem View open, as it would display the schema errors Theia finds from task configuration. With the information from the Problems View, we know if we are truely entering an invalid config.
Open the list of tasks from the top menu bar "Termimal -> Run Tasks"
At this point of time, the list of tasks should NOT have the task config that was entered most recently, because it's invalid. Also, there should be a warning populated that reminds us to open the Problems view to get what the errors are.
Go back to tasks.json and fix the invalid configuration
After the config is fixed, it should show up as one item in the list of tasks.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

akosyakov · 2019-11-08T06:17:54Z

What VS Code does? Usually it is more tolerant and will ignore unnecessary or bogus properties. Please check.

elaihau · 2019-11-08T12:06:22Z

What VS Code does? Usually it is more tolerant and will ignore unnecessary or bogus properties. Please check.

vs code does something similar ... and they have such mechanism to validate "input" properties as well.

Just want to clarify, being tolerant on bogus properties or not being tolerant depends on how the task schema is defined. So in this commit my main focus was comparing the Theia schema agaist that one in vs code.

The part I am missing in this PR is "directing the validation errors into the Output View", and I am trying to improve it in that aspect.

lmcbout · 2019-11-08T12:29:33Z

I think it is a good start to show a dialog with some info, but it goes away. I find what VSCode do is better by having the dialog, but also with a button to open the "Output" view with more details about the invalid task. If the end-user doesn't have the "problem " view open in Theia, it is harder to see the reason why the task is not showing in he quick-open menu. The "Problem" view is not always visible when you modify the task.json file.

elaihau · 2019-11-08T12:35:59Z

If the end-user doesn't have the "problem " view open in Theia, it is harder to see the reason why the task is not showing in he quick-open menu. The "Problem" view is not always visible when you modify the task.json file.

Thank you for the feedback ! @lmcbout
Would it be good to add an action button to the dialog, once users click it we

Open tasks.json, and
Open the Problems View (if they are not opened) ?

Question is which tasks.json to open, if there are invalid task configs in multiple files?
We only open the editor for the 1st invalid config we found, or open all editors where the invalid configs are? Please advise :)

lmcbout · 2019-11-08T12:45:29Z

Would be nice to open task.json and the problem view, but we also need to point to the faulty task (at least for the first faulty task) if there are more than one.
I think we should open all "tasks.json" having faulty tasks. It is a personnel choice here :)

lmcbout · 2019-11-08T12:47:16Z

What about opening the output view with all the faulty tasks if there are more than one and open the tasks.json with the first error in it with the problem view ?

vince-fugnitto · 2019-11-08T14:25:22Z

I think it is a good start to show a dialog with some info, but it goes away. I find what VSCode do is better by having the dialog, but also with a button to open the "Output" view with more details about the invalid task. If the end-user doesn't have the "problem " view open in Theia, it is harder to see the reason why the task is not showing in he quick-open menu. The "Problem" view is not always visible when you modify the task.json file.

If the problems-view is closed, there are still multiple indications that the task has errors.

the statusbar contribution shows the number of markers.
the editor itself has the marker where the incorrect task(s) are.
the editor itself has a decoration on the right-hand side for the marker.
the task does not display in the run task menu, which tells users something may be wrong.

I'm not sure we need to over-engineer the solution, I think properly handling invalid task configs like @elaihau has been doing is great.

elaihau · 2019-11-08T15:54:51Z

What about opening the output view with all the faulty tasks if there are more than one and open the tasks.json with the first error in it with the problem view ?

hmm, i can work on the part that opens the tasks.json that has the 1st invalid task, and the Problems View.

Once that's implemented I can upload a GIF to demo what it looks like, and we could discuss if opening the Output View makes the solution more user friendly.
My only concern is: If both Problems view and Output view are open, which view should stay on top of the other?

vince-fugnitto · 2019-11-08T15:59:01Z

What about opening the output view with all the faulty tasks if there are more than one and open the tasks.json with the first error in it with the problem view ?

hmm, i can work on the part that opens the tasks.json that has the 1st invalid task, and the Problems View.

Once that's implemented I can upload a GIF to demo what it looks like, and we could discuss if opening the Output View makes the solution more user friendly.
My only concern is: If both Problems view and Output view are open, which view should stay on top of the other?

@elaihau I don't think it needs to be overcomplicated for the moment. Informing users that they may have one or more task configs which are invalid due to missing properties is sufficient. An end user knows where the tasks.json is and how to display the problems-view if required.

vince-fugnitto · 2019-11-08T16:02:31Z

In the future we can have a new channel for the output-view to display task errors and prompt users to open the output-view when tasks have errors, but I don't believe it necessarily needs to be addressed in this PR. For most cases, the current implementation you have is sufficient while anything more would be an enhancement.

vince-fugnitto

I'm fine with the following changes 👍
Task configurations which are invalid are not displayed when attempting to run them, the editor and problems-view correctly display makers, and users are informed that the tasks.json may have errors.

vince-fugnitto · 2019-11-11T15:32:23Z

packages/task/src/browser/task-service.ts

+        const schema = this.taskSchemaUpdater.getTaskSchema();
+        const ajv = new Ajv();
+        const validateSchema = ajv.compile(schema);
+        const isValid = !!validateSchema({ tasks: [task] });


[minor] I believe we can simply

return !!validateSchema({ tasks: [task] });

resolved. Thank you for your review !

elaihau · 2019-11-12T03:29:03Z

@lmcbout
I made a change as per our discussion earlier today: the tasks.json and problems view are opened by clicking the action button in the message dialog.

if the tasks.json and problems view are both already open, the action button is not displayed.

RomanNikitenko

Tested for shell and typescript tasks:

a task is not displayed if its configuration is invalid
a task is displayed after fixing its configuration
warning informs user about invalid configuration
there is ability to open the corresponding tasks.json file by clicking on Open button of warning message if config file is closed

Works well for me!

btw: should we remove warning for version property?
I mean case when user opens some existed project with tasks.json file which was created using vs code.

elaihau · 2019-11-12T12:24:28Z

btw: should we remove warning for version property?

Yes you are right. I will update the schema. Thank you for the review !

- not display the invalid task configurations in the list of tasks. - displays a warning message to inform users if Theia finds invalid task configuration(s) in tasks.json. - fixes #6482 Signed-off-by: Liang Huang <liang.huang@ericsson.com>

elaihau requested a review from lmcbout November 8, 2019 04:03

elaihau force-pushed the Liang/filter_invalid_task branch 4 times, most recently from 0912e85 to e272f70 Compare November 8, 2019 04:15

akosyakov requested review from vince-fugnitto and RomanNikitenko November 8, 2019 06:18

akosyakov added tasks issues related to the task system vscode issues related to VSCode compatibility labels Nov 8, 2019

vince-fugnitto approved these changes Nov 11, 2019

View reviewed changes

elaihau force-pushed the Liang/filter_invalid_task branch 2 times, most recently from b942c02 to 9a3b5c2 Compare November 12, 2019 03:27

elaihau force-pushed the Liang/filter_invalid_task branch from 9a3b5c2 to 51b011d Compare November 12, 2019 03:36

RomanNikitenko approved these changes Nov 12, 2019

View reviewed changes

handles invalid task configurations

35aebc1

- not display the invalid task configurations in the list of tasks. - displays a warning message to inform users if Theia finds invalid task configuration(s) in tasks.json. - fixes #6482 Signed-off-by: Liang Huang <liang.huang@ericsson.com>

elaihau force-pushed the Liang/filter_invalid_task branch from 51b011d to 35aebc1 Compare November 12, 2019 12:34

elaihau merged commit fb398d6 into master Nov 12, 2019

elaihau deleted the Liang/filter_invalid_task branch November 12, 2019 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handles invalid task configurations #6515

handles invalid task configurations #6515

elaihau commented Nov 8, 2019

akosyakov commented Nov 8, 2019

elaihau commented Nov 8, 2019 •

edited

Loading

lmcbout commented Nov 8, 2019

elaihau commented Nov 8, 2019 •

edited

Loading

lmcbout commented Nov 8, 2019

lmcbout commented Nov 8, 2019

vince-fugnitto commented Nov 8, 2019 •

edited

Loading

elaihau commented Nov 8, 2019 •

edited

Loading

vince-fugnitto commented Nov 8, 2019

vince-fugnitto commented Nov 8, 2019

vince-fugnitto left a comment

vince-fugnitto Nov 11, 2019

elaihau Nov 12, 2019

elaihau commented Nov 12, 2019

RomanNikitenko left a comment

elaihau commented Nov 12, 2019

handles invalid task configurations #6515

handles invalid task configurations #6515

Conversation

elaihau commented Nov 8, 2019

How to test

Review checklist

akosyakov commented Nov 8, 2019

elaihau commented Nov 8, 2019 • edited Loading

lmcbout commented Nov 8, 2019

elaihau commented Nov 8, 2019 • edited Loading

lmcbout commented Nov 8, 2019

lmcbout commented Nov 8, 2019

vince-fugnitto commented Nov 8, 2019 • edited Loading

elaihau commented Nov 8, 2019 • edited Loading

vince-fugnitto commented Nov 8, 2019

vince-fugnitto commented Nov 8, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto Nov 11, 2019

Choose a reason for hiding this comment

elaihau Nov 12, 2019

Choose a reason for hiding this comment

elaihau commented Nov 12, 2019

RomanNikitenko left a comment

Choose a reason for hiding this comment

elaihau commented Nov 12, 2019

elaihau commented Nov 8, 2019 •

edited

Loading

elaihau commented Nov 8, 2019 •

edited

Loading

vince-fugnitto commented Nov 8, 2019 •

edited

Loading

elaihau commented Nov 8, 2019 •

edited

Loading