[FUSETOOLS2-1673] Choice EIP implementation #861

joshiraez · 2022-11-29T10:16:32Z

Implementation of the Content Based Router on the Camel language server.

Draft to set a working build although I'll be iterating on it to add/refactor stuff for better extendability.

apupier · 2022-11-29T12:42:27Z

src/main/java/com/github/cameltooling/lsp/internal/parser/ParserFileHelperUtil.java

@@ -36,5 +36,27 @@ public String getLine(String text, int line) {
 		}
 		return null;
 	}
+
+	public String getTextUntilPosition(TextDocumentItem document, Position position) {
+		String newLine = System.getProperty("line.separator");


usually Client implementation of the LSP has their own policy for end of lines. Users are defining the type of end of line in either, thier IDE settings, thei rproject config, their git config. There is no need to take of care of the line separator from the system.

apupier · 2022-11-29T12:45:03Z

src/test/java/com/github/cameltooling/lsp/internal/util/RouteTextBuilder.java

+	private static final String JAVA_CLASS_BLUEPRINT_CAMEL_ROUTEBUILDER_IMPORT
+			= "import org.apache.camel.builder.RouteBuilder;";
+
+	private static final String JAVA_CLASS_BLUEPRINT_CLASS_DECLARATION = "public class TestRoute";
+
+	private static final String JAVA_CLASS_BLUEPRINT_CAMEL_ROUTEBUILDER_EXTEMD = "extends RouteBuilder";
+
+	private static final String JAVA_CLASS_BLUEPRINT_CONFIGURE_METHOD_DECLARATION = "public void configure()";


I think the terminology Bluerpint should not be used here.

it will collides with the "Camel XML blueprint namespace" which is related to the Apache Aries Blueprint implementation

I see. Blueprint can be associated with this kind of stye but I didn't know it had domain meaning. I'll change it.

joshiraez · 2023-01-13T11:53:39Z

src/main/java/com/github/cameltooling/lsp/internal/CamelTextDocumentService.java

+									.map(future -> future.join())
+									.flatMap(x -> x.stream())
+									.collect(Collectors.toList())
+					).thenApply(Either::forLeft);


This is needed because we want to be able to add different completions that have different triggers

joshiraez · 2023-01-13T11:54:26Z

src/main/java/com/github/cameltooling/lsp/internal/parser/CamelEIPParser.java

