Ability to register language servers for file names instead of extensions #5107 #5156

tsmaeder · 2017-05-22T14:12:14Z

What does this PR do?

This PR allows language server plugins to register language servers for specific file names (for example pom.xml). Until now, it is only possible to register for filename extension (like *.xml).
The PR introduces two major changes:

Multiple extensions/or and file names can be mapped to a single mime type and language id. This solves the problem that the mime type was only registered for the first extension when multiples were given.
Language registration and language server launcher registration are separated. This way, we can still mimic the old behavior and register multiple mime types for the same language id. As an additional advantage, this change will be useful when allowing to register multiple language servers for the same language in the future.

What issues does this PR fix or reference?

fixes #5107

Changelog

Ability to register language servers for file names instead of extensions

Release Notes

Language Server Support: Add ability to register language servers for file names instead of extensions

Docs PR

eclipse-che/che-docs#230

codenvy-ci · 2017-05-22T14:14:01Z

Can one of the admins verify this patch?

skabashnyuk · 2017-05-22T14:56:14Z

plugins/plugin-languageserver/che-plugin-languageserver-ide/pom.xml

@@ -149,7 +149,7 @@
                <version>${project.version}</version>
                <executions>
                    <execution>
-                        <phase>process-sources</phase>
+                        <phase>generate-sources</phase>


last time we are not able to release with <phase>generate-sources</phase> I think there is a compilation issue in this phase. Why do you want to change it? Can you compile this artifact in offline mode if you local is empty or doesn't contains che-plugin-languageserver-ide artifact

The maven generator plugin is using compiled dependencies to generate the dto classes. Since no dto's are generated from classes in the project itself, this should be fine. In the core.api.languageserver project, there used to be a precompile step in order to be able to generate from classes in the project itself.

Have you checked that I've asked? Because we had troubles with this last time during release.

It's impossible to build in offline mode when the local maven cache is empty. I can remove che stuff from the cache, though.

It's impossible to build in offline mode when the local maven cache is empty

Ok. I wasn't really clear. I mean remove only org.eclipse.che artifacts or to be more precise che-plugin-languageserver-ide and when rebuilt che-plugin-languageserver-ide in offline mode. It's should be possible. Same situation we have when maven release plugin is releasing.

ok. I've checked looks like it's working

thx. I was having trouble building from scratch all last week (some tests failing in unrelated projects)

