feat(api-history): --disallow-comments

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';
@@ -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,
@@ -290,10 +293,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' +
@@ -306,6 +311,33 @@ async function main(
           continue;
         }
 
+        if (disallowComments) {
+          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` +
+                '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);
@@ -380,7 +412,7 @@ function parseCommandLine() {
       console.log(
         'Usage: lint-roller-markdown-api-history [--root <dir>] <globs>' +
           ' [-h|--help]' +
-          ' [--check-placement] [--breaking-changes-file <path>] [--check-strings] [--check-descriptions]' +
+          ' [--check-placement] [--breaking-changes-file <path>] [--check-strings] [--check-descriptions] [--disallow-comments]' +
           ' [--schema <path>]' +
           ' [--ignore <globs>] [--ignore-path <path>]',
       );
@@ -391,13 +423,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,
     },
   });
 
@@ -444,6 +483,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,
       },

tests/fixtures/api-history-line-comment.md

@@ -0,0 +1,18 @@
+#### `win.setTrafficLightPosition(position)` _macOS_ _Deprecated_
+
+<!--
+```YAML history
+added:
+  # comment
+  - 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`."
+deprecated:
+  - pr-url: https://github.com/electron/electron/pull/37094
+    breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition
+```
+-->
+
+Set a custom position for the traffic light buttons in frameless window.
+Passing `{ x: 0, y: 0 }` will reset the position to default.

tests/fixtures/api-history-separated-comment.md

@@ -0,0 +1,17 @@
+#### `win.setTrafficLightPosition(position)` _macOS_ _Deprecated_
+
+<!--
+```YAML history
+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`."
+deprecated:
+  - pr-url: https://github.com/electron/electron/pull/37094
+    breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition # comment
+```
+-->
+
+Set a custom position for the traffic light buttons in frameless window.
+Passing `{ x: 0, y: 0 }` will reset the position to default.

tests/fixtures/api-history-valid-hashtags.md

@@ -0,0 +1,17 @@
+#### `win.setTrafficLightPosition(position)` _macOS_ _Deprecated_
+
+<!--
+```YAML history
+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`.#"
+deprecated:
+  - pr-url: https://github.com/electron/electron/pull/37094
+    breaking-changes-header: deprecated-browserwindowsettrafficlightpositionposition
+```
+-->
+
+Set a custom position for the traffic light buttons in frameless window.
+Passing `{ x: 0, y: 0 }` will reset the position to default.

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,10 +186,11 @@ describe('lint-roller-markdown-api-history', () => {
       '--check-placement',
       '--check-strings',
       '--check-descriptions',
+      '--disallow-comments',
       '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) ?? [];
 
@@ -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,11 +312,12 @@ describe('lint-roller-markdown-api-history', () => {
       '--check-placement',
       '--check-strings',
       '--check-descriptions',
+      '--disallow-comments',
       'api-history-string-invalid.md',
     );
 
     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) ?? [];
 
@@ -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',
     );