From 246cc3428b9c1f57a8f795d4cb0ebc9db01fbe11 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Mon, 24 Mar 2025 18:45:47 -0400 Subject: [PATCH 1/3] chore(lint): report error on invalid change versions, except when REPLACEME --- src/linter/rules/invalid-change-version.mjs | 66 +++++++++---------- src/linter/tests/fixtures/entries.mjs | 5 ++ .../invalidChangeVersion-environment.mjs | 15 +++++ .../rules/invalid-change-version.test.mjs | 56 +++++++++++----- 4 files changed, 90 insertions(+), 52 deletions(-) create mode 100644 src/linter/tests/fixtures/invalidChangeVersion-environment.mjs diff --git a/src/linter/rules/invalid-change-version.mjs b/src/linter/rules/invalid-change-version.mjs index 465e28a..fdaefa4 100644 --- a/src/linter/rules/invalid-change-version.mjs +++ b/src/linter/rules/invalid-change-version.mjs @@ -1,42 +1,38 @@ 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 + * Determines if a given version is invalid. * - * @param {ApiDocMetadataEntry[]} entries - * @returns {Array} + * @param {string} version - The version to check. + * @returns {boolean} True if the version is invalid, false otherwise. */ -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 invalidVersions = allVersions.filter( - version => valid(version) === null - ); +const isInvalid = NODE_RELEASED_VERSIONS + ? version => + version !== 'REPLACEME' && !NODE_RELEASED_VERSIONS.includes(version) + : version => version !== 'REPLACEME' && !valid(version); - issues.push( - ...invalidVersions.map(version => ({ - level: 'warn', - message: LINT_MESSAGES.invalidChangeVersion.replace( - '{{version}}', - version - ), - location: { - path: entry.api_doc_source, - position: entry.yaml_position, - }, - })) - ); - } - - return issues; -}; +/** + * Checks if any change version is invalid. + * + * @param {ApiDocMetadataEntry[]} entries - The metadata entries to check. + * @returns {Array} List of lint issues found. + */ +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 }, + })) + ) + ); diff --git a/src/linter/tests/fixtures/entries.mjs b/src/linter/tests/fixtures/entries.mjs index 89f717c..6b40892 100644 --- a/src/linter/tests/fixtures/entries.mjs +++ b/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', diff --git a/src/linter/tests/fixtures/invalidChangeVersion-environment.mjs b/src/linter/tests/fixtures/invalidChangeVersion-environment.mjs new file mode 100644 index 0000000..6201912 --- /dev/null +++ b/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, []); diff --git a/src/linter/tests/rules/invalid-change-version.test.mjs b/src/linter/tests/rules/invalid-change-version.test.mjs index 561e055..0bcbc1e 100644 --- a/src/linter/tests/rules/invalid-change-version.test.mjs +++ b/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: [ + 'v9.9.0', + 'v13.9.0', + 'v12.16.2', + 'v15.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,30 +46,22 @@ describe('invalidChangeVersion', () => { ...assertEntry, changes: [ ...assertEntry.changes, - { version: ['v13.9.0', 'REPLACEME'] }, + { version: ['v13.9.0', 'INVALID_VERSION'] }, ], }, ]); 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', + message: 'Invalid version number: INVALID_VERSION', }, ]); }); From fba5ee2c050498897f9b4a52e6b61644e65c6974 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Tue, 25 Mar 2025 11:17:43 -0400 Subject: [PATCH 2/3] don't assume`v` in NODE_RELEASED_VERSIONS --- src/linter/rules/invalid-change-version.mjs | 3 ++- src/linter/tests/rules/invalid-change-version.test.mjs | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/linter/rules/invalid-change-version.mjs b/src/linter/rules/invalid-change-version.mjs index fdaefa4..c3e88b5 100644 --- a/src/linter/rules/invalid-change-version.mjs +++ b/src/linter/rules/invalid-change-version.mjs @@ -12,7 +12,8 @@ const NODE_RELEASED_VERSIONS = env.NODE_RELEASED_VERSIONS?.split(','); */ const isInvalid = NODE_RELEASED_VERSIONS ? version => - version !== 'REPLACEME' && !NODE_RELEASED_VERSIONS.includes(version) + version !== 'REPLACEME' && + !NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, '')) : version => version !== 'REPLACEME' && !valid(version); /** diff --git a/src/linter/tests/rules/invalid-change-version.test.mjs b/src/linter/tests/rules/invalid-change-version.test.mjs index 0bcbc1e..75d7aac 100644 --- a/src/linter/tests/rules/invalid-change-version.test.mjs +++ b/src/linter/tests/rules/invalid-change-version.test.mjs @@ -21,10 +21,10 @@ describe('invalidChangeVersion', () => { { env: { NODE_RELEASED_VERSIONS: [ - 'v9.9.0', - 'v13.9.0', - 'v12.16.2', - 'v15.0.0', + '9.9.0', + '13.9.0', + '12.16.2', + '15.0.0', 'REPLACEME', 'SOME_OTHER_RELEASED_VERSION', ].join(','), From 5b348835d48c5fbc95a60257fb87084bcc7ecbdd Mon Sep 17 00:00:00 2001 From: avivkeller Date: Tue, 25 Mar 2025 13:08:44 -0400 Subject: [PATCH 3/3] code review --- src/linter/rules/invalid-change-version.mjs | 31 ++++++++++++++----- .../rules/invalid-change-version.test.mjs | 26 ++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/linter/rules/invalid-change-version.mjs b/src/linter/rules/invalid-change-version.mjs index c3e88b5..c11b247 100644 --- a/src/linter/rules/invalid-change-version.mjs +++ b/src/linter/rules/invalid-change-version.mjs @@ -4,23 +4,38 @@ import { env } from 'node:process'; const NODE_RELEASED_VERSIONS = env.NODE_RELEASED_VERSIONS?.split(','); +/** + * Checks if the given version is "REPLACEME" and the array length is 1. + * + * @param {string} version - The version to check. + * @param {number} length - Length of the version array. + * @returns {boolean} True if conditions match, otherwise false. + */ +const isValidReplaceMe = (version, length) => + length === 1 && version === 'REPLACEME'; + /** * Determines if a given version is invalid. * * @param {string} version - The version to check. - * @returns {boolean} True if the version is invalid, false otherwise. + * @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 => - version !== 'REPLACEME' && - !NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, '')) - : version => version !== 'REPLACEME' && !valid(version); + ? (version, _, { length }) => + !( + isValidReplaceMe(version, length) || + NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, '')) + ) + : (version, _, { length }) => + !(isValidReplaceMe(version, length) || valid(version)); /** - * Checks if any change version is invalid. + * Identifies invalid change versions from metadata entries. * - * @param {ApiDocMetadataEntry[]} entries - The metadata entries to check. - * @returns {Array} List of lint issues found. + * @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 }) => diff --git a/src/linter/tests/rules/invalid-change-version.test.mjs b/src/linter/tests/rules/invalid-change-version.test.mjs index 75d7aac..6ac0fed 100644 --- a/src/linter/tests/rules/invalid-change-version.test.mjs +++ b/src/linter/tests/rules/invalid-change-version.test.mjs @@ -65,4 +65,30 @@ describe('invalidChangeVersion', () => { }, ]); }); + + 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: '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: REPLACEME', + }, + ]); + }); });