From 58d92658b6fe2cbdc00b83a8bb5e5e7045b17bb0 Mon Sep 17 00:00:00 2001 From: Jacob Wallraff Date: Mon, 23 Jan 2023 17:50:54 -0800 Subject: [PATCH] Address feedback --- actions-languageservice/src/hover.test.ts | 4 ++-- actions-languageservice/src/hover.ts | 17 +++++------------ .../src/model/converter/cron.test.ts | 10 ++++++---- .../src/model/converter/cron.ts | 9 +++++---- .../src/templates/template-reader.ts | 2 +- 5 files changed, 19 insertions(+), 23 deletions(-) diff --git a/actions-languageservice/src/hover.test.ts b/actions-languageservice/src/hover.test.ts index 6a3e35d..a45ba9a 100644 --- a/actions-languageservice/src/hover.test.ts +++ b/actions-languageservice/src/hover.test.ts @@ -110,7 +110,7 @@ jobs: ); }); - it("on a cron identifier", async () => { + it("on a cron mapping key", async () => { const input = `on: schedule: - c|ron: '0 0 * * *' @@ -127,7 +127,7 @@ jobs: `; const result = await hover(...getPositionFromCursor(input)); expect(result).not.toBeUndefined(); - expect(result?.contents).toEqual("Invalid cron expression"); + expect(result?.contents).toEqual(""); }); }); diff --git a/actions-languageservice/src/hover.ts b/actions-languageservice/src/hover.ts index e6d641d..2db5dfd 100644 --- a/actions-languageservice/src/hover.ts +++ b/actions-languageservice/src/hover.ts @@ -12,7 +12,7 @@ import {info} from "./log"; import {nullTrace} from "./nulltrace"; import {findToken} from "./utils/find-token"; import {mapRange} from "./utils/range"; -import {getSentence} from "@github/actions-workflow-parser/model/converter/cron"; +import {getCronDescription} from "@github/actions-workflow-parser/model/converter/cron"; export type DescriptionProvider = { getDescription(context: WorkflowContext, token: TemplateToken, path: TemplateToken[]): Promise; @@ -40,20 +40,13 @@ export async function hover(document: TextDocument, position: Position, config?: if (tokenResult.parent && isCronMappingValue(tokenResult)) { const tokenValue = (token as StringToken).value - let description = getSentence(tokenValue); + let description = getCronDescription(tokenValue); if (description) { - description += - "\n\nActions schedules run at most every 5 minutes." + - " [Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)"; return { contents: description, range: mapRange(token.range) } as Hover; } - return { - contents: "Invalid cron expression", - range: mapRange(token.range) - } as Hover; } let description = await getDescription(document, config, result, token, tokenResult.path); @@ -90,7 +83,7 @@ async function getDescription( } function isCronMappingValue(tokenResult: TokenResult): boolean { - return tokenResult.parent?.definition?.key === "cron-mapping" && - (tokenResult.token as StringToken).value !== "cron" && - isString(tokenResult.token!); + //return tokenResult.parent?.definition?.key === "cron-mapping" && + return isString(tokenResult.token!) && + (tokenResult.token as StringToken).value !== "cron" } diff --git a/actions-workflow-parser/src/model/converter/cron.test.ts b/actions-workflow-parser/src/model/converter/cron.test.ts index 6bd3659..56a2a3e 100644 --- a/actions-workflow-parser/src/model/converter/cron.test.ts +++ b/actions-workflow-parser/src/model/converter/cron.test.ts @@ -1,4 +1,4 @@ -import { isValidCron, getSchedule, getSentence } from "./cron" +import { isValidCron, getSchedule, getCronDescription } from "./cron" describe("cron", () => { describe("valid cron", () => { @@ -145,13 +145,15 @@ describe("cron", () => { } }) - describe("getSentence", () => { + describe("getCronDescription", () => { it(`Produces a sentence for valid cron`, () => { - expect(getSentence("0 * * * *")).toEqual("Every hour") + expect(getCronDescription("0 * * * *")).toEqual("Runs every hour\n\n" + + "Actions schedules run at most every 5 minutes. [Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)" + ) }) it(`Returns nothing for invalid cron`, () => { - expect(getSentence("* * * * * *")).toBeUndefined() + expect(getCronDescription("* * * * * *")).toBeUndefined() }) }) }) \ No newline at end of file diff --git a/actions-workflow-parser/src/model/converter/cron.ts b/actions-workflow-parser/src/model/converter/cron.ts index 5246b66..1bc2572 100644 --- a/actions-workflow-parser/src/model/converter/cron.ts +++ b/actions-workflow-parser/src/model/converter/cron.ts @@ -40,7 +40,7 @@ export function isValidCron(cron: string): boolean { ) } -export function getSentence(cronspec: string): string | undefined { +export function getCronDescription(cronspec: string): string | undefined { const schedule = getSchedule(cronspec) if (!schedule) { return @@ -61,7 +61,10 @@ export function getSentence(cronspec: string): string | undefined { } // Make first character lowercase - return "Runs " + desc.charAt(0).toLowerCase() + desc.slice(1) + let result = "Runs " + desc.charAt(0).toLowerCase() + desc.slice(1) + result += "\n\nActions schedules run at most every 5 minutes." + + " [Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)"; + return result } function parseCronPart(part: string, range: Range): number[] | undefined { @@ -195,8 +198,6 @@ export function getSchedule(cron: string): Schedule | undefined { } } -// Helpers - // Converts a string integer or a short name to a number function convertToNumber(value: string, names?: Record): number { if (names && names[value.toLowerCase()] !== undefined) { diff --git a/actions-workflow-parser/src/templates/template-reader.ts b/actions-workflow-parser/src/templates/template-reader.ts index 7523266..f7290a2 100644 --- a/actions-workflow-parser/src/templates/template-reader.ts +++ b/actions-workflow-parser/src/templates/template-reader.ts @@ -215,7 +215,6 @@ class TemplateReader { const nextPropertyDef = this._schema.matchPropertyAndFilter(mappingDefinitions, nextKey.value); if (nextPropertyDef) { const nextDefinition = new DefinitionInfo(definition, nextPropertyDef.type); - const nextValue = this.readValue(nextDefinition); // Store the definition on the key, the value may have its own definition nextKey.definitionInfo = nextDefinition; @@ -226,6 +225,7 @@ class TemplateReader { nextKey.description = nextPropertyDef.description; } + const nextValue = this.readValue(nextDefinition); mapping.add(nextKey, nextValue); continue; }