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

update task schema for installed extensions & plugins #6492

Merged
merged 1 commit into from
Nov 7, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Nov 5, 2019

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

How to test

  • Install vscode extensions that make contributions to the task definitions. Here are a few candidates: npm, grunt, typescript-language-features, jake, & gulp.
  • Open tasks.json file.
  • Start to manually enter a new configuration, and check the information from the content assist.

Peek 2019-11-04 21-33

Review checklist

@elaihau elaihau changed the title update task schema for Theia extensions & plugins update task schema for the installed extensions & plugins Nov 5, 2019
@elaihau elaihau changed the title update task schema for the installed extensions & plugins update task schema for installed extensions & plugins Nov 5, 2019
@akosyakov akosyakov added tasks issues related to the task system vscode issues related to VSCode compatibility labels Nov 5, 2019
@lmcbout
Copy link
Contributor

lmcbout commented Nov 5, 2019

@elaihau I start testing with npm. Initially , when I put the "{}", I can see that the task require "type", "label" and "command". I start to put the type "npm" and if I check again what is missing, the "Label" and "command" are missing, it just proposed "script". If I don't put a label, I will not see it when I select from the quick-open -> Run Task
Reason: no label entered

Second issue:
Using "type": "npm"
Now it is asking for a script which is fine, but it does not proposed the available scripts for "npm". If I run "npm -help" in a terminal, the following list of commands show:, Should we proposed those items as available command for "npm"?
npm -help
Usage: npm

where is one of:
access, adduser, audit, bin, bugs, c, cache, ci, cit,
clean-install, clean-install-test, completion, config,
create, ddp, dedupe, deprecate, dist-tag, docs, doctor,
edit, explore, get, help, help-search, hook, i, init,
install, install-ci-test, install-test, it, link, list, ln,
login, logout, ls, org, outdated, owner, pack, ping, prefix,
profile, prune, publish, rb, rebuild, repo, restart, root,
run, run-script, s, se, search, set, shrinkwrap, star,
stars, start, stop, t, team, test, token, tst, un,
uninstall, unpublish, unstar, up, update, v, version, view,
whoami

Test6492

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Nov 5, 2019

I start to put the type "npm" and if I check again what is missing, the "Label" and "command" are missing, it just proposed "script". If I don't put a label, I will not see it when I select from the quick-open -> Run Task
Reason: no label entered

@lmcbout
About your first case: are you defining script field with correct value?
I'm testing right now and it works for me:
npm_task

@lmcbout
Copy link
Contributor

lmcbout commented Nov 5, 2019

About your first case: are you defining script field with correct value?
I'm testing right now and it works for me:

@RomanNikitenko
See on the following video, it does not show for me, I am on Ubuntu
npmPostInstallDoNotShow

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Nov 5, 2019

@lmcbout
I guess you don't have postinstall script in your package.json file, that's why the task is not displayed for Terminal => Run Task menu.

List of the suggestions for script field doesn't display available scripts for me as well, but I think it's expected behavior (the same as for vs code), I don't think we can improve this behavior.

@elaihau please correct me if I'm wrong

@elaihau
Copy link
Contributor Author

elaihau commented Nov 6, 2019

List of the suggestions for script field doesn't display available scripts for me as well, but I think it's expected behavior (the same as for vs code), I don't think we can improve this behavior.

@RomanNikitenko
I agree with you. The implementation in this patch only gives us suggestions in terms of the defined schemas. Although I believe technically we are able to provide content assists on "what the available scripts are" for "npm", it would require:

  • either quite a lot of work to be done in Theia
  • or relatively less work to be done in BOTH theia and npm extension (and we don't have control over the npm or any other vs code extensions).

Using "type": "npm"
Now it is asking for a script which is fine, but it does not proposed the available scripts for "npm". If I run "npm -help" in a terminal, the following list of commands show:, Should we proposed those items as available command for "npm"?
npm -help
Usage: npm

@lmcbout
It would be really nice to propose scripts that are available for extensions such as npm. However, like I mentioned in the response to Roman, it will not be a small task to do, and vs code does not have this feature either :)
So, let me implement what vs code has, and catch up with vscode if this feature is included in its future release.