+	}
+
+	public boolean canPutEIP(Position position) {
+		Pattern CHOICE_EIP_PATTERN = Pattern.compile("\\)\\s*[.[c[h[o[i[c[e]?]?]?]?]?]?]?\\(?$", Pattern.MULTILINE);


This is ugly :D, but I didn't feel like doing a Trie based implementation was justified with just one EIP, which was my original goal

I'll be refactoring this when I add more EIPs

The CHOICE_EIP_PATTERN can be put as a static attribute. (if not, better to rename to follow convention of naming witout capital letter)

apupier

Can you precise what is working exactly and what is left for another iteration?

From my observations:

Content Based Router template is provided when there is:
- a closing parenthesis before:
- closing parenthesis and a dot
- a closing parenthesis and a dot and a c character:
It doesn't appear when there are more letters. like .ch. That's fine, can be done later but in this case we can simplify the regex. But there is a test which seems to tell that it shoudl be working com.github.cameltooling.lsp.internal.completion.eip.EIPChoiceCompletionTest.testProvideInsertionMidWritingChoice(), in this case I suspect that the fact that the Range is missing is causing VS Code to filter out the completion.:
The insertion when there is .c is adding an extra dot:
Content Based Router template is provided almost at the end of the completion list. I think this can be improved in another iteration by giving Range and priority
For another iteration: After calling the completion, setting the cursor for next edition inside the parenthesis of the first when would be great. having all places to edit would be even better

apupier · 2023-01-16T08:54:47Z

src/main/java/com/github/cameltooling/lsp/internal/parser/CamelEIPParser.java

+	}
+
+	public boolean canPutEIP(Position position) {
+		Pattern CHOICE_EIP_PATTERN = Pattern.compile("\\)\\s*[.[c[h[o[i[c[e]?]?]?]?]?]?]?\\(?$", Pattern.MULTILINE);


The CHOICE_EIP_PATTERN can be put as a static attribute. (if not, better to rename to follow convention of naming witout capital letter)

apupier · 2023-01-16T09:03:31Z

src/main/java/com/github/cameltooling/lsp/internal/parser/CamelEIPParser.java

+		String newLine = System.getProperty("line.separator");
+		CompletionItem completion = new CompletionItem("Content Based Router");
+		completion.setDocumentation(
+				"Read more: https://camel.apache.org/components/3.11.x/eips/content-based-router-eip.html"


too old link, already a 404.

Better to link to the latest LTS, maybe https://camel.apache.org/components/3.20.x/eips/choice-eip.html

apupier · 2023-01-16T09:04:08Z

src/main/java/com/github/cameltooling/lsp/internal/parser/CamelEIPParser.java

+	}
+
+	private CompletionItem choiceEIPcompletion() {
+		String newLine = System.getProperty("line.separator");


Here we can use \n and delegate to editor which end of line is configured.
it is even better in case of remote Camel Languaeg Server (we are not providing some right ow but ihe architecture allows it I provided proof of concept). It would mean that the client can be on a different OS than the server

apupier · 2023-01-16T09:06:34Z

src/main/java/com/github/cameltooling/lsp/internal/parser/CamelEIPParser.java

+	}
+
+	public boolean canPutEIP(Position position) {
+		Pattern CHOICE_EIP_PATTERN = Pattern.compile("\\)\\s*[.[c[h[o[i[c[e]?]?]?]?]?]?]?\\(?$", Pattern.MULTILINE);


Sonar is complaining of duplicates in the regexp. I guess there should be a simpler way to write it.

I'm leaving it out of the iteration for now as RegExr is not complaining nor I can find what would be the alternative right now. Mmm

apupier · 2023-01-16T09:10:51Z

src/main/java/com/github/cameltooling/lsp/internal/parser/ParserFileHelperUtil.java

+		String textUntilPosition = "";
+
+		for(int i = 0; i<position.getLine(); i++) {
+			textUntilPosition += lines[i] + newLine;
+		}


Sonar is recommending to use a StringBuilder

apupier · 2023-01-16T09:44:36Z

src/test/java/com/github/cameltooling/lsp/internal/completion/eip/EIPChoiceCompletionTest.java

+				completionItem -> completionItem.getLabel().startsWith("Content Based Router")
+		).collect(Collectors.toList());
+
+		assertThat(completionItems).hasSize(0); //Removing camel-k modeline insertion. Refactor later


AssertJ is providing an isEmpty() method. Itwill provide a better error message, simplifying the debugging process.

apupier · 2023-01-16T09:44:43Z

src/test/java/com/github/cameltooling/lsp/internal/completion/eip/EIPChoiceCompletionTest.java

+		).collect(Collectors.toList());
+
+
+		assertThat(completionItems).hasSize(0);


AssertJ is providing an isEmpty() method. Itwill provide a better error message, simplifying the debugging process.

apupier · 2023-01-16T09:44:50Z

src/test/java/com/github/cameltooling/lsp/internal/completion/eip/EIPChoiceCompletionTest.java

+				completionItem -> completionItem.getLabel().startsWith("Content Based Router")
+		).collect(Collectors.toList());
+
+		assertThat(completionItems).hasSize(0);


AssertJ is providing an isEmpty() method. It will provide a better error message, simplifying the debugging process.

apupier · 2023-01-16T09:46:53Z

src/test/java/com/github/cameltooling/lsp/internal/completion/eip/EIPChoiceCompletionTest.java

+		List<CompletionItem> completionItems = getCompletionsFor(contents, position);
+		completionItems = completionItems.stream().filter(
+				completionItem -> completionItem.getLabel().startsWith("Content Based Router")
+		).collect(Collectors.toList());


