Adds backticks to dbNames/aliases if needed #310

anderson4j · 2024-12-09T14:30:53Z

Before autocompletions for database names/aliases would be like:
Ex. database name = "matrix-db"
SHOW DATABASE ^ -> SHOW DATABASE matrix-db which can not be parsed

now we get
-> SHOW DATABASE `matrix-db`

Note: we might allow parameters to have escaped names too (my-param) in which case we should update their usages for db-name-suggestions too

Further possibility for stuff that might need backticks - constraints/indexes once we suggest for these.

changeset-bot · 2024-12-09T14:30:56Z

🦋 Changeset detected

Latest commit: a011d66

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@neo4j-cypher/language-support	Patch
@neo4j-cypher/language-server	Patch
@neo4j-cypher/react-codemirror-playground	Patch
@neo4j-cypher/react-codemirror	Patch
@neo4j-cypher/schema-poller	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/language-support/src/autocompletion/completionCoreCompletions.ts

ncordon · 2024-12-10T15:22:10Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    return parameterSuggestions.map((completionItem) => ({
+      label: completionItem.label,
+      kind: completionItem.kind,
+    }));


Why do we need these mappings to label and kind? This is not doing anything I think?

Used to backtick if needed parameters too (so had insertText: backtickIfNeeded(...label) too), but they could not be added with backticks yet. Looks like I forgot to remove the whole thing.

Either way, since we're changing the parser-file for creating params, maybe we should just add backticks if needed here already, so once it's valid, we're good to go?

ncordon · 2024-12-10T15:22:17Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    return result.map((completionItem) => ({
+      label: completionItem.label,
+      kind: completionItem.kind,
+    }));


ncordon · 2024-12-10T15:22:22Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    ].map((completionItem) => ({
+      label: completionItem.label,
+      kind: completionItem.kind,
+    }));


ncordon · 2024-12-10T15:23:34Z

packages/language-support/src/tests/autocompletion/databasenameCompletion.test.ts

