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.

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

Handle errors coming from LS responses for completion items #1039

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

BoykoAlex
Copy link
Contributor

Fixes #1038

@@ -137,6 +137,9 @@ public ICompletionProposal[] computeCompletionProposals(ITextViewer viewer, int
if (isIncomplete) {
anyIncomplete.set(true);
}
}).exceptionally(t -> {
LanguageServerPlugin.logError("'%s' LS failed to compute completion items.".formatted(w.serverDefinition.label), t); //$NON-NLS-1$
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 not need an if such as
if (!CancellationUtil.isRequestCancelledException(e)) { // do not report error if the server has cancelled the request here as well?

Copy link
Contributor Author

@BoykoAlex BoykoAlex Jul 19, 2024

Choose a reason for hiding this comment

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

Fair point :-) Corrected...

Copy link
Contributor

@rubenporras rubenporras left a comment

Choose a reason for hiding this comment

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

Thanks

@@ -137,6 +137,11 @@ public ICompletionProposal[] computeCompletionProposals(ITextViewer viewer, int
if (isIncomplete) {
anyIncomplete.set(true);
}
}).exceptionally(t -> {
if (!CancellationUtil.isRequestCancelledException(t)) {
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 now lines 152-154?

Copy link
Contributor Author

@BoykoAlex BoykoAlex Jul 19, 2024

Choose a reason for hiding this comment

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

No those lines are not needed any more indeed. Corrected as well

@BoykoAlex BoykoAlex self-assigned this Jul 19, 2024
@rubenporras rubenporras merged commit 75d300b into eclipse:main Jul 19, 2024
6 checks passed
@rubenporras
Copy link
Contributor

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do not cancel message handling if one of multiple language servers reports an error
2 participants