From 610ae81ff72720171cb2097588cee9962cb76901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Tue, 6 Aug 2024 01:22:39 +0100 Subject: [PATCH 1/5] feat(api-history): `--check-descriptions` Reference: https://github.com/electron/lint-roller/issues/78#issue-2446621568 --- bin/lint-markdown-api-history.ts | 47 +++++++++++++++++-- tests/fixtures/api-history-format-invalid.md | 2 +- tests/fixtures/api-history-heading-missing.md | 2 +- .../fixtures/api-history-placement-invalid.md | 2 +- tests/fixtures/api-history-schema-invalid.md | 2 +- tests/fixtures/api-history-string-invalid.md | 6 +-- tests/fixtures/api-history-valid.md | 2 +- tests/fixtures/api-history-yaml-invalid.md | 2 +- .../lint-roller-markdown-api-history.spec.ts | 18 +++++-- 9 files changed, 68 insertions(+), 15 deletions(-) diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index 5f6be88..af5ef4e 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -19,6 +19,7 @@ import { DocsWorkspace } from '../lib/markdown'; // ": " const possibleStringRegex = /^[ \S]+?: *?(\S[ \S]+?)$/gm; const nonAlphaNumericDotRegex = /[^a-zA-Z0-9.]/g; +const possibleDescriptionRegex = /^[ \S]+?description: *?(\S[ \S]+?)$/gm; interface ChangeSchema { 'pr-url': string; @@ -39,6 +40,8 @@ interface Options { breakingChangesFile: string; // Check if the API history block contains strings that might cause issues when parsing the YAML checkStrings: boolean; + // Check if the API history block contains descriptions that aren't surrounded by double quotation marks + checkDescriptions: boolean; // Array of glob patterns to ignore when processing files ignoreGlobs: string[]; // Check if the API history block's YAML adheres to the JSON schema at this filepath @@ -98,7 +101,14 @@ type LintingResults = { async function main( workspaceRoot: string, globs: string[], - { checkPlacement, breakingChangesFile, checkStrings, schema, ignoreGlobs = [] }: Options, + { + checkPlacement, + breakingChangesFile, + checkStrings, + checkDescriptions, + schema, + ignoreGlobs = [], + }: Options, ): Promise { let documentCounter = 0; let historyBlockCounter = 0; @@ -237,6 +247,35 @@ async function main( } } + // Throw an error if a description isn't surrounded by double quotation marks + if (checkDescriptions) { + const possibleDescription = codeBlock.value.matchAll(possibleDescriptionRegex); + + for (const [matchedLine, matchedGroup] of possibleDescription) { + const trimmedMatchedGroup = matchedGroup.trim(); + const isMatchedGroupInsideQuotes = + (trimmedMatchedGroup.startsWith('"') && trimmedMatchedGroup.endsWith('"')) || + (trimmedMatchedGroup.startsWith("'") && trimmedMatchedGroup.endsWith("'")); + + if (isMatchedGroupInsideQuotes) continue; + + console.error( + 'Error occurred while parsing Markdown document:\n\n' + + `'${filepath}'\n\n` + + 'Possible description field is not surrounded by double quotes.\n\n' + + 'This might cause issues when parsing the YAML (might not throw an error)\n\n' + + 'Matched group:\n\n' + + `${matchedGroup}\n\n` + + 'Matched line:\n\n' + + `${matchedLine}\n\n` + + 'API history block:\n\n' + + `${possibleHistoryBlock.value}\n`, + ); + errorCounter++; + continue; + } + } + if (checkPlacement) { if (possibleHistoryBlock.previousNode?.type !== 'heading') { console.error( @@ -341,7 +380,7 @@ function parseCommandLine() { console.log( 'Usage: lint-roller-markdown-api-history [--root ] ' + ' [-h|--help]' + - ' [--check-placement] [--breaking-changes-file ] [--check-strings]' + + ' [--check-placement] [--breaking-changes-file ] [--check-strings] [--check-descriptions]' + ' [--schema ]' + ' [--ignore ] [--ignore-path ]', ); @@ -352,12 +391,13 @@ function parseCommandLine() { }; const opts = minimist(process.argv.slice(2), { - boolean: ['help', 'check-placement', 'check-strings'], + boolean: ['help', 'check-placement', 'check-strings', 'check-descriptions'], string: ['root', 'ignore', 'ignore-path', 'schema', 'breaking-changes-file'], unknown: showUsage, default: { 'check-placement': true, 'check-strings': true, + 'check-descriptions': true, }, }); @@ -403,6 +443,7 @@ async function init() { checkPlacement: opts['check-placement'], breakingChangesFile: opts['breaking-changes-file'], checkStrings: opts['check-strings'], + checkDescriptions: opts['check-descriptions'], ignoreGlobs: opts.ignore, schema: opts.schema, }, diff --git a/tests/fixtures/api-history-format-invalid.md b/tests/fixtures/api-history-format-invalid.md index e602747..61b48de 100644 --- a/tests/fixtures/api-history-format-invalid.md +++ b/tests/fixtures/api-history-format-invalid.md @@ -7,7 +7,7 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`. + description: "Made `trafficLightPosition` option work for `customButtonOnHover`." deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition diff --git a/tests/fixtures/api-history-heading-missing.md b/tests/fixtures/api-history-heading-missing.md index 30fa4d1..b40ccdb 100644 --- a/tests/fixtures/api-history-heading-missing.md +++ b/tests/fixtures/api-history-heading-missing.md @@ -6,7 +6,7 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`. + description: "Made `trafficLightPosition` option work for `customButtonOnHover`." deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-missing-header diff --git a/tests/fixtures/api-history-placement-invalid.md b/tests/fixtures/api-history-placement-invalid.md index 8ff360b..9140d53 100644 --- a/tests/fixtures/api-history-placement-invalid.md +++ b/tests/fixtures/api-history-placement-invalid.md @@ -9,7 +9,7 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`. + description: "Made `trafficLightPosition` option work for `customButtonOnHover`." deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition diff --git a/tests/fixtures/api-history-schema-invalid.md b/tests/fixtures/api-history-schema-invalid.md index 7ad1385..bbbd9dc 100644 --- a/tests/fixtures/api-history-schema-invalid.md +++ b/tests/fixtures/api-history-schema-invalid.md @@ -6,7 +6,7 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Ma + description: "Ma" deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition diff --git a/tests/fixtures/api-history-string-invalid.md b/tests/fixtures/api-history-string-invalid.md index ae8e247..6d975a7 100644 --- a/tests/fixtures/api-history-string-invalid.md +++ b/tests/fixtures/api-history-string-invalid.md @@ -5,10 +5,10 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`: + - pr-url: https://github.com/electron/electron/pull/26789: + description: "Made `trafficLightPosition` option work for `customButtonOnHover`" - pr-url: https://github.com/electron/electron/pull/1337 - description: "Made `trafficLightPosition` option work for `customButtonOnHover`:" + description: "Made `trafficLightPosition` option work for `customButtonOnHover`" ``` --> diff --git a/tests/fixtures/api-history-valid.md b/tests/fixtures/api-history-valid.md index 7b5a246..a409567 100644 --- a/tests/fixtures/api-history-valid.md +++ b/tests/fixtures/api-history-valid.md @@ -6,7 +6,7 @@ added: - pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`. + description: "Made `trafficLightPosition` option work for `customButtonOnHover`." deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition diff --git a/tests/fixtures/api-history-yaml-invalid.md b/tests/fixtures/api-history-yaml-invalid.md index da40ac1..39f0da1 100644 --- a/tests/fixtures/api-history-yaml-invalid.md +++ b/tests/fixtures/api-history-yaml-invalid.md @@ -5,7 +5,7 @@ added: pr-url: https://github.com/electron/electron/pull/22533 changes: - pr-url: https://github.com/electron/electron/pull/26789 - description: Made `trafficLightPosition` option work for `customButtonOnHover`. + description: "Made `trafficLightPosition` option work for `customButtonOnHover`." deprecated: - pr-url: https://github.com/electron/electron/pull/37094 breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition diff --git a/tests/lint-roller-markdown-api-history.spec.ts b/tests/lint-roller-markdown-api-history.spec.ts index 170e60b..c667ef3 100644 --- a/tests/lint-roller-markdown-api-history.spec.ts +++ b/tests/lint-roller-markdown-api-history.spec.ts @@ -72,7 +72,7 @@ async function generateRandomApiDocuments(): Promise\n\n' + 'Set a custom position for the traffic light buttons in frameless window.\n' + @@ -161,6 +161,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-valid.md', ); @@ -183,6 +184,7 @@ describe('lint-roller-markdown-api-history', () => { API_HISTORY_SCHEMA, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-yaml-invalid.md', ); @@ -205,6 +207,7 @@ describe('lint-roller-markdown-api-history', () => { API_HISTORY_SCHEMA, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-schema-invalid.md', ); @@ -227,6 +230,7 @@ describe('lint-roller-markdown-api-history', () => { API_HISTORY_SCHEMA, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-format-invalid.md', ); @@ -251,6 +255,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-heading-missing.md', ); @@ -275,6 +280,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-placement-invalid.md', ); @@ -299,6 +305,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', 'api-history-string-invalid.md', ); @@ -322,6 +329,7 @@ describe('lint-roller-markdown-api-history', () => { API_HISTORY_SCHEMA, '--check-placement', '--check-strings', + '--check-descriptions', '--ignore', '**/api-history-yaml-invalid.md', '{api-history-valid,api-history-yaml-invalid}.md', @@ -344,6 +352,7 @@ describe('lint-roller-markdown-api-history', () => { API_HISTORY_SCHEMA, '--check-placement', '--check-strings', + '--check-descriptions', '--ignore', '**/api-history-valid.md', '--ignore', @@ -370,6 +379,7 @@ describe('lint-roller-markdown-api-history', () => { resolve(FIXTURES_DIR, 'ignorepaths'), '--check-placement', '--check-strings', + '--check-descriptions', '{api-history-valid,api-history-yaml-invalid}.md', ); @@ -392,6 +402,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', '{api-history-valid,api-history-yaml-invalid,api-history-heading-missing}.md', ); @@ -426,6 +437,7 @@ describe('lint-roller-markdown-api-history', () => { BREAKING_CHANGES_FILE, '--check-placement', '--check-strings', + '--check-descriptions', '*.md', ); From 8c2c3e06a94820b01de10f596838191576465108 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Tue, 6 Aug 2024 01:28:22 +0100 Subject: [PATCH 2/5] test(api-history): description test --- .../api-history-description-invalid.md | 17 +++++++++++++ .../lint-roller-markdown-api-history.spec.ts | 25 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 tests/fixtures/api-history-description-invalid.md diff --git a/tests/fixtures/api-history-description-invalid.md b/tests/fixtures/api-history-description-invalid.md new file mode 100644 index 0000000..7b5a246 --- /dev/null +++ b/tests/fixtures/api-history-description-invalid.md @@ -0,0 +1,17 @@ +#### `win.setTrafficLightPosition(position)` _macOS_ _Deprecated_ + + + +Set a custom position for the traffic light buttons in frameless window. +Passing `{ x: 0, y: 0 }` will reset the position to default. diff --git a/tests/lint-roller-markdown-api-history.spec.ts b/tests/lint-roller-markdown-api-history.spec.ts index c667ef3..8b4a6c7 100644 --- a/tests/lint-roller-markdown-api-history.spec.ts +++ b/tests/lint-roller-markdown-api-history.spec.ts @@ -321,6 +321,31 @@ describe('lint-roller-markdown-api-history', () => { expect(status).toEqual(1); }); + it('should not run clean when there are description errors', () => { + const { status, stdout, stderr } = runLintMarkdownApiHistory( + '--root', + FIXTURES_DIR, + '--schema', + API_HISTORY_SCHEMA, + '--breaking-changes-file', + BREAKING_CHANGES_FILE, + '--check-placement', + '--check-strings', + '--check-descriptions', + 'api-history-description-invalid.md', + ); + + expect(stderr).toMatch(/Possible description field is not surrounded by double quotes./); + + const [blocks, documents, errors, warnings] = stdoutRegex.exec(stdout)?.slice(1, 5) ?? []; + + expect(Number(blocks)).toEqual(1); + expect(Number(documents)).toEqual(1); + expect(Number(errors)).toEqual(1); + expect(Number(warnings)).toEqual(0); + expect(status).toEqual(1); + }); + it('can ignore a glob', () => { const { status, stdout } = runLintMarkdownApiHistory( '--root', From 18d04406f2ff92798b7621b423676a81306cdad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Tue, 6 Aug 2024 01:32:00 +0100 Subject: [PATCH 3/5] fix(api-history): only double quotes --- bin/lint-markdown-api-history.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index af5ef4e..dc1f35b 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -254,8 +254,7 @@ async function main( for (const [matchedLine, matchedGroup] of possibleDescription) { const trimmedMatchedGroup = matchedGroup.trim(); const isMatchedGroupInsideQuotes = - (trimmedMatchedGroup.startsWith('"') && trimmedMatchedGroup.endsWith('"')) || - (trimmedMatchedGroup.startsWith("'") && trimmedMatchedGroup.endsWith("'")); + trimmedMatchedGroup.startsWith('"') && trimmedMatchedGroup.endsWith('"'); if (isMatchedGroupInsideQuotes) continue; From 7fa70bde7f8439998e365e673a3d7c057ade33df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Fri, 9 Aug 2024 12:13:08 +0100 Subject: [PATCH 4/5] fix(api-history): description `for` loop Co-authored-by: David Sanders Reference: https://github.com/electron/lint-roller/pull/80#discussion_r1710543868 --- bin/lint-markdown-api-history.ts | 33 ++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index dc1f35b..be81dc3 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -179,7 +179,7 @@ async function main( const possibleHistoryBlocks = await findPossibleApiHistoryBlocks(documentText); - for (const possibleHistoryBlock of possibleHistoryBlocks) { + historyBlockForLoop: for (const possibleHistoryBlock of possibleHistoryBlocks) { historyBlockCounter++; const { @@ -256,22 +256,23 @@ async function main( const isMatchedGroupInsideQuotes = trimmedMatchedGroup.startsWith('"') && trimmedMatchedGroup.endsWith('"'); - if (isMatchedGroupInsideQuotes) continue; + if (!isMatchedGroupInsideQuotes) { + console.error( + 'Error occurred while parsing Markdown document:\n\n' + + `'${filepath}'\n\n` + + 'Possible description field is not surrounded by double quotes.\n\n' + + 'This might cause issues when parsing the YAML (might not throw an error)\n\n' + + 'Matched group:\n\n' + + `${matchedGroup}\n\n` + + 'Matched line:\n\n' + + `${matchedLine}\n\n` + + 'API history block:\n\n' + + `${possibleHistoryBlock.value}\n`, + ); + errorCounter++; + continue historyBlockForLoop; + } - console.error( - 'Error occurred while parsing Markdown document:\n\n' + - `'${filepath}'\n\n` + - 'Possible description field is not surrounded by double quotes.\n\n' + - 'This might cause issues when parsing the YAML (might not throw an error)\n\n' + - 'Matched group:\n\n' + - `${matchedGroup}\n\n` + - 'Matched line:\n\n' + - `${matchedLine}\n\n` + - 'API history block:\n\n' + - `${possibleHistoryBlock.value}\n`, - ); - errorCounter++; - continue; } } From ee12f2f84a2e7ded4cd3712d8b0d9d0fafbd8656 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Fri, 9 Aug 2024 12:20:36 +0100 Subject: [PATCH 5/5] style(api-history): fix lint, leave comment --- bin/lint-markdown-api-history.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index be81dc3..019a609 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -270,9 +270,9 @@ async function main( `${possibleHistoryBlock.value}\n`, ); errorCounter++; + // Behold, one of the rare occasions when a labeled statement is useful. continue historyBlockForLoop; } - } }