This code can be factorized.
it will be really useful in case the label of the completion is modified for instance

apupier · 2023-01-16T09:47:47Z

src/test/java/com/github/cameltooling/lsp/internal/completion/eip/EIPChoiceCompletionTest.java

+		RouteTextBuilder.DocumentContentWithPosition document =
+				RouteTextBuilder.createJavaDocumentClass("");
+
+		List<CompletionItem> completionItems = getCompletionsFor(document.content, document.position);


In Java, we usually avoid to expose attributes and use getters and setters

joshiraez · 2023-01-19T10:09:55Z

Just saw this. I'm on with the improvements.

joshiraez · 2023-01-19T10:15:54Z

Ok checking the range and priority things on top of all the comments.

It should work for anything between ..){{{.choice(}}}... as the test stated. I have an idea why it might be failing on .ch+

I'll get first with the comments then on the top comment priorities. Expect an update today

… it works fine...? Leaving it as is because I would have to use capturing group which have a higher load to fix it

…ere written to just have "\n" independently on the system so they broke. It didn't make sense to leave the carriage return as it would be treated as another whitespace character so I decided to simplify it

joshiraez added the DRAFT label Nov 29, 2022

joshiraez force-pushed the lsp_b_2 branch from dc78ffc to 0dfd858 Compare November 29, 2022 10:20

joshiraez changed the title ~~WIP: [FUSETOOLS2-1673] Choice EIP short implementation~~ WIP: [FUSETOOLS2-1673] Choice EIP implementation Nov 29, 2022

apupier reviewed Nov 29, 2022

View reviewed changes

joshiraez force-pushed the lsp_b_2 branch from 0dfd858 to fcb54ef Compare January 9, 2023 13:21

joshiraez requested a review from apupier January 10, 2023 00:47

joshiraez removed the DRAFT label Jan 10, 2023

joshiraez commented Jan 13, 2023

View reviewed changes

joshiraez changed the title ~~WIP: [FUSETOOLS2-1673] Choice EIP implementation~~ [FUSETOOLS2-1673] Choice EIP implementation Jan 13, 2023

apupier requested changes Jan 16, 2023

View reviewed changes

joshiraez and others added 11 commits January 20, 2023 16:11

Choice EIP short implementation

00720d6

Saving changes

d8f0ec9

Saving changes

6d89bcc

Fixed bug on choice EIP checking

943bf57

Added another test for what seemed like a bug according to regexr but…

98a3e97

… it works fine...? Leaving it as is because I would have to use capturing group which have a higher load to fix it

Refactoring RouteTextBuilder

c077274

Fixed tests on Windows, removing System.lineSeparator as some tests w…

2072ef7

…ere written to just have "\n" independently on the system so they broke. It didn't make sense to leave the carriage return as it would be treated as another whitespace character so I decided to simplify it

Refactored blueprint terminology to document

db24826

Line separator os specific in document builder

e512c5e

Forgot that this was what proced the bug :))))

1c72b3d

Refactoring with Aurelien suggestions

45127a2

joshiraez force-pushed the lsp_b_2 branch from 56bc92a to 45127a2 Compare January 20, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FUSETOOLS2-1673] Choice EIP implementation #861

[FUSETOOLS2-1673] Choice EIP implementation #861

joshiraez commented Nov 29, 2022

apupier Nov 29, 2022

apupier Nov 29, 2022

joshiraez Nov 29, 2022

joshiraez Jan 13, 2023

joshiraez Jan 13, 2023

apupier Jan 16, 2023

apupier left a comment

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

joshiraez Jan 20, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

apupier Jan 16, 2023

joshiraez commented Jan 19, 2023

joshiraez commented Jan 19, 2023

		).collect(Collectors.toList());


		assertThat(completionItems).hasSize(0);

[FUSETOOLS2-1673] Choice EIP implementation #861

Are you sure you want to change the base?

[FUSETOOLS2-1673] Choice EIP implementation #861

Conversation

joshiraez commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apupier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshiraez commented Jan 19, 2023

joshiraez commented Jan 19, 2023