diff --git a/src/linter/constants.mjs b/src/linter/constants.mjs index 934b32d2..ab0edd2b 100644 --- a/src/linter/constants.mjs +++ b/src/linter/constants.mjs @@ -6,6 +6,7 @@ export const LLM_DESCRIPTION_REGEX = //; export const LINT_MESSAGES = { missingIntroducedIn: "Missing 'introduced_in' field in the API doc entry", + invalidChangeProperty: 'Invalid change property type', missingChangeVersion: 'Missing version field in the API doc entry', invalidChangeVersion: 'Invalid version number: {{version}}', duplicateStabilityNode: 'Duplicate stability node', diff --git a/src/linter/rules/__tests__/invalid-change-version.test.mjs b/src/linter/rules/__tests__/invalid-change-version.test.mjs index bdae5c40..5df682f6 100644 --- a/src/linter/rules/__tests__/invalid-change-version.test.mjs +++ b/src/linter/rules/__tests__/invalid-change-version.test.mjs @@ -11,14 +11,14 @@ import { invalidChangeVersion } from '../invalid-change-version.mjs'; describe('invalidChangeVersion', () => { it('should not report if all change versions are non-empty', () => { const yamlContent = dedent` -`; + `; const context = { tree: { @@ -41,14 +41,11 @@ changes: it('should report an issue if a change version is missing', () => { const yamlContent = dedent` -`; + `; const context = { tree: { @@ -70,17 +67,27 @@ changes: invalidChangeVersion(context); - strictEqual(context.report.mock.callCount(), 1); + strictEqual(context.report.mock.callCount(), 2); - const call = context.report.mock.calls[0]; + const callArguments = context.report.mock.calls.flatMap( + call => call.arguments + ); - deepStrictEqual(call.arguments, [ + deepStrictEqual(callArguments, [ + { + level: 'error', + message: 'Missing version field in the API doc entry', + position: { + start: { line: 3 }, + end: { line: 3 }, + }, + }, { level: 'error', message: 'Missing version field in the API doc entry', position: { - start: { line: 1, column: 1, offset: 1 }, - end: { line: 1, column: 1, offset: 1 }, + start: { line: 4 }, + end: { line: 4 }, }, }, ]); @@ -117,14 +124,14 @@ changes: it('should not report if all change versions are valid', () => { const yamlContent = dedent` -`; + `; const context = { tree: { @@ -147,14 +154,14 @@ changes: it('should report an issue if a change version is invalid', () => { const yamlContent = dedent` -`; + `; const context = { tree: { @@ -182,8 +189,8 @@ changes: level: 'error', message: 'Invalid version number: INVALID_VERSION', position: { - start: { column: 1, line: 7, offset: 103 }, - end: { column: 35, line: 7, offset: 137 }, + start: { line: 11 }, + end: { line: 11 }, }, }, ]); @@ -191,14 +198,14 @@ changes: it('should report an issue if a change version contains a REPLACEME and a version', () => { const yamlContent = dedent` -`; + `; const context = { tree: { @@ -226,10 +233,150 @@ changes: level: 'error', message: 'Invalid version number: REPLACEME', position: { - start: { column: 1, line: 7, offset: 103 }, - end: { column: 35, line: 7, offset: 137 }, + start: { line: 11 }, + end: { line: 11 }, + }, + }, + ]); + }); + + it('should report an issue if changes is not a sequence', () => { + const yamlContent = dedent` + `; + + const context = { + tree: { + type: 'root', + children: [ + { + type: 'html', + value: yamlContent, + position: { + start: { column: 1, line: 7, offset: 103 }, + end: { column: 35, line: 7, offset: 137 }, + }, + }, + ], + }, + report: mock.fn(), + getIssues: mock.fn(), + }; + + invalidChangeVersion(context); + strictEqual(context.report.mock.callCount(), 1); + const call = context.report.mock.calls[0]; + deepStrictEqual(call.arguments, [ + { + level: 'error', + message: 'Invalid change property type', + position: { + start: { line: 8 }, + end: { line: 8 }, + }, + }, + ]); + }); + + it('should report an issue if version is not a mapping', () => { + const yamlContent = dedent` + `; + + const context = { + tree: { + type: 'root', + children: [ + { + type: 'html', + value: yamlContent, + position: { + start: { column: 1, line: 7, offset: 103 }, + end: { column: 35, line: 7, offset: 137 }, + }, + }, + ], + }, + report: mock.fn(), + getIssues: mock.fn(), + }; + + invalidChangeVersion(context); + strictEqual(context.report.mock.callCount(), 1); + const call = context.report.mock.calls[0]; + deepStrictEqual(call.arguments, [ + { + level: 'error', + message: 'Invalid change property type', + position: { + start: { line: 8 }, + end: { line: 8 }, }, }, ]); }); + + it("should skip validations if yaml root node isn't a mapping", () => { + const yamlContent = dedent` + `; + + const context = { + tree: { + type: 'root', + children: [ + { + type: 'html', + value: yamlContent, + position: { + start: { column: 1, line: 7, offset: 103 }, + end: { column: 35, line: 7, offset: 137 }, + }, + }, + ], + }, + report: mock.fn(), + getIssues: mock.fn(), + }; + + invalidChangeVersion(context); + strictEqual(context.report.mock.callCount(), 0); + }); + + it('should skip validations if changes node is missing', () => { + const yamlContent = dedent` + `; + + const context = { + tree: { + type: 'root', + children: [ + { + type: 'html', + value: yamlContent, + position: { + start: { column: 1, line: 7, offset: 103 }, + end: { column: 35, line: 7, offset: 137 }, + }, + }, + ], + }, + report: mock.fn(), + getIssues: mock.fn(), + }; + + invalidChangeVersion(context); + strictEqual(context.report.mock.callCount(), 0); + }); }); diff --git a/src/linter/rules/invalid-change-version.mjs b/src/linter/rules/invalid-change-version.mjs index d5f980c7..47a54f5c 100644 --- a/src/linter/rules/invalid-change-version.mjs +++ b/src/linter/rules/invalid-change-version.mjs @@ -2,15 +2,19 @@ import { env } from 'node:process'; import { valid, parse } from 'semver'; import { visit } from 'unist-util-visit'; -import yaml from 'yaml'; +import { isMap, isSeq, LineCounter, parseDocument } from 'yaml'; -import { enforceArray } from '../../utils/array.mjs'; import { extractYamlContent, normalizeYamlSyntax, } from '../../utils/parser/index.mjs'; import createQueries from '../../utils/queries/index.mjs'; import { LINT_MESSAGES } from '../constants.mjs'; +import { + createYamlIssueReporter, + findPropertyByName, + normalizeNode, +} from '../utils/yaml.mjs'; const NODE_RELEASED_VERSIONS = env.NODE_RELEASED_VERSIONS?.split(','); @@ -42,20 +46,46 @@ const isIgnoredVersion = version => { /** * Determines if a given version is invalid. * - * @param {string} version - The version to check. + * @param {import('yaml').Scalar} 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 }) => + ? ({ value }, _, { length }) => !( - isValidReplaceMe(version, length) || - isIgnoredVersion(version) || - NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, '')) + isValidReplaceMe(value, length) || + isIgnoredVersion(value) || + NODE_RELEASED_VERSIONS.includes(value.replace(/^v/, '')) ) - : (version, _, { length }) => - !(isValidReplaceMe(version, length) || valid(version)); + : ({ value }, _, { length }) => + !(isValidReplaceMe(value, length) || valid(value)); + +/** + * Validates and extracts versions of a change node + * + * @param {object} root0 + * @param {import('../types.d.ts').LintContext} root0.context + * @param {import('yaml').Node} root0.node + * @param {(message: string, node: import('yaml').Node) => import('../types.d.ts').IssueDescriptor} root0.createYamlIssue + */ +export const extractVersions = ({ context, node, createYamlIssue }) => { + if (!isMap(node)) { + context.report(createYamlIssue(LINT_MESSAGES.invalidChangeProperty, node)); + + return []; + } + + const versionNode = findPropertyByName(node, 'version'); + + if (!versionNode) { + context.report(createYamlIssue(LINT_MESSAGES.missingChangeVersion, node)); + + return []; + } + + return normalizeNode(versionNode.value); +}; /** * Identifies invalid change versions from metadata entries. @@ -69,31 +99,47 @@ export const invalidChangeVersion = context => { const normalizedYaml = normalizeYamlSyntax(yamlContent); - // TODO: Use YAML AST to provide better issues positions - /** - * @type {ApiDocRawMetadataEntry} - */ - const { changes } = yaml.parse(normalizedYaml); + const lineCounter = new LineCounter(); + const document = parseDocument(normalizedYaml, { lineCounter }); + + const createYamlIssue = createYamlIssueReporter(node, lineCounter); - if (!changes) { + // Skip if yaml isn't a mapping + if (!isMap(document.contents)) { return; } - changes.forEach(({ version }) => - enforceArray(version) + const changesNode = findPropertyByName(document.contents, 'changes'); + + // Skip if changes node is not present + if (!changesNode) { + return; + } + + // Validate changes node is a sequence + if (!isSeq(changesNode.value)) { + return context.report( + createYamlIssue(LINT_MESSAGES.invalidChangeProperty, changesNode.key) + ); + } + + changesNode.value.items.forEach(node => { + extractVersions({ context, node, createYamlIssue }) + .filter(Boolean) // Filter already reported empt items, .filter(isInvalid) .forEach(version => - context.report({ - level: 'error', - message: version - ? LINT_MESSAGES.invalidChangeVersion.replace( - '{{version}}', - version - ) - : LINT_MESSAGES.missingChangeVersion, - position: node.position, - }) - ) - ); + context.report( + createYamlIssue( + version?.value + ? LINT_MESSAGES.invalidChangeVersion.replace( + '{{version}}', + version.value + ) + : LINT_MESSAGES.missingChangeVersion, + version + ) + ) + ); + }); }); }; diff --git a/src/linter/types.d.ts b/src/linter/types.d.ts index 2c328e17..41e64cf9 100644 --- a/src/linter/types.d.ts +++ b/src/linter/types.d.ts @@ -1,10 +1,14 @@ import { Root } from 'mdast'; -import { Position } from 'unist'; import reporters from './reporters/index.mjs'; import { VFile } from 'vfile'; export type IssueLevel = 'info' | 'warn' | 'error'; +export interface Position { + start: { line: number }; + end: { line: number }; +} + export interface LintIssueLocation { path: string; // The absolute path to the file position?: Position; diff --git a/src/linter/utils/__tests__/yaml.mjs b/src/linter/utils/__tests__/yaml.mjs new file mode 100644 index 00000000..ec715053 --- /dev/null +++ b/src/linter/utils/__tests__/yaml.mjs @@ -0,0 +1,99 @@ +import { deepStrictEqual, strictEqual, throws } from 'node:assert'; +import { describe, it } from 'node:test'; + +import { Scalar, Pair, YAMLSeq, YAMLMap } from 'yaml'; + +import { findPropertyByName, normalizeNode } from '../yaml.mjs'; + +describe('yaml', () => { + describe('findPropertyByName', () => { + it('should find a property by name when it exists', () => { + const mockMap = { + items: [ + new Pair(new Scalar('propertyA'), new Scalar('valueA')), + new Pair(new Scalar('propertyB'), new Scalar('valueB')), + ], + }; + + const result = findPropertyByName(mockMap, 'propertyA'); + + strictEqual(result.key.value, 'propertyA'); + strictEqual(result.value.value, 'valueA'); + }); + + it('should return undefined when property does not exist', () => { + const mockMap = { + items: [ + new Pair(new Scalar('propertyA'), new Scalar('valueA')), + new Pair(new Scalar('propertyB'), new Scalar('valueB')), + ], + }; + + const result = findPropertyByName(mockMap, 'nonExistent'); + strictEqual(result, undefined); + }); + }); + + describe('normalizeNode', () => { + it('should normalize a scalar node', () => { + const scalar = new Scalar('test-value'); + scalar.range = [0, 10, 10]; + + const result = normalizeNode(scalar); + + deepStrictEqual(result, [ + { + value: 'test-value', + range: [0, 10, 10], + }, + ]); + }); + + it('should normalize a sequence with scalar items', () => { + const item1 = new Scalar('first'); + item1.range = [0, 5, 5]; + + const item2 = new Scalar('second'); + item2.range = [6, 12, 12]; + + const sequence = new YAMLSeq(); + sequence.items = [item1, item2]; + + const result = normalizeNode(sequence); + + deepStrictEqual(result, [ + { value: 'first', range: [0, 5, 5] }, + { value: 'second', range: [6, 12, 12] }, + ]); + }); + + it('should normalize nested sequences', () => { + const innerItem = new Scalar('nested'); + innerItem.range = [0, 6, 6]; + + const innerSeq = new YAMLSeq(); + innerSeq.items = [innerItem]; + + const outerItem = new Scalar('outer'); + outerItem.range = [6, 12, 12]; + + const outerSeq = new YAMLSeq(); + outerSeq.items = [outerItem, innerSeq]; + + const result = normalizeNode(outerSeq); + + deepStrictEqual(result, [ + { value: 'outer', range: [6, 12, 12] }, + { value: 'nested', range: [0, 6, 6] }, + ]); + }); + + it('should throw error for map nodes', () => { + const mapNode = new YAMLMap(); + + throws(() => normalizeNode(mapNode), { + message: 'Unexpected node type: map', + }); + }); + }); +}); diff --git a/src/linter/utils/yaml.mjs b/src/linter/utils/yaml.mjs new file mode 100644 index 00000000..63df8ba8 --- /dev/null +++ b/src/linter/utils/yaml.mjs @@ -0,0 +1,77 @@ +// @ts-check + +'use strict'; + +import { isScalar, isSeq, isNode } from 'yaml'; + +/** + * Searches for a property by name in a map node. + * + * @param {import('yaml').YAMLMap} node - The map node to search in. + * @param {string} propertyName - The property name to search for. + * @returns {import('yaml').Pair | undefined} + */ +export const findPropertyByName = (node, propertyName) => { + return node.items.find(pair => { + if (!isScalar(pair.key)) { + return; + } + + return pair.key.value === propertyName; + }); +}; + +/** + * Normalizes a YAML node values into an array. + * + * @param {import('yaml').Node} node + * @returns {import('yaml').Scalar[]} + */ +export const normalizeNode = node => { + if (isScalar(node)) { + return [node]; + } + + if (isSeq(node)) { + // @ts-ignore + return node.items.flatMap(item => normalizeNode(item)); + } + + throw new Error(`Unexpected node type: map`); +}; + +/** + * Creates a factory function for generating error descriptors with proper line positioning + * for YAML nodes within markdown documents. + * + * @param {import('mdast').Node} yamlNode + * @param {import('yaml').LineCounter} lineCounter + */ +export const createYamlIssueReporter = (yamlNode, lineCounter) => { + const initialLine = yamlNode.position?.start.line ?? 0; + + /** + * @param {string} message + * @param {import('yaml').Node} node + * @returns {import('../types').IssueDescriptor} + */ + return (message, node) => { + const absoluteLine = + isNode(node) && node.range + ? initialLine + lineCounter.linePos(node.range[0]).line + : initialLine; + + return { + level: 'error', + message, + position: { + start: { + line: absoluteLine, + }, + end: { + line: absoluteLine, + }, + }, + }; + }; +}; diff --git a/src/utils/parser/index.mjs b/src/utils/parser/index.mjs index 5a6fc485..b47cf7af 100644 --- a/src/utils/parser/index.mjs +++ b/src/utils/parser/index.mjs @@ -16,7 +16,7 @@ import createQueries from '../queries/index.mjs'; /** * Extracts raw YAML content from a node * - * @param {import('mdast').Html} node A HTML node containing the YAML content + * @param {import('mdast').Node} node A HTML node containing the YAML content * @returns {string} The extracted raw YAML content */ export const extractYamlContent = node => {