Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignoring failures when checking for hyperlinks #1186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −20
Open

Ignoring failures when checking for hyperlinks #1186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions org.eclipse.lsp4e/.project
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,16 @@
<arguments>
</arguments>
</buildCommand>
<buildCommand>
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks unrelated to the ticket you are resolving. Could you move the change to a new PR so that it can be looked at independently?

<name>org.whitesource.eclipse.plugin.WSbuilder</name>
<arguments>
</arguments>
</buildCommand>
</buildSpec>
<natures>
<nature>org.eclipse.m2e.core.maven2Nature</nature>
<nature>org.eclipse.pde.PluginNature</nature>
<nature>org.eclipse.jdt.core.javanature</nature>
<nature>org.whitesource.eclipse.plugin.WSNature</nature>
</natures>
</projectDescription>
44 changes: 34 additions & 10 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@
*******************************************************************************/
package org.eclipse.lsp4e;

import org.eclipse.core.resources.IProject;
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like organize imports is configured different than by default. Could you change it so that the PR does not change the order of the import in all the modified files?

import org.eclipse.core.runtime.Assert;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jface.text.IDocument;
import org.eclipse.lsp4e.LanguageServersRegistry.LanguageServerDefinition;
import org.eclipse.lsp4e.internal.ArrayUtil;
import org.eclipse.lsp4j.ServerCapabilities;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.eclipse.lsp4j.services.LanguageServer;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -27,16 +37,6 @@
import java.util.function.Predicate;
import java.util.stream.Stream;

import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.Assert;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jface.text.IDocument;
import org.eclipse.lsp4e.LanguageServersRegistry.LanguageServerDefinition;
import org.eclipse.lsp4e.internal.ArrayUtil;
import org.eclipse.lsp4j.ServerCapabilities;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.eclipse.lsp4j.services.LanguageServer;

/**
* Main entry point for accessors to run requests on the language servers, and some utilities
* for manipulating the asynchronous response objects in streams
Expand Down Expand Up @@ -410,6 +410,30 @@ public static <T> CompletableFuture<List<T>> addAll(CompletableFuture<List<T>> a
return res;
}

/**
* Combines two async lists of results into a single list by adding all the elements of the second list to the first one,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Combines two async lists of results into a single list by adding all the elements of the second list to the first one,
* Combines async lists of results into a single list by adding all the elements of each lists to a new list

* ignoring abnormal results and only including succeses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ignoring abnormal results and only including succeses.
* ignoring any CompletableFuture that completed exceptionally.

*
* @param <T> Result type
* @param futures Async results
* @return Async combined result
*/
public static <T> CompletableFuture<List<T>> addAllSuccessful(CompletableFuture<List<T>>... futures) {
CompletableFuture<List<T>> res = CompletableFuture.allOf(futures)
.thenApply( v -> {
List<T> results = new ArrayList<>(futures.length);
for(CompletableFuture<List<T>> f : futures) {
if (!f.isCompletedExceptionally()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of just ignoring exceptions, I think we should log them using LanguageServerPlugin.logError

results.addAll(f.join());
}
}
return results;
});
forwardCancellation(res, futures);
return res;
}


/**
* Retrieves the initialized servers and apply the given query.
* <p>The query must ideally be a direct query to the language server
Expand Down
20 changes: 10 additions & 10 deletions ....lsp4e/src/org/eclipse/lsp4e/operations/declaration/OpenDeclarationHyperlinkDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,6 @@
*******************************************************************************/
package org.eclipse.lsp4e.operations.declaration;

import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jface.text.BadLocationException;
Expand All @@ -43,6 +34,15 @@
import org.eclipse.lsp4j.TextDocumentPositionParams;
import org.eclipse.lsp4j.jsonrpc.messages.Either;

import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

public class OpenDeclarationHyperlinkDetector extends AbstractHyperlinkDetector {

@Override
Expand All @@ -68,7 +68,7 @@ public class OpenDeclarationHyperlinkDetector extends AbstractHyperlinkDetector
.collectAll(ls -> ls.getTextDocumentService().typeDefinition(LSPEclipseUtils.toTypeDefinitionParams(params)).thenApply(l -> Pair.of(Messages.typeDefinitionHyperlinkLabel, l)));
var implementations = LanguageServers.forDocument(document).withCapability(ServerCapabilities::getImplementationProvider)
.collectAll(ls -> ls.getTextDocumentService().implementation(LSPEclipseUtils.toImplementationParams(params)).thenApply(l -> Pair.of(Messages.implementationHyperlinkLabel, l)));
LanguageServers.addAll(LanguageServers.addAll(LanguageServers.addAll(definitions, declarations), typeDefinitions), implementations)
LanguageServers.addAllSuccessful(definitions, declarations, typeDefinitions, implementations)
.get(800, TimeUnit.MILLISECONDS)
.stream().flatMap(locations -> toHyperlinks(document, region, locations.first(), locations.second()).stream())
.forEach(link -> allLinks.putIfAbsent(link.getLocation(), link));
Expand Down