@elaihau
Copy link
Contributor Author

elaihau commented Nov 6, 2019

I start testing with npm. Initially , when I put the "{}", I can see that the task require "type", "label" and "command". I start to put the type "npm" and if I check again what is missing, the "Label" and "command" are missing, it just proposed "script". If I don't put a label, I will not see it when I select from the quick-open -> Run Task
Reason: no label entered

@lmcbout
you are right, the content assist is a little confusing when you start typing.
vscode has similar issues (well i am NOT saying "vscode has such issue so i am comfortable with having it in theia"). And I am not sure what is the best way to handle it, because tasks.json is the place for

  • user-defined tasks (whose type is shell or process), and
  • customized tasks (the configs that override those detected tasks)

Therefore, when users start typing in the editor, Theia has no idea of which schema should be used in the content assist.

That's why

  • you saw "type, lable, and command are missing" at the very beginning (because they are mandatory for "shell" and "process" tasks), and
  • once you set yout task type to "npm", content assists told you "script is missing" (because "script" is mandatory for "npm" tasks).
  • And, if you set task type to "typescript", content assist tells you "tsConfig" is missing (because "tsConfig" is madatory for "typescript" tasks)
  • etc etc

To sum up, content assist assumes the task being entered into tasks.json is a "shell/process" task at the very beginning, and it narrows down its suggestions once users enter more and more information.

@elaihau elaihau force-pushed the Liang/task_schema_for_plugins branch from d6b3ab6 to 39ea9f1 Compare November 6, 2019 04:13
@RomanNikitenko
Copy link
Contributor

@elaihau
Can we improve the behavior for properties?

I mean, at the moment it looks like: https://youtu.be/b3aWdFABIpE

current_properties

=============================================
Improved: https://youtu.be/V8O9InEKH1E

improved_properties

Is it possible? Are there any problems?

@elaihau
Copy link
Contributor Author

elaihau commented Nov 6, 2019

@elaihau
Can we improve the behavior for properties?

I mean, at the moment it looks like: https://youtu.be/b3aWdFABIpE

=============================================
Improved: https://youtu.be/V8O9InEKH1E

Is it possible? Are there any problems?

Hi Roman @RomanNikitenko , thank you for the feedback !

Is "the type of the script property" the only difference between the existing and your improved schema? am i missing something?

If the missing part is "type of the property", yes we can improve it by changing the TaskDefinition interface and possibly a few lines of plugin-ext in Theia. I can work on it today.

@RomanNikitenko
Copy link
Contributor

@elaihau
yes, I added type to property. But I didn't investigate it deeply - just to check the behavior.
I don't mind to merge the PR as is and improve the behavior later.

@elaihau
Copy link
Contributor Author

elaihau commented Nov 6, 2019

yes, I added type to property. But I didn't investigate it deeply - just to check the behavior.
I don't mind to merge the PR as is and improve the behavior later.

thanks for the clarification. let me see if i can manage implementing it with a small change. if it turns out too big i will let you know.

@lmcbout
Copy link
Contributor

lmcbout commented Nov 6, 2019

@RomanNikitenko you were right, I did not have the "postinstall" in the package.json, it works now

@elaihau should we take care of te following on a separate PR?
Question 1: When I add a new task in "tasks.json", it shows in the "Task run"
If I add a label to this task, it does not show on the initial occurrence, but the label shows on every other opening
TaskLabelNotShowingInitially

Question 2: If I remove the label from "tasks.json", the label still shows on the menu "Task run", but when executing it, it puts an error on the server side:
task ERROR Can't get task launch configuration for label: JACK
TaskNoMoreLabel

@elaihau
Copy link
Contributor Author

elaihau commented Nov 6, 2019

