HTTP Interface #495

lukroth · 2016-12-19T07:59:28Z

Content of the pull request:

added an new HTTP Interface
added a new 'file_save', 'file_list', and 'file_delete' command
added a new 'add_dependency' and 'remove_dependency' command
added new test cases for the new commands and the HTTP Interface

Reason for our pull request:

We are creating an online editor (we took codemirror as a base editor) which provides code completion functionality for groovy code. To achieve this, we run an eclipse instance in the background. To get the completion proposals out of eclipse we selected the eclim plug-in. Since eclipse should run in a docker image we do not want to use the “command line interface”, but would prefer a HTTP interface which provides the functionality of eclim.

We need the additional commands to control eclim completely over the HTTP Interface.
With the file_save command we can

save code files and
save jar dependencies (which we later add to the .classpath file of eclipse over the add_dependency command)

- added an new HTTP Interface - added a new 'file_save', 'file_list', and 'file_delete' command - added a new 'add_dependency' and 'remove_dependency' command - added new test cases for the new commands and the HTTP Interface

ervandew · 2016-12-27T18:04:30Z

ant/build.gant

-    ant.classpath{ant.pathelement(path: '${build.classes}/org.eclim')}
+    ant.classpath{
+      ant.pathelement(path: '${build.classes}/org.eclim')
+      ant.pathelement(path: '${build.classes}/org.eclim.jdt')


This should be changed to to use ${pluginName} rather than hardcoding in jdt, which not all users may be able to compile (they may only be interested in running/testing cdt for example).

I agree, thanks for the comment!
See changes in the next commit.

ervandew · 2016-12-27T18:05:27Z

ant/build.gant

@@ -1130,6 +1133,7 @@ junit = { pluginName ->
      ant.path(refid: 'junit')
      ant.pathelement(path: "build/test/junit/classes/${pluginName}")
      ant.pathelement(path: '${build.classes}/org.eclim')
+      ant.pathelement(path: '${build.classes}/org.eclim.jdt')


Again, this shouldn't be hardcoded as jdt.

Also done in the next commit.

ervandew · 2016-12-27T18:10:05Z

org.eclim.core/java/org/eclim/plugin/core/command/file/FileSaveCommand.java

+    int len;
+    while ((len = in.read(buf)) > 0) {
+      out.write(buf, 0, len);
+    }


You can remove this code and replace it with org.eclim.util.IOUtils.copy(in, out).

Sorry, I did not see this Utility class.
I changed it -> See changes in the next commit.

ervandew · 2016-12-27T18:16:54Z

org.eclim.jdt/java/org/eclim/plugin/jdt/command/dependency/AddDependencyCommand.java

+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.eclim.plugin.jdt.command.dependency;


There is already a package for .classpath commands at jdt.command.classpath. Can you move your new commands there.

Done - i moved all my classes from the dependency package including the test cases to the classpath package.
See changes in the next commit.

ervandew · 2016-12-27T18:18:39Z

org.eclim.jdt/java/org/eclim/plugin/jdt/command/dependency/AddDependencyCommand.java

+import org.eclim.plugin.core.util.PathUtilException;
+
+@Command(
+      name = "add_dependency",


For consistency and to prevent collisions, all language specific commands have a common prefix. Since this is a jdt (java) command, this should have a java_ prefix. For consistency with the existing classpath commands I'd call this java_classpath_add_entry.

Done - Changed the add_dependency and also the remove_dependency to java_classpath_add_entry and java_classpath_remove_entry.
See changes in the next commit.

ervandew · 2016-12-27T18:21:50Z

org.eclim/plugin.properties

@@ -1,4 +1,7 @@
 pluginVersion=${eclim.version}
 nailgun.server.host=127.0.0.1
 nailgun.server.port=9091
+http.server.enabled=true


Please disable this by default. Currently no other users will be using this.

Makes sense - this was the initial reason why I introduced this flag -> I just forgot to set it to false by default.
Now changed -> See changes in the next commit.

ervandew · 2016-12-27T18:25:33Z

I scanned through this and left some comments. I'll need to spend more time looking at this more closely and testing, but for the most part it looks good.

Please run $ ant checkstyle for this and address any failures (Note: i fixed the jdt SearchCommand failures on master).

…iew round' of ervandew - build.gant: use ${pluginName} instead of hard coding - use IOUtils in FileSaveCommand - move org.eclim.plugin.jdt.command.dependency to jdt.command.classpath - disable http server by default - rename add_dependency and remove_dependency commands to java_classpath_add_entry and java_classpath_remove_entry

lukroth · 2017-01-03T13:43:54Z

First: Thanks for your comments and your time you took to look at the code!

I changed everything accordingly to your suggestions.
I ran the $ ant checkstyle command already :) -> it should be fine?
(I just realized that the checkstyle command does not check the junit test classes?, is there any reason for this?)

lukroth · 2017-01-03T16:25:43Z

Note: Before you merge the code: Please run the junit tests on your machine to ensure all tests pass - i had to comment some tests out which did never run on my system (https://groups.google.com/forum/#!msg/eclim-dev/O8dqcvOEFWI/_VSC3ncmDwAJ)

Please do so also for #496 and #497
Thanks!

- changed wrong logging call (tried to log an object which is no instance of exception in an error logging call)

The HTTP Server returned an error message if a command returns null. Since there exist commands which return null when they succeed (e.g. project_refresh_file) we changed this. The HTTP Server now returns an empty string in the message if a command returns null.

…iew round' of ervandew - build.gant: use ${pluginName} instead of hard coding - use IOUtils in FileSaveCommand - move org.eclim.plugin.jdt.command.dependency to jdt.command.classpath - disable http server by default - rename add_dependency and remove_dependency commands to java_classpath_add_entry and java_classpath_remove_entry

- changed wrong logging call (tried to log an object which is no instance of exception in an error logging call)

The HTTP Server returned an error message if a command returns null. Since there exist commands which return null when they succeed (e.g. project_refresh_file) we changed this. The HTTP Server now returns an empty string in the message if a command returns null.

ervandew · 2017-01-18T18:55:56Z

I just wanted to leave a note that I haven't forgotten about these pull requests. I've just haven't had the time to fully review them yet (I was hoping to tackle a couple the past weekend, but my internet went out friday afternoon and wasn't back until monday afternoon).

vitkoczi · 2017-08-11T08:45:43Z

hi @ervandew

do you plan to have a look?
we would need the rest interface

HTTP Interface

0bd94b7

- added an new HTTP Interface - added a new 'file_save', 'file_list', and 'file_delete' command - added a new 'add_dependency' and 'remove_dependency' command - added new test cases for the new commands and the HTTP Interface

ervandew reviewed Dec 27, 2016

View reviewed changes

lukroth added 2 commits January 4, 2017 17:00

HTTP Interface ervandew#495: small bug in logging message

4185c48

- changed wrong logging call (tried to log an object which is no instance of exception in an error logging call)

lukroth added a commit to lukroth/eclim that referenced this pull request Jan 11, 2017

HTTP Interface ervandew#495: small bug in logging message

afa29cb

- changed wrong logging call (tried to log an object which is no instance of exception in an error logging call)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP Interface #495

HTTP Interface #495

lukroth commented Dec 19, 2016

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew Dec 27, 2016

lukroth Jan 3, 2017

ervandew commented Dec 27, 2016

lukroth commented Jan 3, 2017

lukroth commented Jan 3, 2017

ervandew commented Jan 18, 2017

vitkoczi commented Aug 11, 2017

HTTP Interface #495

Are you sure you want to change the base?

HTTP Interface #495

Conversation

lukroth commented Dec 19, 2016

Content of the pull request:

Reason for our pull request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ervandew commented Dec 27, 2016

lukroth commented Jan 3, 2017

lukroth commented Jan 3, 2017

ervandew commented Jan 18, 2017

vitkoczi commented Aug 11, 2017