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 fetchTasks() and executeTask() to tasks API #6058

Merged
merged 1 commit into from
Sep 5, 2019
Merged

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Aug 28, 2019

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

How to test

This pull request can be tested with adding a vscode extension to Theia:

import * as vscode from 'vscode';

export function activate(context: vscode.ExtensionContext) {

	console.log('Congratulations, your extension "fetchtest" is now active!');

	let disposable = vscode.commands.registerCommand('extension.helloWorld', () => {
		// The code you place here will be executed every time your command is executed

		// Display a message box to the user
		vscode.window.showInformationMessage('Test Fetch Task and Execute Task!');

		// vscode.tasks.fetchTasks({ type: 'shell' }).then((value) => {
		// 	console.log('shell fulfilled', value);
		// }, (reason) => {
		// 	console.log('rejected', reason);
		// });

		vscode.tasks.fetchTasks({ type: 'npm' }).then((fetchedTasks) => {
			console.log('npm tasks found: ', fetchedTasks.length);
			for (const task of fetchedTasks) {
				console.log('\n\nlooking at ', task.name, '\n\n');
				vscode.tasks.executeTask(task);
			}
		}, (reason) => {
			console.log('fetch task was rejected', reason);
		});

		// const myTask = new vscode.Task(
		// 	{ type: 'shell' }, // task definition
		// 	vscode.TaskScope.Workspace,
		// 	'test',
		// 	'testExtension',
		// 	new vscode.ShellExecution('ls -alR', { cwd: '/home/elaihau/dev' })
		// );
		// vscode.tasks.executeTask(myTask);
	});

	context.subscriptions.push(disposable);
}

export function deactivate() { }

For reviewer's convenience I uploaded the source code of my extension to https://github.com/elaihau/fetchtest

Review checklist

return taskExecution;
}
}
return Promise.reject(new Error(reason));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved Roman's feedback #5357 (comment)

