Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add task plugin #97

Merged
merged 6 commits into from
Mar 14, 2019
Merged

Add task plugin #97

merged 6 commits into from
Mar 14, 2019

Conversation

RomanNikitenko
Copy link
Member

@RomanNikitenko RomanNikitenko commented Mar 6, 2019

Move task extension functionality to plugin
Related issue: eclipse-che/che#11458

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
plugins/task-plugin/package.json Outdated Show resolved Hide resolved

export const CheWorkspaceClient = Symbol('CheWorkspaceClient');

export interface CheWorkspaceClient {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this interface? Is more than one implementation expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed, thank you!


/**
* Generated using theia-plugin-generator
*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment required since the file is generated once and isn't overwritten anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed, thank you!

commands.forEach(command => {
tasks.push(this.toTask(command));
});
return tasks;
Copy link
Member

Choose a reason for hiding this comment

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

can be simplified to

const commands = await this.cheWorkspaceClient.getCommands();
return commands.map(c => this.toTask(c));

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, thank you!

const PROJECTS_ROOT_VARIABLE = 'CHE_PROJECTS_ROOT';
const ERROR_MESSAGE_TEMPLATE = 'Can not resolve \'current.project.path\' variable.';
/**
* Contributes the path for current project as a relative path to the first directory under the root workspace.
Copy link
Member

Choose a reason for hiding this comment

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

Contributes the variable for getting path ... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

package.json Outdated
@@ -3,6 +3,7 @@
"private": true,
"devDependencies": {
"@types/jest": "24.0.3",
"@types/jest-diff": "^20.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a build and test dependency CQ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed, thank you!

@@ -0,0 +1,62 @@
{
Copy link
Contributor

@benoitf benoitf Mar 7, 2019

Choose a reason for hiding this comment

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

package-lock.json file should be removed (we're using yarn workspaces)

Copy link
Member Author

Choose a reason for hiding this comment

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

removed, thank you!

"theiaPlugin": {
"backend": "lib/task-plugin-backend.js"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

EOF

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

import { injectable, inject } from 'inversify';
import { CheWorkspaceClient } from '../che-workspace/che-workspace-client';

const ReconnectingWebSocket = require('reconnecting-websocket');
Copy link
Contributor

Choose a reason for hiding this comment

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

from where this dependency is coming from ? (I don't see it in package.json) and there is no CQ

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm removing it from my PR, but btw, we use it at least here, here and here
So, should we remove it from all these places?


import * as theia from '@theia/plugin';
import * as che from '@eclipse-che/plugin';
import 'reflect-metadata';
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that reflect-metadata is imported more than once in the plug-in while it shouldn't

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, thank you!

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@RomanNikitenko
Copy link
Member Author

@benoitf I updated the PR, please review.

@RomanNikitenko RomanNikitenko merged commit dfd97f5 into master Mar 14, 2019
@RomanNikitenko RomanNikitenko deleted the task-plugin-backend branch March 14, 2019 15:23
@l0rd l0rd mentioned this pull request Apr 2, 2019
vinokurig pushed a commit that referenced this pull request Apr 6, 2021
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants