From 634cac8eb47aa643e5fae862e3798b1690feed42 Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Wed, 16 Nov 2022 14:38:25 -0500 Subject: [PATCH 1/3] Handle completion edge cases with empty mapping nodes --- actions-languageservice/src/complete.test.ts | 24 +++++++++ actions-languageservice/src/complete.ts | 10 +--- .../src/utils/find-token.ts | 49 +++++++++++++++---- 3 files changed, 65 insertions(+), 18 deletions(-) diff --git a/actions-languageservice/src/complete.test.ts b/actions-languageservice/src/complete.test.ts index c52e5f7..08d73e7 100644 --- a/actions-languageservice/src/complete.test.ts +++ b/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"]); + }); }); diff --git a/actions-languageservice/src/complete.ts b/actions-languageservice/src/complete.ts index 7286edc..b6011e0 100644 --- a/actions-languageservice/src/complete.ts +++ b/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 { @@ -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; diff --git a/actions-languageservice/src/utils/find-token.ts b/actions-languageservice/src/utils/find-token.ts index cdb12b7..3ca06cc 100644 --- a/actions-languageservice/src/utils/find-token.ts +++ b/actions-languageservice/src/utils/find-token.ts @@ -1,4 +1,4 @@ -import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/index"; +import {StringToken, 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"; import {TokenType} from "@github/actions-workflow-parser/templates/tokens/types"; @@ -54,8 +54,26 @@ 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)) { + const sameLine = onSameLine(pos, key, value); + if (posInToken(pos, key)) { + if (sameLine && 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 + }; + } + + return { + token: key, + keyToken: null, + parent: mappingToken + }; + } + + // Empty nodes positions won't always match the cursor, so check if we're on the same line + if (sameLine && emptyNode(value)) { return { token: value, keyToken: null, @@ -122,11 +140,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 +150,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 +162,20 @@ 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 (token.templateTokenType === TokenType.String) { + const stringToken = token as StringToken; + return stringToken.value === ""; + } + + return false; +} From b4f6e674b77fea934a2d5b08614aad48cdd57c78 Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Wed, 16 Nov 2022 14:58:36 -0500 Subject: [PATCH 2/3] Simplify same line logic in find token --- .../src/utils/find-token.ts | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/actions-languageservice/src/utils/find-token.ts b/actions-languageservice/src/utils/find-token.ts index 3ca06cc..4a7c8bf 100644 --- a/actions-languageservice/src/utils/find-token.ts +++ b/actions-languageservice/src/utils/find-token.ts @@ -54,9 +54,8 @@ export function findToken(pos: Position, root?: TemplateToken): TokenResult { for (let i = 0; i < mappingToken.count; i++) { const {key, value} = mappingToken.get(i); - const sameLine = onSameLine(pos, key, value); - if (posInToken(pos, key)) { - if (sameLine && key.range!.end[1] + 1 === value.range!.start[1]) { + 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, @@ -65,20 +64,14 @@ export function findToken(pos: Position, root?: TemplateToken): TokenResult { }; } - return { - token: key, - keyToken: null, - parent: mappingToken - }; - } - - // Empty nodes positions won't always match the cursor, so check if we're on the same line - if (sameLine && emptyNode(value)) { - return { - token: value, - keyToken: null, - parent: key - }; + // 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({ From 285ed7c3db2c42a6ecaa9ef4dbf6f1d01e7ad63b Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Thu, 17 Nov 2022 11:25:40 -0800 Subject: [PATCH 3/3] Use type-guard --- actions-languageservice/src/utils/find-token.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actions-languageservice/src/utils/find-token.ts b/actions-languageservice/src/utils/find-token.ts index 4a7c8bf..88e43ee 100644 --- a/actions-languageservice/src/utils/find-token.ts +++ b/actions-languageservice/src/utils/find-token.ts @@ -1,4 +1,5 @@ -import {StringToken, TemplateToken} from "@github/actions-workflow-parser/templates/tokens/index"; +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"; import {TokenType} from "@github/actions-workflow-parser/templates/tokens/types"; @@ -165,9 +166,8 @@ function emptyNode(token: TemplateToken | null): boolean { return true; } - if (token.templateTokenType === TokenType.String) { - const stringToken = token as StringToken; - return stringToken.value === ""; + if (isString(token)) { + return token.value === ""; } return false;