Question 1: When I add a new task in "tasks.json", it shows in the "Task run"
If I add a label to this task, it does not show on the initial occurrence, but the label shows on every other opening

@lmcbout
If I am not mistaken your task config is something like

{
  "type": "npm",
  "script": "posttest",
  "label": "JACQUE"
}

I believe this config (i.e., type, script, & label) does not match any of the schemas defined in Theia, as Theia does not support having "label" in configurations that customize detected tasks ...
However the combination of "type, script, & label" is supported in vs code. Let me investigate how it works in vs code and see if it belongs to a separate PR.

- With this change, Theia processes task definitions contributed by extensions and plugins, and updates the task schema.
- resolves #6485

Signed-off-by: Liang Huang <[email protected]>
@elaihau elaihau force-pushed the Liang/task_schema_for_plugins branch from 39ea9f1 to 3ba3520 Compare November 7, 2019 02:02
@elaihau
Copy link
Contributor Author

elaihau commented Nov 7, 2019

What @RomanNikitenko suggested has been included in the lastest patch.

Also created #6507 for adding what @lmcbout suggested in theia.

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 after update for npm and typescript-language-features vs code extensions and for shell task: works well for me!
The descried case has been fixed.
The improvement has been applied.

@elaihau thank you very much!

@lmcbout
Copy link
Contributor

lmcbout commented Nov 7, 2019

Question 1:
When I hover the "{", you proposed 3 things: "type, label command", but the minute I expand the "{}", and try to enter the type, you proposed more "String" to be selected, should we just proposed initially what is needed to create a task, i.e. "type, label, command"
ExtraString

Question 2:
When I defined a task as "type":"shell", then define the command but not the label, it shows as "undefined in the quick menu. Should we pre-defined the template with the minimal mandatory fields required after selecting the "type" of task, so the end-user knows the minimal fields to define depending of the task type ?
TaskShellUndefined

@vince-fugnitto
Copy link
Member

Question 1:
When I hover the "{", you proposed 3 things: "type, label command", but the minute I expand the "{}", and try to enter the type, you proposed more "String" to be selected, should we just proposed initially what is needed to create a task, i.e. "type, label, command"

I don't think so, the first 3 things (type, label and command) are the mandatory fields that's why the problem marker displays a warning. For the rest, it is simply displaying all the possible values of the schema. These are two completely different things.

@vince-fugnitto
Copy link
Member

Question 2:
When I defined a task as "type":"shell", then define the command but not the label, it shows as "undefined in the quick menu. Should we pre-defined the template with the minimal mandatory fields required after selecting the "type" of task, so the end-user knows the minimal fields to define depending of the task type ?
TaskShellUndefined

If you look, you are still getting a warning that your task config is incomplete so as a user I do not expect my task to be working correctly until the mandatory fields are met.

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.

If you look, you are still getting a warning that your task config is incomplete so as a user I do not expect my task to be working correctly until the mandatory fields are met.

You need to pay attention to see the squiggling line under the begin "{", I find it not enough intuitive . If everyone is happy, then I am OK with it.

Otherwise, it works fine. Thanks @elaihau

@elaihau
Copy link
Contributor Author

elaihau commented Nov 7, 2019

Question 2:
When I defined a task as "type":"shell", then define the command but not the label, it shows as "undefined in the quick menu. Should we pre-defined the template with the minimal mandatory fields required after selecting the "type" of task, so the end-user knows the minimal fields to define depending of the task type ?

You are right @lmcbout - a task config that only has "type" and "command" is considered incomplete, and we should have a mechanism to prevent it (i.e., shell: undefined) from showing up in the task list.

I filed #6482 for the problem. I am currently on it, and once it is in review I will add you to the reviewers list. Thank you !

@elaihau elaihau merged commit c26f35d into master Nov 7, 2019
@elaihau elaihau deleted the Liang/task_schema_for_plugins branch November 7, 2019 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tasks issues related to the task system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update task schema according to installed extensions/plugins
5 participants