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

Initial Che api for Theia #1

Merged
merged 11 commits into from
Dec 10, 2018
Merged

Initial Che api for Theia #1

merged 11 commits into from
Dec 10, 2018

Conversation

evidolob
Copy link
Contributor

Signed-off-by: Yevhen Vydolob [email protected]

@evidolob evidolob self-assigned this Nov 28, 2018
Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

There is not a single comment in all the classes.
IMHO some classes should have comments to describe what they're doing
also a README file should describe the extension


export interface Factory {
workspace: WorkspaceConfig;
ide?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it related to CHE IDE or also to Theia?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Che is able to produce typescript definition of current DTOs
https://github.com/eclipse/che/blob/21318aeec878a02ee9a64453ad8e737204ccfe21/dockerfiles/lib/build.sh#L35-L44

might be good if we could reuse these definitions automatically instead of trying to duplicate it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I will look at this

Copy link
Contributor

Choose a reason for hiding this comment

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

as an intermediate step, we could just move out upstream che definitions to a separate .d.ts file for now

export interface Factory {
workspace: WorkspaceConfig;
ide?: {
onAppLoaded?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

onDidLoadApplication?

onAppLoaded?: {
actions?: FactoryAction[]
};
onProjectsLoaded?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

onDidLoadProjects

onProjectsLoaded?: {
actions?: FactoryAction[]
};
onAppClosed?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same ad s above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evidolob
Copy link
Contributor Author

evidolob commented Dec 6, 2018

I create separate issue for Che DTO generator eclipse-che/che#12132

*
* SPDX-License-Identifier: EPL-2.0
**********************************************************************/
import './che-proposed';
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 need this import ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, other vice types from che-proposed is not visible in API implementation

@evidolob evidolob merged commit 8567332 into master Dec 10, 2018
@evidolob evidolob deleted the che-api branch February 5, 2019 08:49
monaka referenced this pull request in opentestmodeling/che-theia Jul 1, 2019
vinokurig pushed a commit that referenced this pull request Apr 6, 2021
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