Add Either.map(...)

[mickaelistria]

Many case of usage of Either in LSP4E involve turning Either into a value. That requires some boilerplate: declare variable, if/else.

This constructs can allow for instance replacing

Either<TextEdit, InsertReplaceEdit> textEdit = item.getTextEdit();
if (textEdit.isLeft())
    return offset ==

LSPEclipseUtils.toOffset(textEdit.getLeft().getRange().getStart(), document); else { Position replace = textEdit.getRight().getReplace().getStart(); Position insert = textEdit.getRight().getInsert().getStart(); Position start = replace; if (replace.getLine() > insert.getLine() || replace.getCharacter() > insert.getCharacter()) start = insert; return offset == LSPEclipseUtils.toOffset(start, document); }

by

return offset == LSPEclipseUtils.toOffset(
    textEdit.map(
        edit -> edit.getRange().getStart(),
        insertReplaceEdit -> {
            Position replace = insertReplaceEdit.getReplace().getStart();
            Position insert = insertReplaceEdit.getInsert().getStart();
            Position start = replace;
            if (replace.getLine() > insert.getLine() || replace.getCharacter() >

insert.getCharacter()) start = insert; }), document);

which better captures the logic.

[eclipse-lsp4j-bot]

Can one of the admins verify this patch?

[mickaelistria]

cc @jcompagner

[jonahgraham]

Lgtm - could we get a test for the new code? There are some other tests for either here:

https://github.com/eclipse/lsp4j/blob/1470d748e556476369dfaf6a77cb32efddb764be/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/json/EitherTest.java#L79-L90

[mickaelistria]

could we get a test for the new code?

Done.

[pisv]

Just a note that there is also Either3, where a three-argument overload for map would probably make sense.

[mickaelistria]

Anything else I should do to complete this PR?

[szarnekow]

Anything else I should do to complete this PR?

What do you think about @pisv remark?

Just a note that there is also Either3, where a three-argument overload for map would probably make sense.

[mickaelistria]

What do you think about @pisv remark?

I think it's right, but I don't plan to work on this case right now as I don't see usage of Either3 in LSP4E for the moment. So that would be another patch later maybe.

[pisv]

What do you think about @pisv remark?

I think it's right, but I don't plan to work on this case right now as I don't see usage of Either3 in LSP4E for the moment. So that would be another patch later maybe.

No worries. For the sake of API consistency, I'll take care of it after this PR is merged. Thank you for the contribution. (+1 from my side.)

[pisv]

@eclipse-lsp4j-bot ok to test

[pisv]

@mickaelistria I've also tried to run the build for this PR locally (using ./gradlew build) and it fails with EitherTest.java:16: error: package org.junit.jupiter.api does not exist. Can you please take a look at it?

[jcompagner]

is that not a matter of changing this import import static org.junit.jupiter.api.Assertions.assertEquals;

to just

import static org.junit.Assert..assertEquals;

[pisv]

@eclipse-lsp4j-bot run tests

[pisv]

@jonahgraham The local build completes successfully now. Not sure what happens with our Jenkins PR Build. Strangely, the test result does not even include tests in the org.eclipse.lsp4j.jsonrpc.test.json package. Can you please help?

[jonahgraham]

@jonahgraham The local build completes successfully now. Not sure what happens with our Jenkins PR Build. Strangely, the test result does not even include tests in the org.eclipse.lsp4j.jsonrpc.test.json package. Can you please help?

I suspect you are using Java 11 locally? The build machine uses Java 8 as there is a Java 8 requirement. I don't know whether there is an actual requirement to stay on Java 8 or not.

[pisv]

I suspect you are using Java 11 locally? The build machine uses Java 8 as there is a Java 8 requirement. I don't know whether there is an actual requirement to stay on Java 8 or not.

Yes. Got it. Thanks for the clarification and your help!

[mickaelistria]

I tried to Java8-ify this test, but I would really appreciate if LSP4J can move to Java 11 in a near future. Java 8 is really a burden that doesn't make sense to keep bothering with (unless LSP4J has some major contributor really needing this and contributing enough to balance the extra effort).

[jonahgraham]

Java 8 is really a burden that doesn't make sense to keep bothering with (unless LSP4J has some major contributor really needing this and contributing enough to balance the extra effort).

I just created #547 - we should at least know why we are still on Java 8.