Skip to content

Commit c317c35

Browse files
araujoguiaraujogui
committed
feat: improve changes issues positions
1 parent 6d86c0c commit c317c35

7 files changed

Lines changed: 370 additions & 40 deletions

File tree

‎src/linter/constants.mjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ export const LLM_DESCRIPTION_REGEX = /<!--\s?llm_description=.*-->/;
66

77
export const LINT_MESSAGES = {
88
missingIntroducedIn: "Missing 'introduced_in' field in the API doc entry",
9+
invalidChangeProperty: 'Invalid change property type: {{type}}',
910
missingChangeVersion: 'Missing version field in the API doc entry',
1011
invalidChangeVersion: 'Invalid version number: {{version}}',
1112
duplicateStabilityNode: 'Duplicate stability node',

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

Lines changed: 106 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,7 @@ changes:
4444
<!-- YAML
4545
changes:
4646
- version:
47-
- v15.7.0
48-
- v14.18.0
49-
- version: v6.4.0
50-
- version:
47+
- pr-url: https://github.com/nodejs/node/pull/1
5148
-->`;
5249

5350
const context = {
@@ -70,17 +67,30 @@ changes:
7067

7168
invalidChangeVersion(context);
7269

73-
strictEqual(context.report.mock.callCount(), 1);
70+
strictEqual(context.report.mock.callCount(), 2);
7471

75-
const call = context.report.mock.calls[0];
72+
const first_call = context.report.mock.calls[0];
7673

77-
deepStrictEqual(call.arguments, [
74+
deepStrictEqual(first_call.arguments, [
75+
{
76+
level: 'error',
77+
message: 'Missing version field in the API doc entry',
78+
position: {
79+
start: { line: 4 },
80+
end: { line: 4 },
81+
},
82+
},
83+
]);
84+
85+
const second_call = context.report.mock.calls[1];
86+
87+
deepStrictEqual(second_call.arguments, [
7888
{
7989
level: 'error',
8090
message: 'Missing version field in the API doc entry',
8191
position: {
82-
start: { line: 1, column: 1, offset: 1 },
83-
end: { line: 1, column: 1, offset: 1 },
92+
start: { line: 3 },
93+
end: { line: 3 },
8494
},
8595
},
8696
]);
@@ -182,8 +192,8 @@ changes:
182192
level: 'error',
183193
message: 'Invalid version number: INVALID_VERSION',
184194
position: {
185-
start: { column: 1, line: 7, offset: 103 },
186-
end: { column: 35, line: 7, offset: 137 },
195+
start: { line: 11 },
196+
end: { line: 11 },
187197
},
188198
},
189199
]);
@@ -226,8 +236,91 @@ changes:
226236
level: 'error',
227237
message: 'Invalid version number: REPLACEME',
228238
position: {
229-
start: { column: 1, line: 7, offset: 103 },
230-
end: { column: 35, line: 7, offset: 137 },
239+
start: { line: 11 },
240+
end: { line: 11 },
241+
},
242+
},
243+
]);
244+
});
245+
246+
it('should report an issue if changes is not a sequence', () => {
247+
const yamlContent = dedent`
248+
<!-- YAML
249+
changes:
250+
abc:
251+
def:
252+
-->`;
253+
254+
const context = {
255+
tree: {
256+
type: 'root',
257+
children: [
258+
{
259+
type: 'html',
260+
value: yamlContent,
261+
position: {
262+
start: { column: 1, line: 7, offset: 103 },
263+
end: { column: 35, line: 7, offset: 137 },
264+
},
265+
},
266+
],
267+
},
268+
report: mock.fn(),
269+
getIssues: mock.fn(),
270+
};
271+
272+
invalidChangeVersion(context);
273+
strictEqual(context.report.mock.callCount(), 1);
274+
const call = context.report.mock.calls[0];
275+
deepStrictEqual(call.arguments, [
276+
{
277+
level: 'error',
278+
message: 'Invalid change property type: PLAIN',
279+
position: {
280+
start: { line: 8 },
281+
end: { line: 8 },
282+
},
283+
},
284+
]);
285+
});
286+
287+
it('should report an issue if version is not a mapping', () => {
288+
const yamlContent = dedent`
289+
<!-- YAML
290+
changes:
291+
version:
292+
- abc
293+
- def
294+
-->`;
295+
296+
const context = {
297+
tree: {
298+
type: 'root',
299+
children: [
300+
{
301+
type: 'html',
302+
value: yamlContent,
303+
position: {
304+
start: { column: 1, line: 7, offset: 103 },
305+
end: { column: 35, line: 7, offset: 137 },
306+
},
307+
},
308+
],
309+
},
310+
report: mock.fn(),
311+
getIssues: mock.fn(),
312+
};
313+
314+
invalidChangeVersion(context);
315+
strictEqual(context.report.mock.callCount(), 1);
316+
const call = context.report.mock.calls[0];
317+
deepStrictEqual(call.arguments, [
318+
{
319+
level: 'error',
320+
message: 'Invalid change property type: PLAIN',
321+
position: {
322+
start: { line: 8 },
323+
end: { line: 8 },
231324
},
232325
},
233326
]);

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

Lines changed: 81 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,19 @@ import { env } from 'node:process';
22

33
import { valid, parse } from 'semver';
44
import { visit } from 'unist-util-visit';
5-
import yaml from 'yaml';
5+
import { isMap, isSeq, LineCounter, parseDocument } from 'yaml';
66

7-
import { enforceArray } from '../../utils/array.mjs';
87
import {
98
extractYamlContent,
109
normalizeYamlSyntax,
1110
} from '../../utils/parser/index.mjs';
1211
import createQueries from '../../utils/queries/index.mjs';
1312
import { LINT_MESSAGES } from '../constants.mjs';
13+
import {
14+
createYamlIssueReporter,
15+
findPropertyByName,
16+
normalizeNode,
17+
} from '../utils/yaml.mjs';
1418

1519
const NODE_RELEASED_VERSIONS = env.NODE_RELEASED_VERSIONS?.split(',');
1620

@@ -42,20 +46,50 @@ const isIgnoredVersion = version => {
4246
/**
4347
* Determines if a given version is invalid.
4448
*
45-
* @param {string} version - The version to check.
49+
* @param {Scalar} version - The version to check.
4650
* @param {unknown} _ - Unused parameter.
4751
* @param {{ length: number }} context - Array containing the length property.
4852
* @returns {boolean} True if the version is invalid, otherwise false.
4953
*/
5054
const isInvalid = NODE_RELEASED_VERSIONS
5155
? (version, _, { length }) =>
5256
!(
53-
isValidReplaceMe(version, length) ||
54-
isIgnoredVersion(version) ||
55-
NODE_RELEASED_VERSIONS.includes(version.replace(/^v/, ''))
57+
isValidReplaceMe(version.value, length) ||
58+
isIgnoredVersion(version.value) ||
59+
NODE_RELEASED_VERSIONS.includes(version.value.replace(/^v/, ''))
5660
)
5761
: (version, _, { length }) =>
58-
!(isValidReplaceMe(version, length) || valid(version));
62+
!(isValidReplaceMe(version.value, length) || valid(version.value));
63+
64+
/**
65+
*
66+
* @param {object} root0
67+
* @param {import('../types.d.ts').LintContext} root0.context
68+
* @param {import('yaml').Node} root0.node
69+
* @param {(message: string, node: import('yaml').Node<unknown>) => import('../types.d.ts').IssueDescriptor} root0.report
70+
*/
71+
export const extractVersions = ({ context, node, report }) => {
72+
if (!isMap(node)) {
73+
context.report(
74+
report(
75+
LINT_MESSAGES.invalidChangeProperty.replace('{{type}}', node.NODE_TYPE),
76+
node
77+
)
78+
);
79+
80+
return null;
81+
}
82+
83+
const versionNode = findPropertyByName(node, 'version');
84+
85+
if (!versionNode) {
86+
context.report(report(LINT_MESSAGES.missingChangeVersion, node));
87+
88+
return null;
89+
}
90+
91+
return normalizeNode(versionNode.value);
92+
};
5993

6094
/**
6195
* Identifies invalid change versions from metadata entries.
@@ -69,30 +103,52 @@ export const invalidChangeVersion = context => {
69103

70104
const normalizedYaml = normalizeYamlSyntax(yamlContent);
71105

72-
// TODO: Use YAML AST to provide better issues positions
73-
/**
74-
* @type {ApiDocRawMetadataEntry}
75-
*/
76-
const { changes } = yaml.parse(normalizedYaml);
106+
const lineCounter = new LineCounter();
107+
const document = parseDocument(normalizedYaml, { lineCounter });
77108

78-
if (!changes) {
109+
const report = createYamlIssueReporter(node, lineCounter);
110+
111+
// Skip if yaml isn't a mapping
112+
if (!isMap(document.contents)) {
79113
return;
80114
}
81115

82-
changes.forEach(({ version }) =>
83-
enforceArray(version)
116+
const changesNode = findPropertyByName(document.contents, 'changes');
117+
118+
// Skip if changes node is not present
119+
if (!changesNode) {
120+
return;
121+
}
122+
123+
// Validate changes node is a sequence
124+
if (!isSeq(changesNode.value)) {
125+
return context.report(
126+
report(
127+
LINT_MESSAGES.invalidChangeProperty.replace(
128+
'{{type}}',
129+
changesNode.value.NODE_TYPE
130+
),
131+
changesNode.key
132+
)
133+
);
134+
}
135+
136+
changesNode.value.items.forEach(node =>
137+
extractVersions({ context, node, report })
138+
.filter(Boolean) // Filter already reported empt items
84139
.filter(isInvalid)
85140
.forEach(version =>
86-
context.report({
87-
level: 'error',
88-
message: version
89-
? LINT_MESSAGES.invalidChangeVersion.replace(
90-
'{{version}}',
91-
version
92-
)
93-
: LINT_MESSAGES.missingChangeVersion,
94-
position: node.position,
95-
})
141+
context.report(
142+
report(
143+
version?.value
144+
? LINT_MESSAGES.invalidChangeVersion.replace(
145+
'{{version}}',
146+
version.value
147+
)
148+
: LINT_MESSAGES.missingChangeVersion,
149+
version
150+
)
151+
)
96152
)
97153
);
98154
});

‎src/linter/types.d.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import { Root } from 'mdast';
2-
import { Position } from 'unist';
32
import reporters from './reporters/index.mjs';
43
import { VFile } from 'vfile';
54

65
export type IssueLevel = 'info' | 'warn' | 'error';
76

7+
export interface Position {
8+
start: { line: number };
9+
end: { line: number };
10+
}
11+
812
export interface LintIssueLocation {
913
path: string; // The absolute path to the file
1014
position?: Position;

0 commit comments

Comments
 (0)