feat: improve changes issues positions

Author: araujogui

src/linter/constants.mjs

@@ -6,6 +6,7 @@ export const LLM_DESCRIPTION_REGEX = /<!--\s?llm_description=.*-->/;
 
 export const LINT_MESSAGES = {
   missingIntroducedIn: "Missing 'introduced_in' field in the API doc entry",
+  invalidChangeProperty: 'Invalid change property type: {{type}}',
   missingChangeVersion: 'Missing version field in the API doc entry',
   invalidChangeVersion: 'Invalid version number: {{version}}',
   duplicateStabilityNode: 'Duplicate stability node',

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

@@ -44,10 +44,7 @@ changes:
 <!-- YAML
 changes:
   - version:
-      - v15.7.0
-      - v14.18.0
-  - version: v6.4.0
-  - version:
+  - pr-url: https://github.com/nodejs/node/pull/1
 -->`;
 
     const context = {
@@ -70,17 +67,30 @@ changes:
 
     invalidChangeVersion(context);
 
-    strictEqual(context.report.mock.callCount(), 1);
+    strictEqual(context.report.mock.callCount(), 2);
 
-    const call = context.report.mock.calls[0];
+    const first_call = context.report.mock.calls[0];
 
-    deepStrictEqual(call.arguments, [
+    deepStrictEqual(first_call.arguments, [
+      {
+        level: 'error',
+        message: 'Missing version field in the API doc entry',
+        position: {
+          start: { line: 4 },
+          end: { line: 4 },
+        },
+      },
+    ]);
+
+    const second_call = context.report.mock.calls[1];
+
+    deepStrictEqual(second_call.arguments, [
       {
         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: 3 },
+          end: { line: 3 },
         },
       },
     ]);
@@ -182,8 +192,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 },
         },
       },
     ]);
@@ -226,8 +236,91 @@ 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`
+<!-- YAML
+changes:
+  abc:
+  def:
+-->`;
+
+    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: PLAIN',
+        position: {
+          start: { line: 8 },
+          end: { line: 8 },
+        },
+      },
+    ]);
+  });
+
+  it('should report an issue if version is not a mapping', () => {
+    const yamlContent = dedent`
+<!-- YAML
+changes:
+  version:
+    - abc
+    - def
+-->`;
+
+    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: PLAIN',
+        position: {
+          start: { line: 8 },
+          end: { line: 8 },
         },
       },
     ]);

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,50 @@ const isIgnoredVersion = version => {
 /**
  * Determines if a given version is invalid.
  *
- * @param {string} version - The version to check.
+ * @param {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 }) =>
       !(
-        isValidReplaceMe(version, length) ||
-        isIgnoredVersion(version) ||
-        NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, ''))
+        isValidReplaceMe(version.value, length) ||
+        isIgnoredVersion(version.value) ||
+        NODE_RELEASED_VERSIONS.includes(version.value.replace(/^v/, ''))
       )
   : (version, _, { length }) =>
-      !(isValidReplaceMe(version, length) || valid(version));
+      !(isValidReplaceMe(version.value, length) || valid(version.value));
+
+/**
+ *
+ * @param {object} root0
+ * @param {import('../types.d.ts').LintContext} root0.context
+ * @param {import('yaml').Node} root0.node
+ * @param {(message: string, node: import('yaml').Node<unknown>) => import('../types.d.ts').IssueDescriptor} root0.report
+ */
+export const extractVersions = ({ context, node, report }) => {
+  if (!isMap(node)) {
+    context.report(
+      report(
+        LINT_MESSAGES.invalidChangeProperty.replace('{{type}}', node.NODE_TYPE),
+        node
+      )
+    );
+
+    return null;
+  }
+
+  const versionNode = findPropertyByName(node, 'version');
+
+  if (!versionNode) {
+    context.report(report(LINT_MESSAGES.missingChangeVersion, node));
+
+    return null;
+  }
+
+  return normalizeNode(versionNode.value);
+};
 
 /**
  * Identifies invalid change versions from metadata entries.
@@ -69,30 +103,52 @@ 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 });
 
-    if (!changes) {
+    const report = createYamlIssueReporter(node, lineCounter);
+
+    // 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(
+        report(
+          LINT_MESSAGES.invalidChangeProperty.replace(
+            '{{type}}',
+            changesNode.value.NODE_TYPE
+          ),
+          changesNode.key
+        )
+      );
+    }
+
+    changesNode.value.items.forEach(node =>
+      extractVersions({ context, node, report })
+        .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(
+            report(
+              version?.value
+                ? LINT_MESSAGES.invalidChangeVersion.replace(
+                    '{{version}}',
+                    version.value
+                  )
+                : LINT_MESSAGES.missingChangeVersion,
+              version
+            )
+          )
         )
     );
   });

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;