chore(lint): report error on invalid change versions, except REPLACEME (#238)

* chore(lint): report error on invalid change versions, except when REPLACEME

* don't assume`v` in NODE_RELEASED_VERSIONS

* code review

Author: avivkeller

src/linter/rules/invalid-change-version.mjs

@@ -1,42 +1,54 @@
 import { LINT_MESSAGES } from '../constants.mjs';
 import { valid } from 'semver';
+import { env } from 'node:process';
+
+const NODE_RELEASED_VERSIONS = env.NODE_RELEASED_VERSIONS?.split(',');
 
 /**
- * Checks if any change version is invalid
+ * Checks if the given version is "REPLACEME" and the array length is 1.
  *
- * @param {ApiDocMetadataEntry[]} entries
- * @returns {Array<import('../types').LintIssue>}
+ * @param {string} version - The version to check.
+ * @param {number} length - Length of the version array.
+ * @returns {boolean} True if conditions match, otherwise false.
  */
-export const invalidChangeVersion = entries => {
-  const issues = [];
-
-  for (const entry of entries) {
-    if (entry.changes.length === 0) continue;
-
-    const allVersions = entry.changes
-      .filter(change => change.version)
-      .flatMap(change =>
-        Array.isArray(change.version) ? change.version : [change.version]
-      );
+const isValidReplaceMe = (version, length) =>
+  length === 1 && version === 'REPLACEME';
 
-    const invalidVersions = allVersions.filter(
-      version => valid(version) === null
-    );
-
-    issues.push(
-      ...invalidVersions.map(version => ({
-        level: 'warn',
-        message: LINT_MESSAGES.invalidChangeVersion.replace(
-          '{{version}}',
-          version
-        ),
-        location: {
-          path: entry.api_doc_source,
-          position: entry.yaml_position,
-        },
-      }))
-    );
-  }
+/**
+ * Determines if a given version is invalid.
+ *
+ * @param {string} version - The version to check.
+ * @param {unknown} _ - Unused parameter.
+ * @param {{ length: number }} context - Array containing the length property.
+ * @returns {boolean} True if the version is invalid, otherwise false.
+ */
+const isInvalid = NODE_RELEASED_VERSIONS
+  ? (version, _, { length }) =>
+      !(
+        isValidReplaceMe(version, length) ||
+        NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, ''))
+      )
+  : (version, _, { length }) =>
+      !(isValidReplaceMe(version, length) || valid(version));
 
-  return issues;
-};
+/**
+ * Identifies invalid change versions from metadata entries.
+ *
+ * @param {ApiDocMetadataEntry[]} entries - Metadata entries to check.
+ * @returns {import('../types').LintIssue[]} List of detected lint issues.
+ */
+export const invalidChangeVersion = entries =>
+  entries.flatMap(({ changes, api_doc_source, yaml_position }) =>
+    changes.flatMap(({ version }) =>
+      (Array.isArray(version) ? version : [version])
+        .filter(isInvalid)
+        .map(version => ({
+          level: 'error',
+          message: LINT_MESSAGES.invalidChangeVersion.replace(
+            '{{version}}',
+            version
+          ),
+          location: { path: api_doc_source, position: yaml_position },
+        }))
+    )
+  );

src/linter/tests/fixtures/entries.mjs

@@ -39,6 +39,11 @@ export const assertEntry = {
       'pr-url': 'https://github.com/nodejs/node/pull/34001',
       description: "Exposed as `require('node:assert/strict')`.",
     },
+    {
+      version: 'REPLACEME',
+      'pr-url': 'https://github.com/nodejs/node/pull/12345',
+      description: 'This is a test entry.',
+    },
   ],
   heading: {
     type: 'heading',

src/linter/tests/fixtures/invalidChangeVersion-environment.mjs

@@ -0,0 +1,15 @@
+import { invalidChangeVersion } from '../../rules/invalid-change-version.mjs';
+import { deepEqual } from 'node:assert';
+import { assertEntry } from './entries.mjs';
+
+const issues = invalidChangeVersion([
+  {
+    ...assertEntry,
+    changes: [
+      ...assertEntry.changes,
+      { version: ['SOME_OTHER_RELEASED_VERSION'] },
+    ],
+  },
+]);
+
+deepEqual(issues, []);

src/linter/tests/rules/invalid-change-version.test.mjs

@@ -1,13 +1,43 @@
 import { describe, it } from 'node:test';
 import { invalidChangeVersion } from '../../rules/invalid-change-version.mjs';
-import { deepEqual } from 'node:assert';
+import { deepEqual, strictEqual } from 'node:assert';
 import { assertEntry } from '../fixtures/entries.mjs';
+import { spawnSync } from 'node:child_process';
+import { fileURLToPath } from 'node:url';
+import { execPath } from 'node:process';
 
 describe('invalidChangeVersion', () => {
-  it('should return an empty array if all change versions are valid', () => {
-    const issues = invalidChangeVersion([assertEntry]);
+  it('should work with NODE_RELEASED_VERSIONS', () => {
+    const result = spawnSync(
+      execPath,
+      [
+        fileURLToPath(
+          new URL(
+            '../fixtures/invalidChangeVersion-environment.mjs',
+            import.meta.url
+          )
+        ),
+      ],
+      {
+        env: {
+          NODE_RELEASED_VERSIONS: [
+            '9.9.0',
+            '13.9.0',
+            '12.16.2',
+            '15.0.0',
+            'REPLACEME',
+            'SOME_OTHER_RELEASED_VERSION',
+          ].join(','),
+        },
+      }
+    );
 
-    deepEqual(issues, []);
+    strictEqual(result.status, 0);
+    strictEqual(result.error, undefined);
+  });
+
+  it('should return an empty array if all change versions are valid', () => {
+    deepEqual(invalidChangeVersion([assertEntry]), []);
   });
 
   it('should return an issue if a change version is invalid', () => {
@@ -16,27 +46,45 @@ describe('invalidChangeVersion', () => {
         ...assertEntry,
         changes: [
           ...assertEntry.changes,
-          { version: ['v13.9.0', 'REPLACEME'] },
+          { version: ['v13.9.0', 'INVALID_VERSION'] },
+        ],
+      },
+    ]);
+
+    deepEqual(issues, [
+      {
+        level: 'error',
+        location: {
+          path: 'doc/api/assert.md',
+          position: {
+            start: { column: 1, line: 7, offset: 103 },
+            end: { column: 35, line: 7, offset: 137 },
+          },
+        },
+        message: 'Invalid version number: INVALID_VERSION',
+      },
+    ]);
+  });
+
+  it('should return an issue if a change version contains a REPLACEME and a version', () => {
+    const issues = invalidChangeVersion([
+      {
+        ...assertEntry,
+        changes: [
+          ...assertEntry.changes,
+          { version: ['v24.0.0', 'REPLACEME'] },
         ],
       },
     ]);
 
     deepEqual(issues, [
       {
-        level: 'warn',
+        level: 'error',
         location: {
           path: 'doc/api/assert.md',
           position: {
-            end: {
-              column: 35,
-              line: 7,
-              offset: 137,
-            },
-            start: {
-              column: 1,
-              line: 7,
-              offset: 103,
-            },
+            start: { column: 1, line: 7, offset: 103 },
+            end: { column: 35, line: 7, offset: 137 },
           },
         },
         message: 'Invalid version number: REPLACEME',