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/7] 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/7] 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/7] 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 6a5ed31463f0dc4fd4a99a2de7c129e697ed1ea8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Wed, 7 Aug 2024 01:26:31 +0100 Subject: [PATCH 4/7] feat(api-history): `--disallow-comments` Reference: https://github.com/electron/lint-roller/issues/79 --- bin/lint-markdown-api-history.ts | 36 ++++++++++++++++- tests/fixtures/api-history-line-comment.md | 18 +++++++++ .../fixtures/api-history-separated-comment.md | 17 ++++++++ tests/fixtures/api-history-valid-hashtags.md | 17 ++++++++ .../lint-roller-markdown-api-history.spec.ts | 40 +++++++++++++++++++ 5 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 tests/fixtures/api-history-line-comment.md create mode 100644 tests/fixtures/api-history-separated-comment.md create mode 100644 tests/fixtures/api-history-valid-hashtags.md diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index dc1f35b..b4ce517 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -42,6 +42,8 @@ interface Options { checkStrings: boolean; // Check if the API history block contains descriptions that aren't surrounded by double quotation marks checkDescriptions: boolean; + // Check if the API history block contains comments + disallowComments: 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 @@ -106,6 +108,7 @@ async function main( breakingChangesFile, checkStrings, checkDescriptions, + disallowComments, schema, ignoreGlobs = [], }: Options, @@ -305,6 +308,27 @@ async function main( continue; } + if (disallowComments) { + const numberOfHashtagsInCodeBlock = (codeBlock.value.match(/#/g) || []).length; + const unsafeHistoryJsonString = JSON.stringify(unsafeHistory); + const numberOfHashtagsInHistoryJsonString = (unsafeHistoryJsonString.match(/#/g) || []) + .length; + + // If the number of hashtags in the code block is greater than the number of hashtags in the JSON string, + // then the API History contains comments. + if (numberOfHashtagsInCodeBlock > numberOfHashtagsInHistoryJsonString) { + console.error( + 'Error occurred while parsing Markdown document:\n\n' + + `'${filepath}'\n\n` + + 'API History cannot contain YAML comments.\n\n' + + 'API history block:\n\n' + + `${possibleHistoryBlock.value}\n`, + ); + errorCounter++; + continue; + } + } + if (!schema || validateAgainstSchema === null) continue; const isValid = validateAgainstSchema(unsafeHistory); @@ -379,7 +403,7 @@ function parseCommandLine() { console.log( 'Usage: lint-roller-markdown-api-history [--root ] ' + ' [-h|--help]' + - ' [--check-placement] [--breaking-changes-file ] [--check-strings] [--check-descriptions]' + + ' [--check-placement] [--breaking-changes-file ] [--check-strings] [--check-descriptions] [--disallow-comments]' + ' [--schema ]' + ' [--ignore ] [--ignore-path ]', ); @@ -390,13 +414,20 @@ function parseCommandLine() { }; const opts = minimist(process.argv.slice(2), { - boolean: ['help', 'check-placement', 'check-strings', 'check-descriptions'], + boolean: [ + 'help', + 'check-placement', + 'check-strings', + 'check-descriptions', + 'disallow-comments', + ], string: ['root', 'ignore', 'ignore-path', 'schema', 'breaking-changes-file'], unknown: showUsage, default: { 'check-placement': true, 'check-strings': true, 'check-descriptions': true, + 'disallow-comments': true, }, }); @@ -443,6 +474,7 @@ async function init() { breakingChangesFile: opts['breaking-changes-file'], checkStrings: opts['check-strings'], checkDescriptions: opts['check-descriptions'], + disallowComments: opts['disallow-comments'], ignoreGlobs: opts.ignore, schema: opts.schema, }, diff --git a/tests/fixtures/api-history-line-comment.md b/tests/fixtures/api-history-line-comment.md new file mode 100644 index 0000000..251f7ad --- /dev/null +++ b/tests/fixtures/api-history-line-comment.md @@ -0,0 +1,18 @@ +#### `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/fixtures/api-history-separated-comment.md b/tests/fixtures/api-history-separated-comment.md new file mode 100644 index 0000000..796fdc5 --- /dev/null +++ b/tests/fixtures/api-history-separated-comment.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/fixtures/api-history-valid-hashtags.md b/tests/fixtures/api-history-valid-hashtags.md new file mode 100644 index 0000000..6003cc9 --- /dev/null +++ b/tests/fixtures/api-history-valid-hashtags.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 8b4a6c7..553aa75 100644 --- a/tests/lint-roller-markdown-api-history.spec.ts +++ b/tests/lint-roller-markdown-api-history.spec.ts @@ -162,6 +162,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-valid.md', ); @@ -185,6 +186,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-yaml-invalid.md', ); @@ -208,6 +210,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-schema-invalid.md', ); @@ -231,6 +234,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-format-invalid.md', ); @@ -256,6 +260,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-heading-missing.md', ); @@ -281,6 +286,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-placement-invalid.md', ); @@ -306,6 +312,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-string-invalid.md', ); @@ -332,6 +339,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', 'api-history-description-invalid.md', ); @@ -346,6 +354,32 @@ describe('lint-roller-markdown-api-history', () => { expect(status).toEqual(1); }); + it('should not run clean when there are yaml comments', () => { + const { status, stdout, stderr } = runLintMarkdownApiHistory( + '--root', + FIXTURES_DIR, + '--schema', + API_HISTORY_SCHEMA, + '--breaking-changes-file', + BREAKING_CHANGES_FILE, + '--check-placement', + '--check-strings', + '--check-descriptions', + '--disallow-comments', + '{api-history-line-comment,api-history-separated-comment,api-history-valid-hashtags}.md', + ); + + expect(stderr).toMatch(/API History cannot contain YAML comments./); + + const [blocks, documents, errors, warnings] = stdoutRegex.exec(stdout)?.slice(1, 5) ?? []; + + expect(Number(blocks)).toEqual(3); + expect(Number(documents)).toEqual(3); + expect(Number(errors)).toEqual(2); + expect(Number(warnings)).toEqual(0); + expect(status).toEqual(1); + }); + it('can ignore a glob', () => { const { status, stdout } = runLintMarkdownApiHistory( '--root', @@ -355,6 +389,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', '--ignore', '**/api-history-yaml-invalid.md', '{api-history-valid,api-history-yaml-invalid}.md', @@ -378,6 +413,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', '--ignore', '**/api-history-valid.md', '--ignore', @@ -405,6 +441,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', '{api-history-valid,api-history-yaml-invalid}.md', ); @@ -428,6 +465,7 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', '{api-history-valid,api-history-yaml-invalid,api-history-heading-missing}.md', ); @@ -463,6 +501,8 @@ describe('lint-roller-markdown-api-history', () => { '--check-placement', '--check-strings', '--check-descriptions', + '--disallow-comments', + 'false', '*.md', ); From fbdef1543b2989eea92d4e28095e946370b41fb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20P=C5=82aczek?= Date: Wed, 7 Aug 2024 16:50:57 +0100 Subject: [PATCH 5/7] feat(api-history): traverse tree instead --- bin/lint-markdown-api-history.ts | 28 ++++++++++++------- .../lint-roller-markdown-api-history.spec.ts | 4 +-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/bin/lint-markdown-api-history.ts b/bin/lint-markdown-api-history.ts index b4ce517..2282aa1 100644 --- a/bin/lint-markdown-api-history.ts +++ b/bin/lint-markdown-api-history.ts @@ -11,7 +11,7 @@ import * as minimist from 'minimist'; import type { Literal, Node } from 'unist'; import type { visit as VisitFunction } from 'unist-util-visit'; import { URI } from 'vscode-uri'; -import { parse as parseYaml } from 'yaml'; +import { parseDocument, visit as yamlVisit } from 'yaml'; import { dynamicImport } from '../lib/helpers'; import { DocsWorkspace } from '../lib/markdown'; @@ -292,10 +292,12 @@ async function main( } } + let unsafeHistoryDocument = null; let unsafeHistory = null; try { - unsafeHistory = parseYaml(codeBlock.value); + unsafeHistoryDocument = parseDocument(codeBlock.value); + unsafeHistory = unsafeHistoryDocument.toJS(); } catch (error) { console.error( 'Error occurred while parsing Markdown document:\n\n' + @@ -309,14 +311,20 @@ async function main( } if (disallowComments) { - const numberOfHashtagsInCodeBlock = (codeBlock.value.match(/#/g) || []).length; - const unsafeHistoryJsonString = JSON.stringify(unsafeHistory); - const numberOfHashtagsInHistoryJsonString = (unsafeHistoryJsonString.match(/#/g) || []) - .length; - - // If the number of hashtags in the code block is greater than the number of hashtags in the JSON string, - // then the API History contains comments. - if (numberOfHashtagsInCodeBlock > numberOfHashtagsInHistoryJsonString) { + let commentFound = false; + + yamlVisit(unsafeHistoryDocument, (_, node) => { + if ( + typeof node === 'object' && + node !== null && + ('comment' in node || 'commentBefore' in node) + ) { + commentFound = true; + return yamlVisit.BREAK; + } + }); + + if (commentFound) { console.error( 'Error occurred while parsing Markdown document:\n\n' + `'${filepath}'\n\n` + diff --git a/tests/lint-roller-markdown-api-history.spec.ts b/tests/lint-roller-markdown-api-history.spec.ts index 553aa75..3485cde 100644 --- a/tests/lint-roller-markdown-api-history.spec.ts +++ b/tests/lint-roller-markdown-api-history.spec.ts @@ -190,7 +190,7 @@ describe('lint-roller-markdown-api-history', () => { 'api-history-yaml-invalid.md', ); - expect(stderr).toMatch(/YAMLParseError: Nested mappings are not allowed/); + expect(stderr).toMatch(/must be array/); const [blocks, documents, errors, warnings] = stdoutRegex.exec(stdout)?.slice(1, 5) ?? []; @@ -317,7 +317,7 @@ describe('lint-roller-markdown-api-history', () => { ); expect(stderr).toMatch(/Possible string value starts\/ends with a non-alphanumeric character/); - expect(stderr).toMatch(/YAMLParseError: Nested mappings are not allowed/); + expect(stderr).toMatch(/must be string/); const [blocks, documents, errors, warnings] = stdoutRegex.exec(stdout)?.slice(1, 5) ?? []; 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 6/7] 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 7/7] 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; } - } }