Add CheEditor object to the Workspace Next model by garagatyi · Pull Request #10457 · eclipse-che/che

garagatyi · 2018-07-17T15:10:09Z

What does this PR do?

Introduces changes in Workspace.Next model objects.
Field 'commands' in CheContainer renamed to editor-commands.
Add CheEditor object that has all the fields ChePlugin previously had.
ChePlugin gets field editors that contains a list of objects called in PR EditorCompatibility.
EditorCompatibility contains fields name and

What issues does this PR fix or reference?

Fixes #10203

Release Notes

Docs PR

sleshchenko · 2018-07-18T05:53:54Z

deploy/kubernetes/kubectl/wsnext/Readme.md


-`-t garagatyi/che-marketplace:1` is needed to set identifier of the image. Do not forget to change `feature-api/yaml` file accordingly if you change this identifier to something else.
+`-t garagatyi/che-marketplace:os` is needed to set identifier of the image.
+Do not forget to change `feature-api/yaml` file accordingly if you change this identifier to something else.


Should feature-api/yaml be feature-api.yaml?

feature-api/yaml - what is it?

@sleshchenko thanks
@skabashnyuk it is typo

feature-api/yaml is this a file? Are we going to use it?

sleshchenko · 2018-07-18T05:59:21Z

...ace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/EditorCompatibility.java

+    return name;
+  }
+
+  public void name(String name) {


As far as I understand methods which are named as a field are like with methods, like e89a9fb#diff-67aebb3b9dc0621b7a6f25e408c74a1dR30. Should not it return EditorCompatibility?

Nice catch, tnx

sleshchenko · 2018-07-18T06:03:32Z

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/ChePlugin.java

-        + ", endpoints="
-        + endpoints
-        + '}';
+    return "ChePlugin{" + "editors=" + editors + "} " + super.toString();


Are you sure that it's a good idea to have such output

ChePlugin{editors=[....]} PluginBase{name=qwe...}

I'm not sure. Do you think that it is bad?

It may be confused in such cases:

LOG.debug("Debug info {} {}", chePlugin, otherPluginBase); as output: "Debug info ChePlugin{editors=[....]} PluginBase{name=qwe...} PluginBase{name=qwe...}"

Looks like three objects are printed =)
I think that it would be better to group fields of one objects, typically for Che code base with {}

Makes sense

sleshchenko · 2018-07-18T06:05:44Z

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/CheEditor.java

+      return false;
+    }
+    CheEditor cheEditor = (CheEditor) o;
+    return Objects.equals(getName(), cheEditor.getName())


I think it would be better to write equals and hashcode methods in the same manner e89a9fb#diff-b3ca1f169efa0ad30c481db6568521a7R41

Can you elaborate why it would be better?

Let me clarify that I wanted to say that it is better to write these methods in the same way, but both ways are OK for me (with reusing super methods, or with a duplicated piece of code).
I see two points here:

Nobody will ask why two very similar classes have a different style of equals and hashcode method.

One style is a less error-prone way because you get used to writing in this way, and you'll find an error easier.

like here https://github.com/eclipse/che/pull/10457/files#diff-a8707ef642f5ec96204cccfd4e370ce3R40 is not an error but overhead when ChePluginBase fields are used twice in hash code, the first time in super.hashCode, and the second time in CheEditor.

I see. But you may also consider using super as a less error-prone approach since it reflects all the changes in a superclass automatically and that adding a field to a superclass and forgetting modifying a subclass doesn't introduce a bug.
I fixed the hashcode method, thanks for noticing it.

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/CheEditor.java

+ *
+ * <p>It may be classic GWT IDE, Eclipse Theia or something else.
+ */
+public class CheEditor extends PluginBase {


sleshchenko · 2018-07-18T06:17:47Z

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/ChePlugin.java

-public class ChePlugin {
+/** Represents Che plugin in sidecar-powered workspace. */
+public class ChePlugin extends PluginBase {
+  private List<EditorCompatibility> editors = new ArrayList<>();


I'm not sure that I fully understand all last changes in WsNext Skeleton, and it's just a note to consider: in editors field I would expect to see a list of CheEditors objects, or list of CheEditors identifiers.

editor field contains a list of editors supported by a plugin with identifiers in a field name and other plugins that needed for correct integration of the editor and the plugin in a field plugins. All of that is incorporated in EditorCompatibility object in this PR.
Does this clarify the idea to you?

Yeah, thanks for the explanation.

Does it mean that different CheEditors can be used at the same time? It's OK just to clarify.

As far as I understand ChePlugin depends on EditorCompatibilitys, EditorCompatibility depends on ChePlugins. Are circular dependencies are handled correctly?

ChePlugin shows that different plugins might be used with it, but Che should ensure that just one is in use.
Circular dependencies are not handled because compatibility is not implemented and I'm not sure we need this verification on this step where parts are still moving rapidly and things that are here now might be gone tomorrow.

skabashnyuk · 2018-07-18T08:23:25Z

deploy/kubernetes/kubectl/wsnext/Readme.md


-`-t garagatyi/che-marketplace:1` is needed to set identifier of the image. Do not forget to change `feature-api/yaml` file accordingly if you change this identifier to something else.
+`-t garagatyi/che-marketplace:os` is needed to set identifier of the image.
+Do not forget to change `feature-api.yaml` file accordingly if you change this identifier to something else.


Now we have editor and plugins. Maybe we should rename feature-api.yaml to something different. Because we don't have features anymore.

I don't want to change it because there is fresh blog post from Mario that uses instructions where this name is used.

oook. however, it looks weird. For how long are you going to keep it in this way?

I don't know. Is it important to you?

sleshchenko

Please take a look one more time that equals and hashCode methods are written properly and they use the same fields for computing a result

garagatyi · 2018-07-18T13:45:28Z

ci-build

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi · 2018-07-19T07:22:16Z

@riuvshin do you happen to know why CI didn't respond to my comment that supposed to start a build on CI?

garagatyi requested a review from a user July 17, 2018 15:10

garagatyi requested review from l0rd, riuvshin and sleshchenko as code owners July 17, 2018 15:10

garagatyi requested review from ibuziuk and removed request for a user and riuvshin July 17, 2018 15:12

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jul 17, 2018

sleshchenko reviewed Jul 18, 2018

View reviewed changes

skabashnyuk reviewed Jul 18, 2018

View reviewed changes

garagatyi force-pushed the wsnextCheEditor branch from 544ab71 to 3db106b Compare July 18, 2018 13:06

sleshchenko approved these changes Jul 18, 2018

View reviewed changes

skabashnyuk approved these changes Jul 18, 2018

View reviewed changes

Oleksandr Garagatyi added 2 commits July 19, 2018 09:20

Add CheEditor object to the Workspace Next model

421d58e

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Small fixes WS.NEXT deployment readme

3ebcbe0

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the wsnextCheEditor branch from 7236ab1 to 3ebcbe0 Compare July 19, 2018 07:44

garagatyi merged commit e17fbed into eclipse-che:master Jul 19, 2018

garagatyi deleted the wsnextCheEditor branch July 19, 2018 07:54

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 19, 2018

benoitf added this to the 6.9.0 milestone Jul 19, 2018

Conversation

garagatyi commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleshchenko Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garagatyi Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

garagatyi commented Jul 18, 2018

Uh oh!

garagatyi commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garagatyi commented Jul 17, 2018 •

edited

Loading

sleshchenko Jul 18, 2018 •

edited

Loading

garagatyi Jul 18, 2018 •

edited

Loading