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

New API: Port code lenses #433

Merged

Conversation

ahmedneilhussain
Copy link
Contributor

Ports code lenses across to the new API. Hopefully a straightforward change.

import org.eclipse.swt.events.MouseEvent;

public class LSPCodeMining extends LineHeaderCodeMining {
private CodeLens codeLens;
private final LanguageServer languageServer;

// TODO: Store a SingleServerDocumentExecutor instead when that codestream is merged
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just create a small PR with the SingleServerDocumentExecutor, merge it, and the use it here?

I would be quick on the review/merge process, so no need to have here a TODO with a follow up ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #434

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know when you have merged then I will rebase this.

Boolean resolveProvider = capabilities.getCodeLensProvider().getResolveProvider();
return resolveProvider != null && resolveProvider;
})) {
if (Boolean.TRUE != this.languageServerWrapper.getServerCapabilities().getCodeLensProvider().getResolveProvider()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think

		Boolean resolveProvider = capabilities.getCodeLensProvider().getResolveProvider();
					
		if (resolveProvider != null && resolveProvider) {
			return CompletableFuture.completedFuture(null);
		}					

would read better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That inverts the logic! I have rewritten it along the lines you suggest, though ;-)

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.

Could we please create a PR for SingleServerDocumentExecutor first and then use it in this PR?

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.

@ahmedneilhussain , the discussion was not as easy as expected, sorry. If you want you can rebase and fix the comments, then we can merge this one.

@ahmedneilhussain
Copy link
Contributor Author

@ahmedneilhussain , the discussion was not as easy as expected, sorry. If you want you can rebase and fix the comments, then we can merge this one.

Done I hope

.thenApply(codeLenses -> LanguageServers.streamSafely(codeLenses)
.map(codeLens -> toCodeMining(document, w, codeLens))
.filter(Objects::nonNull)))
.thenApply(result -> result.stream().flatMap(s -> s).collect(Collectors.toList()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could just use toList()

@rubenporras rubenporras self-requested a review February 10, 2023 13:39
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.

Looks good to me, let me know if you want to use toList() as suggested or if I shall merge it as it is

@ahmedneilhussain
Copy link
Contributor Author

Looks good to me, let me know if you want to use toList() as suggested or if I shall merge it as it is

Would you mind taking it as is? I approve of the more succinct form but it's only been added in 'recent' Java. As we still have to support a java 8 customer I'm going to have to backport everything using later constructs when I pull all the new API stuff from public-lsp4e into Cocotec's private fork. It can't be helped, but it does feel a bit like Cruel and Unusual Punishment to be submitting contributions to an open source repository that I will then need to backport in order to take advantage ;-) ;-)

@rubenporras rubenporras merged commit b3738ed into eclipse:master Feb 10, 2023
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.

None yet

2 participants