Merge pull request #8 from github/joshmgross/empty-mapping-nodes

Handle completion edge cases with empty mapping nodes

actions-languageservice/src/complete.test.ts

@@ -145,4 +145,28 @@ jobs:
     expect(result.length).toEqual(1);
     expect(result[0].label).toEqual("my-custom-label");
   });
+
+  it("does not show parent mapping sibling keys", async () => {
+    const input = `on: push
+jobs:
+  build:
+    container: |
+    runs-on: ubuntu-latest`;
+    const result = await complete(...getPositionFromCursor(input));
+    expect(result).not.toBeUndefined();
+    expect(result.length).toEqual(6);
+    // Should not contain other top-level job keys like `if` and `runs-on`
+    expect(result.map(x => x.label)).not.toContain("if");
+    expect(result.map(x => x.label)).not.toContain("runs-on");
+  });
+
+  it("shows mapping keys within a new map ", async () => {
+    const input = `on: push
+jobs:
+  build:
+    concurrency: |`;
+    const result = await complete(...getPositionFromCursor(input));
+    expect(result).not.toBeUndefined();
+    expect(result.map(x => x.label).sort()).toEqual(["cancel-in-progress", "group"]);
+  });
 });

actions-languageservice/src/complete.ts

@@ -71,14 +71,13 @@ export async function complete(
     }
   }
 
-  const values = await getValues(token, parent, newPos, textDocument.uri, valueProviderConfig);
+  const values = await getValues(token, parent, textDocument.uri, valueProviderConfig);
   return values.map(value => CompletionItem.create(value.label));
 }
 
 async function getValues(
   token: TemplateToken | null,
   parent: TemplateToken | null,
-  position: Position,
   workflowUri: string,
   valueProviderConfig: ValueProviderConfig | undefined
 ): Promise<Value[]> {
@@ -86,13 +85,6 @@ async function getValues(
     return [];
   }
 
-  if (token?.templateTokenType === TokenType.Null) {
-    // Ensure there's a space after the parent key
-    if (parent.range && position.character + 1 === parent.range.end[1]) {
-      return [];
-    }
-  }
-
   const existingValues = getExistingValues(token, parent);
 
   let customValues: Value[] | undefined = undefined;

actions-languageservice/src/utils/find-token.ts

@@ -1,3 +1,4 @@
+import {isString} from "@github/actions-workflow-parser";
 import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/index";
 import {MappingToken} from "@github/actions-workflow-parser/templates/tokens/mapping-token";
 import {SequenceToken} from "@github/actions-workflow-parser/templates/tokens/sequence-token";
@@ -54,13 +55,24 @@ export function findToken(pos: Position, root?: TemplateToken): TokenResult {
         for (let i = 0; i < mappingToken.count; i++) {
           const {key, value} = mappingToken.get(i);
 
-          // Null tokens don't have a position, we can only use the line information
-          if (nullNodeOnLine(pos, key, value)) {
-            return {
-              token: value,
-              keyToken: null,
-              parent: key
-            };
+          if (onSameLine(pos, key, value)) {
+            if (posInToken(pos, key) && key.range!.end[1] + 1 === value.range!.start[1]) {
+              // There's no space between the key and value, there's nothing valid to complete here
+              return {
+                token: null,
+                keyToken: null,
+                parent: null
+              };
+            }
+
+            // Empty nodes positions won't always match the cursor, so check if we're on the same line
+            if (emptyNode(value)) {
+              return {
+                token: value,
+                keyToken: null,
+                parent: key
+              };
+            }
           }
 
           s.push({
@@ -122,11 +134,7 @@ function posInToken(pos: Position, token: TemplateToken): boolean {
   return true;
 }
 
-function nullNodeOnLine(pos: Position, key: TemplateToken, value: TemplateToken): boolean {
-  if (value.templateTokenType !== TokenType.Null) {
-    return false;
-  }
-
+function onSameLine(pos: Position, key: TemplateToken, value: TemplateToken): boolean {
   if (!value.range) {
     return false;
   }
@@ -136,7 +144,7 @@ function nullNodeOnLine(pos: Position, key: TemplateToken, value: TemplateToken)
   }
 
   if (value.range.start[0] !== value.range.end[0]) {
-    // Token occupies multiple lines, can't be a null node
+    // Token occupies multiple lines, can't be an empty node
     return false;
   }
 
@@ -148,3 +156,19 @@ function nullNodeOnLine(pos: Position, key: TemplateToken, value: TemplateToken)
 
   return true;
 }
+
+function emptyNode(token: TemplateToken | null): boolean {
+  if (!token) {
+    return false;
+  }
+
+  if (token.templateTokenType === TokenType.Null) {
+    return true;
+  }
+
+  if (isString(token)) {
+    return token.value === "";
+  }
+
+  return false;
+}