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

support input variables for tasks #6331

Merged
merged 1 commit into from
Oct 6, 2019
Merged

support input variables for tasks #6331

merged 1 commit into from
Oct 6, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Oct 4, 2019

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

How to test

Peek 2019-10-04 16-53

Please kindly note, the input variable resolution is not supported in all properties under the task config.
Properties include

  • type & label for configured tasks,
  • the ones that are invovled in the task definition, such as script for detected npm tasks, and
  • name of problemMatchers
    cannot have input variables. (vsCode does the same)

Review checklist

- add support to input variables which have the syntax: ${input:variableID}, where the variableID refers to entries in the inputs section of `tasks.json`.
- resolves #5836

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
@vince-fugnitto
Copy link
Member

@elaihau I think it'd also be useful to update tasks/test-resources to include examples of inputs and perhaps at least one task which uses input variables so we have an example of a reproducible task to test upon.

@vince-fugnitto vince-fugnitto added the tasks issues related to the task system label Oct 5, 2019
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 great for me 👍

I tried using the angular example from VSCode and all the features worked correctly including the dropdown (options) and the input.

@elaihau
Copy link
Contributor Author

elaihau commented Oct 6, 2019

Thank you Vincent @vince-fugnitto !

@elaihau elaihau merged commit 0f7fc56 into master Oct 6, 2019
@elaihau elaihau deleted the Liang/task_input_var branch October 6, 2019 02:09
@@ -150,6 +150,7 @@ export namespace VariableResolverService {
} catch (e) {
console.error(`Failed to resolved '${name}' variable`, e);
this.resolved.set(name, undefined);
throw e;
Copy link
Member

@akosyakov akosyakov Oct 6, 2019

Choose a reason for hiding this comment

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

@elaihau could you elaborate why? It is a breaking change, before broken variables will computed as undefined, but the entire operation won't be stopped.

It affects not only tasks but also launch configurations. We should test this changes agains the debug extension before merging this PR.

@akosyakov
Copy link
Member

@elaihau it would be better if you wait till Monday that other back to work and can have a look

@@ -85,7 +85,7 @@ export class CommonVariableContribution implements VariableContribution {
const inputs = !!configuration && 'inputs' in configuration ? configuration.inputs : undefined;
const input = Array.isArray(inputs) && inputs.find(item => !!item && item.id === variable);
if (!input) {
return undefined;
throw new Error(`Undefined input variable "${variable}" encountered. Remove or define "${variable}" to continue.`);
Copy link
Member

Choose a reason for hiding this comment

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

@elaihau please revert all changes to resolver

I see what you want now, but we should do it holistically and fix in all other places. Please file an issue for it

@elaihau
Copy link
Contributor Author

elaihau commented Oct 7, 2019

@elaihau it would be better if you wait till Monday that other back to work and can have a look

Sorry I didn't realize the change i made to variable-resolver-service.ts was a breaking one. You are right. I filed an issue #6333

@akosyakov
In order to address this comment, I will

  • revert changes I made to common-variable-contribution.ts and variable-resolver-service.ts
  • change process-task-resolver.ts and task-service.ts: when an input variable is resolved as undefined, I will throw an error saying "Undefined input variable xxx encountered". The idea behind it is: in tasks.json, the input varibles, if defined, should only be resolved as strings. If resolved as undefined, the user should have mistyped the variable name, or missed the default value in the definition.

I will also change the schema updater so that users have content assist.

elaihau pushed a commit that referenced this pull request Oct 7, 2019
- content assist is added to help the users of tasks.json input variables.
- changes made to VariableResolverService and CommonVariableContribution
in #6331 are reverted.

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
vince-fugnitto pushed a commit that referenced this pull request Oct 7, 2019
- content assist is added to help the users of tasks.json input variables.
- changes made to VariableResolverService and CommonVariableContribution
in #6331 are reverted.

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
akosyakov pushed a commit to akosyakov/theia that referenced this pull request Feb 24, 2020
- content assist is added to help the users of tasks.json input variables.
- changes made to VariableResolverService and CommonVariableContribution
in eclipse-theia#6331 are reverted.

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support input variable substitution for tasks
3 participants