diff --git a/PR_REVIEW.md b/PR_REVIEW.md deleted file mode 100644 index c7b32b0..0000000 --- a/PR_REVIEW.md +++ /dev/null @@ -1,59 +0,0 @@ -# PR #283 Review: Use property descriptions for completion items - -## Summary - -This PR fixes a bug where completion items for action.yml were missing descriptions. The root cause was that `mappingValues()` only looked at the type definition's description, ignoring property-level descriptions in the schema. - -## Changes Analysis - -### Core Fix ([definition.ts](languageservice/src/value-providers/definition.ts)) - -**Before:** -```typescript -let description: string | undefined; -if (value.type) { - const typeDef = definitions[value.type]; - description = typeDef?.description; -} -``` - -**After:** -```typescript -let description: string | undefined = value.description; -if (value.type) { - const typeDef = definitions[value.type]; - if (!description) { - description = typeDef?.description; - } -} -``` - -✅ **Correct approach** - prioritizes property description, falls back to type description. - -### Test Coverage - -1. **complete-action.test.ts**: Two new tests verify `author` and `branding` completions include documentation -2. **hover-action.test.ts**: New test for `author` hover + updated `branding` test to verify "Documentation" link - -## Potential Issues - -### 1. One-of expansion doesn't use property description - -Looking at line 140-142: -```typescript -const expanded = expandOneOfToCompletions(oneOfDef, definitions, key, description, indentation, mode); -``` - -This passes `description` to `expandOneOfToCompletions`, but at this point `description` may have been populated from the property. **This is correct** - the property description is passed through. - -### 2. Consistency check - -The PR description mentions this is consistent with hover. Verified: [template-reader.ts#L225](workflow-parser/src/templates/template-reader.ts#L225) shows hover uses `nextPropertyDef.description` when available. - -## Verdict - -✅ **LGTM** - Clean, minimal fix that aligns completion behavior with hover. Good test coverage for the specific cases mentioned. - -## Minor Suggestions (non-blocking) - -1. Could add a test for a property that has NO description but whose type DOES have one, to verify fallback works (e.g., `inputs` which references `inputs-strict` type that has a description) diff --git a/languageservice/src/validate.expressions-literal-text.test.ts b/languageservice/src/validate.expressions-literal-text.test.ts index e6f4084..ae3e97e 100644 --- a/languageservice/src/validate.expressions-literal-text.test.ts +++ b/languageservice/src/validate.expressions-literal-text.test.ts @@ -211,4 +211,104 @@ jobs: ); }); }); + + // https://github.com/github/vscode-github-actions/issues/542 + describe("YAML-quoted expressions", () => { + it("allows double-quoted expression in job-if", async () => { + // Quotes are needed when the expression contains a colon + const input = ` +on: push +jobs: + publish: + if: "\${{ startsWith(github.event.head_commit.message, 'chore: release') }}" + runs-on: ubuntu-latest + steps: + - run: echo hi +`; + const result = await validate(createDocument("wf.yaml", input)); + + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "expression-literal-text-in-condition" + }) + ); + }); + + it("allows single-quoted expression in job-if", async () => { + const input = ` +on: push +jobs: + publish: + if: '\${{ startsWith(github.event.head_commit.message, "chore: release") }}' + runs-on: ubuntu-latest + steps: + - run: echo hi +`; + const result = await validate(createDocument("wf.yaml", input)); + + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "expression-literal-text-in-condition" + }) + ); + }); + + it("allows double-quoted expression in step-if", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: "\${{ contains(github.event.head_commit.message, 'skip: ci') }}" + run: echo hi +`; + const result = await validate(createDocument("wf.yaml", input)); + + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "expression-literal-text-in-condition" + }) + ); + }); + + it("still errors when there is actual literal text outside expression", async () => { + // Even with quotes, if there's literal text outside ${{ }}, it should error + const input = ` +on: push +jobs: + build: + if: "push == \${{ github.event_name }}" + runs-on: ubuntu-latest + steps: + - run: echo hi +`; + const result = await validate(createDocument("wf.yaml", input)); + + expect(result).toContainEqual( + expect.objectContaining({ + code: "expression-literal-text-in-condition" + }) + ); + }); + + it("errors on multiple expressions with literal text between them", async () => { + const input = ` +on: push +jobs: + build: + if: "\${{ true }} and \${{ false }}" + runs-on: ubuntu-latest + steps: + - run: echo hi +`; + const result = await validate(createDocument("wf.yaml", input)); + + expect(result).toContainEqual( + expect.objectContaining({ + code: "expression-literal-text-in-condition" + }) + ); + }); + }); }); diff --git a/workflow-parser/src/templates/template-reader.ts b/workflow-parser/src/templates/template-reader.ts index 462dcb9..ff28fed 100644 --- a/workflow-parser/src/templates/template-reader.ts +++ b/workflow-parser/src/templates/template-reader.ts @@ -451,7 +451,13 @@ class TemplateReader { } const allowedContext = definitionInfo.allowedContext; - const raw = token.source || token.value; + const isSingleLine = token.range === undefined || token.range.start.line === token.range.end.line; + + // For single-line strings, use token.value (without YAML quotes) for expression detection, + // because token.source includes quote characters that would be incorrectly detected as literal text. + // For multi-line block scalars, use token.source directly because it makes position calculation easier + // (no quote characters to handle, and token.source preserves the original line/column structure in YAML). + const raw = isSingleLine ? token.value : token.source ?? token.value; let startExpression: number = raw.indexOf(OPEN_EXPRESSION); if (startExpression < 0) { @@ -496,14 +502,17 @@ class TemplateReader { ); let tr = token.range!; - if (tr.start.line === tr.end.line) { - // If it's a single line expression, adjust the range to only cover the sub-expression + if (isSingleLine) { + // Single-line: Adjust the range to only cover the sub-expression. + // Calculate offset to account for YAML quote characters. + // For example, `"${{ expr }}"` has source with quotes, value without. + const offset = (token.source ?? raw).indexOf(OPEN_EXPRESSION) - raw.indexOf(OPEN_EXPRESSION); tr = { - start: {line: tr.start.line, column: tr.start.column + startExpression}, - end: {line: tr.end.line, column: tr.start.column + endExpression + 1} + start: {line: tr.start.line, column: tr.start.column + startExpression + offset}, + end: {line: tr.end.line, column: tr.start.column + endExpression + 1 + offset} }; } else { - // Adjust the range to only cover the expression for multi-line strings + // Multi-line: Adjust the range to only cover the expression const startRaw = raw.substring(0, startExpression); const adjustedStartLine = startRaw.split("\n").length; const beginningOfLine = startRaw.lastIndexOf("\n");