+  test('Correctly completes database names/aliases with special symbols using backticks', () => {
+    const query = 'SHOW DATABASE ';


This test is unnecessary, you could just add the completions to the existing test?

I split it on purpose so you would see that only this test failed if something purely related to backticks broke. You don't think there's any value to this?

ncordon · 2024-12-10T15:24:59Z

packages/language-support/src/tests/autocompletion/databasenameCompletion.test.ts

+      ],
+    });
+  });
+
  test('Correctly completes database names and aliases in SHOW DATABASE', () => {


There's also a SHOW ALIAS test in this file which you should add the corresponding checks to

ncordon · 2024-12-12T09:47:13Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    return parameterSuggestions.map((completionItem) => ({
+      label: completionItem.label,
+      kind: completionItem.kind,
+      insertText: backtickDbNameIfNeeded(completionItem.label),


Can a parameter include the ., otherwise this should be backtickIfNeeded, no? Same for some of the uses below

Ah yeah, this is not valid

anderson4j · 2024-12-13T12:52:02Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    return undefined;
+  }
+  const namePart = e.slice(1);
+  if (e[0] == '$') {


Should I even have this test, or just trust the backtickParam... -method to only be called with params?

I think it's okay to have double guards in case something goes wrong in the other part of the code.

… of parameters (for now)

…olicName

anderson4j · 2024-12-16T08:44:09Z

I think what is new from your last review is just the PR shifting to $param-Name backticks over $param-Name. The old comments are handled

ncordon · 2024-12-18T10:16:36Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    if (/[^\p{L}\p{N}_]/u.test(namePart) || /[^\p{L}_]/u.test(namePart[0])) {
+      return '$' + `\`${namePart}\``;
+    } else {
+      return undefined;
+    }
+  } else {
+    return backtickIfNeeded(e);
+  }
+}


You could refactor all this method to depend on the backtickIfNeeded as:

function backtickParamIfNeeded(e: string): string | undefined { if (e == null || e == '') { return undefined; } const namePart = e.slice(1); const backticked = backtickIfNeeded(namePart); if (backticked) { return '$' + backticked; } else { return undefined; } }

Looks like fixing comment 2 will actually mean I can get ahead of adding the $ and just use backtickIfNeeded from scratch

ncordon · 2024-12-18T10:23:27Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+    return parameterSuggestions.map((completionItem) => ({
+      label: completionItem.label,
+      kind: completionItem.kind,
+      insertText: backtickParamIfNeeded(completionItem.label),
+    }));


We should move the insertText thing to where the parameterSuggestions are defined instead of repeating this in the code a few times

ncordon · 2024-12-18T15:40:32Z

packages/language-support/src/autocompletion/completionCoreCompletions.ts

+      return {
+        label: `$${paramName}`,
+        kind: CompletionItemKind.Variable,
+        ...(backtickedName != undefined


This comparison needs to be !== I think.

You can just do this instead

backtickedName ? { insertText: `$${backtickIfNeeded(paramName)}` } : {}

ncordon

Address my last comment please and ready to go 😄

anderson4j added the auto completion Issues related to the auto-completion label Dec 9, 2024

anderson4j requested a review from ncordon December 9, 2024 14:30

anderson4j commented Dec 9, 2024

View reviewed changes

packages/language-support/src/autocompletion/completionCoreCompletions.ts Show resolved Hide resolved

anderson4j force-pushed the backtickExtension branch from 32b7c93 to 4a9391c Compare December 9, 2024 15:15

ncordon reviewed Dec 10, 2024

View reviewed changes

ncordon reviewed Dec 12, 2024

View reviewed changes

anderson4j commented Dec 13, 2024

View reviewed changes

anderson4j added 8 commits December 13, 2024 14:01

added backtick-support to aliases, database-names and parameters

e4fcbf0

expanded dbname/alias testing for backticks

130d2cf

split dbname and label/reltype/propkey backticks. Removed backticking…

eec44da

… of parameters (for now)

removed commented lines

b9ecfb2

re-added backticking of parameters, removed leftovers in completeSymb…

fdc7251

…olicName

added SHOW ALIAS test for backtick-testing

4fcc829

modified params suggested with backticks to be like $param name

ee1580f

re-added param from too agressive undo

050a918

anderson4j force-pushed the backtickExtension branch from bec5e8c to 050a918 Compare December 13, 2024 13:02

anderson4j requested a review from ncordon December 16, 2024 08:44

Merge remote-tracking branch 'origin/main' into backtickExtension

0df1e8e

ncordon reviewed Dec 18, 2024

View reviewed changes

refactored where backticking for parameters is done

6ea0517

ncordon reviewed Dec 18, 2024

View reviewed changes

ncordon approved these changes Dec 18, 2024

View reviewed changes

final small refactor

0e8d19a

added changeset

a011d66

anderson4j merged commit b0e419e into main Dec 19, 2024
4 checks passed

anderson4j deleted the backtickExtension branch December 19, 2024 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds backticks to dbNames/aliases if needed #310

Adds backticks to dbNames/aliases if needed #310

anderson4j commented Dec 9, 2024 •

edited by ncordon

Loading

changeset-bot bot commented Dec 9, 2024 •

edited

Loading

ncordon Dec 10, 2024

anderson4j Dec 11, 2024

ncordon Dec 10, 2024

ncordon Dec 10, 2024

ncordon Dec 10, 2024

anderson4j Dec 11, 2024

ncordon Dec 10, 2024

ncordon Dec 12, 2024 •

edited

Loading

anderson4j Dec 13, 2024

anderson4j Dec 13, 2024

ncordon Dec 18, 2024

anderson4j commented Dec 16, 2024 •

edited

Loading

ncordon Dec 18, 2024

anderson4j Dec 18, 2024

ncordon Dec 18, 2024

ncordon Dec 18, 2024 •

edited

Loading

ncordon left a comment

		test('Correctly completes database names/aliases with special symbols using backticks', () => {
		const query = 'SHOW DATABASE ';

Adds backticks to dbNames/aliases if needed #310

Adds backticks to dbNames/aliases if needed #310

Conversation

anderson4j commented Dec 9, 2024 • edited by ncordon Loading

changeset-bot bot commented Dec 9, 2024 • edited Loading

🦋 Changeset detected

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

ncordon Dec 12, 2024 • edited Loading

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

anderson4j commented Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncordon Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

ncordon left a comment

Choose a reason for hiding this comment

anderson4j commented Dec 9, 2024 •

edited by ncordon

Loading

changeset-bot bot commented Dec 9, 2024 •

edited

Loading

ncordon Dec 12, 2024 •

edited

Loading

anderson4j commented Dec 16, 2024 •

edited

Loading

ncordon Dec 18, 2024 •

edited

Loading