skabashnyuk · 2017-05-22T14:56:43Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

 /**
 * @author Evgen Vidolob
 */
 @Singleton
 public class LanguageServerFileTypeRegister implements WsAgentComponent {
-
+    private static Logger logger = Logger.getLogger(LanguageServerFileTypeRegister.class.getName());


skabashnyuk · 2017-05-22T14:57:48Z

...ugin-python-lang-server/src/main/java/org/eclipse/che/plugin/python/inject/PythonModule.java

+    private static final String[] EXTENSIONS = new String[]{ProjectAttributes.PYTHON_EXT};
+    private static final String MIME_TYPE = "text/x-python";
+    static {
+    }


redundant code

sunix · 2017-05-22T14:18:05Z

...ver/src/main/java/org/eclipse/che/plugin/json/languageserver/JsonLanguageServerLauncher.java

@@ -58,8 +49,7 @@ public boolean isAbleToLaunch() {
    }

    protected LanguageServer connectToLanguageServer(final Process languageServerProcess, LanguageClient client) {
-        Launcher<LanguageServer> launcher = Launcher.createLauncher(client, LanguageServer.class,


if this is only formatting, I would include in a seperated PR or commit

That would mean that I have to create an issue and open a PR just for a formatting change? I politely disagree here. My favorite solution would be to automate formatting completely, then formatting changes would never occur.

at least seperate in 2 commits in the same PR

sunix · 2017-05-22T14:24:38Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

        this.resources = resources;
        this.editorRegistry = editorRegistry;
        this.contentTypeRegistrant = contentTypeRegistrant;
-        this.orionHoverRegistrant = orionHoverRegistrant;
+        this.orionHoverRegistrant = orionHoverRegistrant; 


I guess this is not wanted

sunix · 2017-05-22T14:37:31Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

    @Inject
-    public LanguageServerFileTypeRegister(LanguageServerRegistryServiceClient serverLanguageRegistry,
-                                          FileTypeRegistry fileTypeRegistry,


mixing format and changes makes review and merge harder

I see what you mean here. And Githubs line based diff algorithm does not help at all.

sunix · 2017-05-22T15:36:07Z

...src/main/java/org/eclipse/che/plugin/languageserver/ide/registry/LanguageServerRegistry.java

-                                              final NotificationManager notificationManager,
-                                              final EventBus eventBus) {
+    protected void subscribeToInitializeEvent(final DtoUnmarshallerFactory unmarshallerFactory, final MessageBusProvider messageBusProvider,
+                                              final NotificationManager notificationManager, final EventBus eventBus) {


just format ?

sunix · 2017-05-22T15:36:16Z

...src/main/java/org/eclipse/che/plugin/languageserver/ide/registry/LanguageServerRegistry.java

-                Unmarshallable<LanguageServerInitializeEvent> unmarshaller =
-                        unmarshallerFactory.newWSUnmarshaller(LanguageServerInitializeEvent.class);
+                Unmarshallable<LanguageServerInitializeEvent> unmarshaller = unmarshallerFactory
+                                .newWSUnmarshaller(LanguageServerInitializeEvent.class);


just format ?

sunix · 2017-05-22T15:36:36Z

...src/main/java/org/eclipse/che/plugin/languageserver/ide/registry/LanguageServerRegistry.java

                                     initializeEvent.getServerCapabilities());
                        }

                        @Override
                        protected void onErrorReceived(Throwable exception) {
-                            notificationManager.notify(exception.getMessage(),
-                                                       StatusNotification.Status.FAIL,
+                            notificationManager.notify(exception.getMessage(), StatusNotification.Status.FAIL,


just format ?

sunix · 2017-05-22T15:38:03Z

.../src/main/java/org/eclipse/che/plugin/python/languageserver/PythonLanguageSeverLauncher.java

    }
-
+    


trailing space/tab not to be included

sunix · 2017-05-22T15:38:13Z

...-plugin-web-ext-server/src/main/java/org/eclipse/che/plugin/web/typescript/TSLSLauncher.java

    }
-
+    


trailing space/tab not to be included

sunix · 2017-05-22T15:39:42Z

...eserver/src/main/java/org/eclipse/che/api/languageserver/registry/ServerInitializerImpl.java

        InitializeParams initializeParams = prepareInitializeParams(projectPath);

        LanguageServer server;
        try {
            server = launcher.launch(projectPath, languageClient);
        } catch (LanguageServerException e) {
            throw new LanguageServerException(
-                    "Can't initialize Language Server " + languageId + " on " + projectPath + ". " + e.getMessage(), e);
+                            "Can't initialize Language Server " + languageId + " on " + projectPath + ". " + e.getMessage(), e);


just format ?

sunix · 2017-05-22T15:39:58Z

...eserver/src/main/java/org/eclipse/che/api/languageserver/registry/ServerInitializerImpl.java

@@ -172,7 +162,7 @@ protected LanguageServer doInitialize(LanguageServerLauncher launcher, String pr
    protected void registerCallbacks(LanguageServer server) {

        if (server instanceof ServerInitializerObserver) {
-            addObserver((ServerInitializerObserver)server);
+            addObserver((ServerInitializerObserver) server);


just format ?

tsmaeder · 2017-05-29T09:10:38Z

I'll get back to this second half of this week.

skabashnyuk · 2017-06-02T15:05:55Z

...json/che-plugin-json-server/src/main/java/org/eclipse/che/plugin/json/inject/JsonModule.java

+
+
+    static {
+    }


unused block

skabashnyuk · 2017-06-02T15:06:06Z

...json/che-plugin-json-server/src/main/java/org/eclipse/che/plugin/json/inject/JsonModule.java

+                                                             "babelrc"};
+    private static final String MIME_TYPE  = "application/json";
+
+


extra spaces

skabashnyuk · 2017-06-02T15:06:53Z

.../src/main/java/org/eclipse/che/plugin/python/languageserver/PythonLanguageSeverLauncher.java

-        description.setMimeTypes(Arrays.asList(MIME_TYPES));
-    }
-}
+  }


extra spaces

evidolob · 2017-06-15T06:57:50Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/inject/LanguageServerGinModule.java


        GinMapBinder<String, WsAgentComponent> wsAgentComponentsBinder =
                GinMapBinder.newMapBinder(binder(), String.class, WsAgentComponent.class);
        wsAgentComponentsBinder.addBinding("Load Language Server file types.").to(LanguageServerFileTypeRegister.class);
-
-        bind(PublishDiagnosticsReceiver.class).asEagerSingleton();
-        bind(ShowMessageJsonRpcReceiver.class).asEagerSingleton();


Why you delete this bindings?

That is a mistake. It also explains why notifications don't work in my current working branch. Good catch!

benoitf · 2017-06-19T13:07:39Z

...json/che-plugin-json-server/src/main/java/org/eclipse/che/plugin/json/inject/JsonModule.java

@@ -15,15 +15,29 @@

 import org.eclipse.che.inject.DynaModule;
 import org.eclipse.che.plugin.json.languageserver.JsonLanguageServerLauncher;
+
+import static java.util.Arrays.asList;


I think the static import is not correctly sorted

benoitf · 2017-06-19T13:08:17Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

                                          HoverProvider hoverProvider,
-                                          OccurrencesProvider occurrencesProvider) {
+                                          OccurrencesProvider occurrencesProvider) { 


why do we have trailing spaces ?

@tsmaeder you have to be consistent, sometimes you add trailing spaces, sometimes not. Can you remove them ?

benoitf · 2017-06-27T11:52:44Z

...src/main/java/org/eclipse/che/plugin/languageserver/ide/registry/LanguageServerRegistry.java

+
+    /**
+     * Register file type for a language description
+     * @param type


maybe add description around these type and description or remove the javadoc ?

fair enough.

benoitf · 2017-06-27T11:53:12Z

...src/main/java/org/eclipse/che/plugin/languageserver/ide/registry/LanguageServerRegistry.java

+    /**
+     * Get the language that is registered for this file. May return null if none is found.
+     * @param file
+     * @return


maybe add description around file and what is returned or remove the javadoc ?

benoitf · 2017-06-27T11:59:39Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

+                                      Provider<ProjectManager> projectManagerProvider, ServerInitializer initializer) {
+        this.languages.addAll(languages);
+        this.launchers.putAll(languageServerLaunchers.stream()
+                        .collect(Collectors.toMap(LanguageServerLauncher::getLanguageId, launcher -> launcher)));


if languages and launchers are initialized there, do we need to initialize the fields and use addAll/putAll after instead of just using the new directly there

this.languages = new ArrayList<>(languages);

vs

List list = new ArrayList<>() (when declaring field) list.addAll(myElements) (in constructor)

yes, generally, I like to initialized fields that are not passed as init parameters in the field declaration; it makes sure the field has a usefule value.

as it's final fields, we're sure that they're forced to be initialized

benoitf · 2017-06-27T12:16:16Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

        }
+        return false;
+    }


return Optional.ofNullable(language.getFileExtensions()).map(Collection::stream).orElse(Stream.empty()).anyMatch(extension -> path.endsWith(extension)); ?

Sorry, but I strongly disagree here: while your version has fewer characters, I find it enormously more difficult to read. When did we decide that if and for are "harmful" (with my apologies to the late Professor Goto)?

It may be subjective but as JDK8 is out since a long time, I assume that ppl are fluent on using streams.
BTW if getFileExtensions would always return something (like empty) it could have been updated to only

return language.getFileExtensions().stream().anyMatch(extension -> path.endsWith(extension));

which is definitely better.

Apart from the difficulty enforcing the non-nullness, I disagree with the statement that the stream version is "better". It's shorter, I agree, but try putting a breakpoint into your version, for example.

benoitf · 2017-06-27T12:16:57Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

+                }
+            } 
+        }
+        return false;


could java streams make it more readable/smaller ?

Same comment as above: smaller, maybe, but not more readable.

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder

???

tsmaeder · 2017-06-29T11:55:48Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

        }
+        return false;
+    }


Sorry, but I strongly disagree here: while your version has fewer characters, I find it enormously more difficult to read. When did we decide that if and for are "harmful" (with my apologies to the late Professor Goto)?

tsmaeder · 2017-06-29T11:56:40Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

+                }
+            } 
+        }
+        return false;


Same comment as above: smaller, maybe, but not more readable.

benoitf · 2017-06-29T12:07:29Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

-                            fileTypeRegistry.registerFileType(fileType);
-                            editorRegistry.registerDefaultEditor(fileType, editorProvider);
-                            ext2langId.put(ext, lang.getLanguageId());
+                        if (lang.getFileExtensions() != null) {


could we make getFileExtensions() always returning non-null list

We could avoid almost useless if (lang.getFileExtensions() != null)

That sounds good, but it's not easily possible since LanguageDescription has a setter for the file extensions. So I can't really guarantee that it's not null.

@tsmaeder yes LanguageDescription has a setter.
But the setter could modify the internal list only if the given element is not null instead of setting the given value directly without any check.

for example in LanguageDescription we could have
constructor : initialize fileExtensions to an empty list

in the setter: update this internal list only if the provided list is not null. (we could also make it immutable list by copying the elements of the provided list to the internal list and not reusing the provided list but it's another topic. Getter returning a copy of the list as well)

we could also in getter, return Collections.emptyList if the internal list is null. (either change setter logic or getter logic)

so, even with a setter, getFileExtensions is always returning a not null list

benoitf · 2017-06-29T12:07:57Z

.../src/main/java/org/eclipse/che/plugin/languageserver/ide/LanguageServerFileTypeRegister.java

-                        List<String> mimeTypes = lang.getMimeTypes();
-                        if (mimeTypes.isEmpty()) {
-                            mimeTypes = newArrayList("text/x-" + lang.getLanguageId());
+                        if (lang.getFileNames() != null) {


same remark here for getFileNames()

again, I can't enforce non-null here.

gazarenkov · 2017-06-29T13:49:39Z

@sunix @tsmaeder as I can see there are requested changes. What is the status of this issue? Are you guys able to come into agreement?

sunix

LGTM

Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf · 2017-07-03T09:07:37Z

...hared/src/main/java/org/eclipse/che/api/languageserver/shared/model/LanguageDescription.java

     */
-    private List<String> fileExtensions;
+    private List<String> fileExtensions= Collections.emptyList();


fileExtensions<space>=

benoitf · 2017-07-03T09:09:32Z

...er/src/main/java/org/eclipse/che/api/languageserver/registry/LanguageServerRegistryImpl.java

-                                      ServerInitializer initializer) {
+    public LanguageServerRegistryImpl(Set<LanguageServerLauncher> languageServerLaunchers, Set<LanguageDescription> languages,
+                                      Provider<ProjectManager> projectManagerProvider, ServerInitializer initializer) {
+        this.languages= new ArrayList<>(languages);


there is a missing space before the =

Thank you for making my point about auto-formatting ;-)

benoitf · 2017-07-03T09:14:23Z

ci-build

codenvy-ci · 2017-07-03T10:42:33Z

Build # 2976 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2976/ to view the results.

benoitf · 2017-07-03T10:55:35Z

@tsmaeder there is a test error there : https://ci.codenvycorp.com/job/che-pullrequests-build/org.eclipse.che.core$che-core-api-languageserver/2976/testReport/junit/org.eclipse.che.api.languageserver.registry/LanguageServerRegistryImplTest/testFindByPattern/

benoitf · 2017-07-03T11:47:53Z

...hared/src/main/java/org/eclipse/che/api/languageserver/shared/model/LanguageDescription.java

    public void setFileExtensions(final List<String> fileExtensions) {
-        this.fileExtensions = fileExtensions;
+        this.fileExtensions = new ArrayList<>(fileExtensions);


it seems per the test result that we should do the assignment only if fileExtensions is not null (or update the test to comply with javadoc)

Test is bogus.

Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf · 2017-07-03T12:44:13Z

ci-build

codenvy-ci · 2017-07-03T15:07:46Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2981/

…ions eclipse-che#5107 (eclipse-che#5156) * Register languages by file name Signed-off-by: Thomas Mäder <tmader@redhat.com>

skabashnyuk reviewed May 22, 2017

View reviewed changes

tsmaeder mentioned this pull request May 22, 2017

5107 register by filename eclipse-che/che-docs#230

Merged

sunix requested changes May 22, 2017

View reviewed changes

slemeur self-requested a review May 29, 2017 08:08

slemeur added the kind/enhancement A feature request - must adhere to the feature request template. label May 29, 2017

slemeur approved these changes May 31, 2017

View reviewed changes

tsmaeder force-pushed the 5107_register_by_filename branch from 8d8b275 to 8392c8b Compare June 2, 2017 12:04

skabashnyuk reviewed Jun 2, 2017

View reviewed changes

tsmaeder force-pushed the 5107_register_by_filename branch from e17591e to 4f54fff Compare June 6, 2017 07:48

vparfonov added sprint/next team/ide labels Jun 8, 2017

gazarenkov requested a review from evidolob June 14, 2017 18:54

evidolob suggested changes Jun 15, 2017

View reviewed changes

tsmaeder force-pushed the 5107_register_by_filename branch from 4f54fff to 33c3ff3 Compare June 19, 2017 12:59

evidolob approved these changes Jun 19, 2017

View reviewed changes

benoitf reviewed Jun 19, 2017

View reviewed changes

tsmaeder mentioned this pull request Jun 22, 2017

Add support for using multiple language servers to be registered for the same files #4609 #5442

Merged

tsmaeder force-pushed the 5107_register_by_filename branch from 3840d30 to 74aa83d Compare June 27, 2017 11:14

benoitf reviewed Jun 27, 2017

View reviewed changes

tsmaeder added 4 commits June 29, 2017 11:34

Register languages by file name

8b537bb

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Remove bogus binding.

1d11841

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Remove dead code

6bf0e57

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Renaned logger constant

e450b58

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder added 4 commits June 29, 2017 11:34

Formatting and removing unused code

5c825a7

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Revive event receiver bindings

d48c3f8

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Remove extra spaces

68e2932

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Formatting

e2067e2

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder commented Jun 29, 2017

View reviewed changes

benoitf reviewed Jun 29, 2017

View reviewed changes

benoitf changed the title ~~5107 register by filename~~ Ability to register language servers for file names instead of extensions #5107 Jun 29, 2017

sunix approved these changes Jun 30, 2017

View reviewed changes

Addressing PR comments

2a53bb0

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 5107_register_by_filename branch from 41cb53c to 2a53bb0 Compare June 30, 2017 11:15

benoitf assigned tsmaeder Jun 30, 2017

Ensure non-null for file names and extensions

25a6ded

Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf reviewed Jul 3, 2017

View reviewed changes

Fix formatting & bogus test

3ed6a52

Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf approved these changes Jul 3, 2017

View reviewed changes

benoitf merged commit 9027aa4 into eclipse-che:master Jul 3, 2017

benoitf added team/californication and removed sprint/next team/ide labels Jul 3, 2017

benoitf added this to the 5.15.0 milestone Jul 3, 2017

sunix mentioned this pull request Jul 4, 2017

[Rebase oscon] Fix startup of bayesian analytics language server redhat-developer/rh-che#166

Closed

		"babelrc"};
		private static final String MIME_TYPE = "application/json";

Ability to register language servers for file names instead of extensions #5107 #5156

Ability to register language servers for file names instead of extensions #5107 #5156

Conversation

tsmaeder commented May 22, 2017 • edited by benoitf Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

codenvy-ci commented May 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Jun 2, 2017 • edited Loading

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

tsmaeder commented May 29, 2017

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

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

benoitf Jun 27, 2017 • edited Loading

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

tsmaeder left a 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

gazarenkov commented Jun 29, 2017 • edited Loading

sunix left a comment

Choose a reason for hiding this comment

benoitf Jul 3, 2017 • edited Loading

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

benoitf commented Jul 3, 2017

codenvy-ci commented Jul 3, 2017

benoitf commented Jul 3, 2017

benoitf Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

tsmaeder commented May 22, 2017 •

edited by benoitf

Loading

tsmaeder Jun 2, 2017 •

edited

Loading

benoitf Jun 27, 2017 •

edited

Loading

gazarenkov commented Jun 29, 2017 •

edited

Loading

benoitf Jul 3, 2017 •

edited

Loading

benoitf Jul 3, 2017 •

edited

Loading