Add Either.map(...) #546
Add Either.map(...) #546
Conversation
|
Can one of the admins verify this patch? |
|
cc @jcompagner |
|
Lgtm - could we get a test for the new code? There are some other tests for either here: |
mickaelistria
force-pushed
the
Either.map
branch
from
65f4693
to
8e0b06c
Compare
Done. |
| @@ -63,7 +64,17 @@ public boolean isLeft() { | |||
| public boolean isRight() { | |||
| return right != null; | |||
| } | |||
|
|
|||
|
|
|||
| public <T> T map(Function<L, T> mapLeft, Function <R, T> mapRight) { | |||
There was a problem hiding this comment.
The signature should be map(Function<? super L, ? extends T> mapLeft, Function<? super R, ? extends T> mapRight) I suppose. Also the functions should be mandatory and null mappers should be prohibited to avoid surprises.
There was a problem hiding this comment.
Thanks for the suggestion, I applied it.
|
Just a note that there is also |
mickaelistria
force-pushed
the
Either.map
branch
from
8e0b06c
to
4a92b08
Compare
|
Anything else I should do to complete this PR? |
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.) |
|
@eclipse-lsp4j-bot ok to test |
|
@mickaelistria I've also tried to run the build for this PR locally (using |
|
is that not a matter of changing this import to just import static org.junit.Assert..assertEquals; |
mickaelistria
force-pushed
the
Either.map
branch
from
4a92b08
to
0924a39
Compare
|
@eclipse-lsp4j-bot run tests |
|
@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 |
| @@ -107,6 +110,19 @@ public void testEqualsFalseWithNonNull() { | |||
| assertFalse(either1.equals(either2)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testMap() { | |||
| Collection<Either<Collection<?>, String>> toTest = List.of( | |||
There was a problem hiding this comment.
LSP4J is Java 8 - No List.of available:
07:31:37 > Task :org.eclipse.lsp4j.jsonrpc:compileTestJava FAILED
07:31:37 /home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/json/EitherTest.java:116: error: cannot find symbol
07:31:37 Either.forLeft(List.of("f", "o", "o")),
07:31:37 ^
07:31:37 symbol: method of(String,String,String)
07:31:37 location: interface List
07:31:37 1 error
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! |
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.
mickaelistria
force-pushed
the
Either.map
branch
from
0924a39
to
675a26b
Compare
|
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). |
I just created #547 - we should at least know why we are still on Java 8. |
This is a follow-up to eclipse-lsp4j#546
This is a follow-up to eclipse-lsp4j#546
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
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
insert.getCharacter()) start = insert;
}), document);
which better captures the logic.