@akosyakov akosyakov added the tasks issues related to the task system label Aug 28, 2019
CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
- [task] displayed the customized tasks as "configured tasks" in the task quick open [#5777](https://github.com/theia-ide/theia/pull/5777)
- [task] allowed users to override any task properties other than the ones used in the task definition [#5777](https://github.com/theia-ide/theia/pull/5777)
- [task] notified clients of TaskDefinitionRegistry on change [#5915](https://github.com/theia-ide/theia/pull/5915)
- [task] added `tasks.fetchTasks()` and `tasks.executeTask()` to plugins API [#???](https://github.com/theia-ide/theia/pull/???)
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can add the number now :) #6058

version?: string;

/**
* The task type to return;
Copy link
Member

Choose a reason for hiding this comment

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

Can we adjust the formatting?

version?: string;

/**
* The task type to return;
Copy link
Member

Choose a reason for hiding this comment

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

The task type to return. instead of The task type to return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i changed it to the type of tasks to return.

packages/task/src/browser/task-service.ts Show resolved Hide resolved
@elaihau elaihau force-pushed the Liang/redo_fetch branch 2 times, most recently from 3223cfc to 3cb394c Compare August 30, 2019 00:41
@RomanNikitenko
Copy link
Contributor

@elaihau @akosyakov
I'm on vacation and going to be offline next 2 weeks.
thank you for understanding!

@elaihau
Copy link
Contributor Author

elaihau commented Sep 2, 2019

@RomanNikitenko thank you for the heads up ! Have fun :)

@vince-fugnitto maybe i can walk you through the code and explain how to test it this Friday (Sep 6) ?

Copy link
Contributor

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

I looked though the code, looks ok to me.
Will test this PR a bit later.

}

const filtered: TaskConfiguration[] = [];
found.forEach((taskConfig, ind) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use taskIndex, index or i.

if (executionDto) {
const taskExecution = this.getTaskExecution(executionDto);
return taskExecution;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you, but I'd prefer not to introduce another variable for error message.
This way we'll avoid usually unneded variable, (small plus to performance and memory usage) and it will be easier to read:

const x = getX(...);
if (x) {
   const y = getY(...);
   if (y) {
      ...
      return;
   }
   throw new Error('Y not found');
}
throw new Error ('X not found')

(And it is clear after which if it failed and exited).

*
* @param filter a filter to filter the return tasks.
*/
export function fetchTasks(filter?: TaskFilter): Thenable<Task[]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use PromiseLike insted of Thenable

@vince-fugnitto
Copy link
Member

@vince-fugnitto maybe i can walk you through the code and explain how to test it this Friday (Sep 6) ?

Yes, that sounds fine with me :)

@mmorhun
Copy link
Contributor

mmorhun commented Sep 3, 2019

I've tested the changes for the following cases:

  • Fetch tasks:
    • Fetch tasks from tasks.json inside .theia folder of specific type
    • Fetch tasks of specific type which are provided by a TaskProvider contributed by plugin.
    • Mix of two previous cases: fetch tasks of specific type when they are provided by tasks.json and some by a TaskProvider from within a plugin.
  • Run tasks:
    • Fetch tasks of specific type, found the task with specifi label, change its configuration and run altered task.

All described above cases worked for me.

Resource used for testing of this PR:

Source code of the test plugin
import * as theia from '@theia/plugin';

export function start(context: theia.PluginContext) {
    const listTAsksTestCommand = {
        id: 'list-tasks-test-command',
        label: "Test list tasks of type"
    };
    context.subscriptions.push(theia.commands.registerCommand(listTAsksTestCommand, (...args: any[]) => {
        theia.window.showInformationMessage('On list tasks');

        theia.tasks.fetchTasks({ type: 'ctask' }).then((tasks: theia.Task[]) => {
            console.log('IN CALLBACK:', tasks);
            for (const task of tasks) {
                console.log('TASK:', task.name);
            }
        });
    }));

    const runTaskTestCommand = {
        id: 'run-task-test-command',
        label: "Test alter task"
    };
    context.subscriptions.push(theia.commands.registerCommand(runTaskTestCommand, (...args: any[]) => {
        theia.window.showInformationMessage('On run altered task');

        theia.tasks.fetchTasks({ type: 'ctask' }).then((tasks: theia.Task[]) => {
            console.log('IN RUN TASK:', tasks);
            for (const task of tasks) {
                if (task.name.startsWith('Echo hello world')) {
                    if (!task.execution) {
                        task.execution = {};
                    }
                    if (!task.execution.options) {
                        task.execution.options = {};
                    }

                    (task.execution as theia.ShellExecution).commandLine = 'sleep 1 && ls -la';
                    task.execution.options.cwd = "/tmp";

                    theia.tasks.executeTask(task);

                    return;
                }
            }
        });
    }));

    theia.tasks.registerTaskProvider('ctask', new CTaskProvider());
}

class CTaskProvider {
    async provideTasks(): Promise<theia.Task[]> {
        const task1 = new theia.Task({ type: 'ctask' }, theia.TaskScope.Global, 'first task', 'the-extension');
        const task2 = new theia.Task({ type: 'ctask' }, theia.TaskScope.Workspace, 'second task', 'ctask');
        return [task1, task2];
    }

    async resolveTask(task: theia.Task): Promise<theia.Task> {
        return task;
    }
}

export function stop() { }
tasks.json
{
    "tasks": [
        {
            "type": "abcd",
            "label": "rnodejs-hello-world:run",
            "command": "yarn",
            "target": {
                "workingDir": "/projects/theia"
            },
            "previewUrl": "${server.3000/tcp}"
        },
        {
            "type": "ctask",
            "label": "Echo hello world",
            "command": "echo \"hello world\"",
            "target": {
                "workingDir": "/projects",
                "containerName": "theia-dev"
            }
        }
    ]
}

Copy link
Contributor

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

I've tested changes (see comment above) and it works as expected.
@elaihau please take a look at my previous comments at least about PromiseLike and Thenable.

- This pull request adds the support of executing a vscode.Task, and fetching tasks by type and schema version to the plugins API.
- resolved #5342

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

elaihau commented Sep 5, 2019

Thank you for the review @mmorhun !

I updated the code to address your review comments.

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.

[plug-in][tasks] Add ability to fetch all tasks
5 participants