Skip to content

linter: add deprecation code check, linter declarations #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/cli.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ const {
const linter = createLinter(lintDryRun, disableRule);

const { loadFiles } = createMarkdownLoader();
const { parseApiDocs } = createMarkdownParser();
const { parseApiDocs } = createMarkdownParser(linter);

const apiDocFiles = await loadFiles(input, ignore);

Expand Down
5 changes: 5 additions & 0 deletions src/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -425,4 +425,9 @@ export const LINT_MESSAGES = {
missingIntroducedIn: "Missing 'introduced_in' field in the API doc entry",
missingChangeVersion: 'Missing version field in the API doc entry',
invalidChangeVersion: 'Invalid version number: {{version}}',
malformedDeprecationHeader: 'Malformed deprecation header',
outOfOrderDeprecationCode:
"Deprecation code '{{code}}' out of order (expected {{expectedCode}})",
invalidLinterDeclaration: "Invalid linter declaration '{{declaration}}'",
malformedLinterDeclaration: 'Malformed linter declaration: {{message}}',
};
2 changes: 1 addition & 1 deletion src/generators/legacy-json/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export interface HierarchizedEntry extends ApiDocMetadataEntry {
/**
* List of child entries that are part of this entry's hierarchy.
*/
hierarchyChildren: ApiDocMetadataEntry[];
hierarchyChildren: HierarchizedEntry[];
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/generators/legacy-json/utils/buildSection.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { buildHierarchy } from './buildHierarchy.mjs';
import { buildHierarchy } from '../../../utils/buildHierarchy.mjs';
import { getRemarkRehype } from '../../../utils/remark.mjs';
import { transformNodesToString } from '../../../utils/unist.mjs';
import { parseList } from './parseList.mjs';
Expand Down
6 changes: 6 additions & 0 deletions src/linter/constants.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

// Validates a deprecation header from doc/api/deprecation.md and captures the
// code
// For example, `DEP0001: `http.OutgoingMessage.prototype.flush` captures `0001`
export const DEPRECATION_HEADER_REGEX = /DEP(\d{4}): .+?/;
18 changes: 13 additions & 5 deletions src/linter/engine.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ const createLinterEngine = rules => {
* Validates a ApiDocMetadataEntry entry against all defined rules
*
* @param {ApiDocMetadataEntry} entry
* @param {import('./types').LintDeclarations}
* @param declarations
* @returns {import('./types').LintIssue[]}
*/
const lint = entry => {
const lint = (entry, declarations) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declarations kinda feel retrofitted in but not sure if there's a better way to pass them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think declarations could be part of metadata entries

const issues = [];

for (const rule of rules) {
const ruleIssues = rule(entry);
const ruleIssues = rule([entry], declarations);

if (ruleIssues.length > 0) {
issues.push(...ruleIssues);
Expand All @@ -30,13 +32,19 @@ const createLinterEngine = rules => {
* Validates an array of ApiDocMetadataEntry entries against all defined rules
*
* @param {ApiDocMetadataEntry[]} entries
* @param {import('./types').LintDeclarations}
* @param declarations
* @returns {import('./types').LintIssue[]}
*/
const lintAll = entries => {
const lintAll = (entries, declarations) => {
const issues = [];

for (const entry of entries) {
issues.push(...lint(entry));
for (const rule of rules) {
const ruleIssues = rule(entries, declarations);

if (ruleIssues.length > 0) {
issues.push(...ruleIssues);
}
}

return issues;
Expand Down
92 changes: 91 additions & 1 deletion src/linter/index.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

import { LINT_MESSAGES } from '../constants.mjs';
import createLinterEngine from './engine.mjs';
import reporters from './reporters/index.mjs';
import rules from './rules/index.mjs';
Expand All @@ -22,6 +23,13 @@ const createLinter = (dryRun, disabledRules) => {
.map(([, rule]) => rule);
};

/**
* @type {import('./types').LintDeclarations}
*/
const declarations = {
skipDeprecation: [],
};

const engine = createLinterEngine(getEnabledRules(disabledRules));

/**
Expand All @@ -37,7 +45,7 @@ const createLinter = (dryRun, disabledRules) => {
* @param entries
*/
const lintAll = entries => {
issues.push(...engine.lintAll(entries));
issues.push(...engine.lintAll(entries, declarations));
};

/**
Expand All @@ -58,6 +66,87 @@ const createLinter = (dryRun, disabledRules) => {
}
};

/**
* Parse an inline-declaration found in the markdown input
*
* @param {string} declaration
*/
const parseLinterDeclaration = declaration => {
// Trim off any excess spaces from the beginning & end
declaration = declaration.trim();

// Extract the name for the declaration
const [name, ...value] = declaration.split(' ');

switch (name) {
case 'skip-deprecation': {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt of defining these definitions similarly to the rules & reporters? (i.e. have them defined in a declarations/skip-deprecation.mjs, then here we look them up & call them if they exist)

if (value.length !== 1) {
issues.push({
level: 'error',
location: {
// TODO,
path: '',
position: 0,
},
message: LINT_MESSAGES.malformedLinterDeclaration.replace(
'{{message}}',
`Expected 1 argument, got ${value.length}`
),
});

break;
}

// Get the deprecation code. This should be something like DEP0001.
const deprecation = value[0];

// Extract the number from the code
const deprecationCode = Number(deprecation.substring('DEP'.length));

// Make sure this is a valid deprecation code, output an error otherwise
if (
deprecation.length !== 7 ||
!deprecation.startsWith('DEP') ||
isNaN(deprecationCode)
) {
issues.push({
level: 'error',
location: {
// TODO,
path: '',
position: 0,
},
message: LINT_MESSAGES.malformedLinterDeclaration.replace(
'{{message}}',
`Invalid deprecation code ${deprecation}`
),
});

break;
}

declarations.skipDeprecation.push(deprecationCode);

break;
}
default: {
issues.push({
level: 'error',
location: {
// TODO
path: '',
position: 0,
},
message: LINT_MESSAGES.invalidLinterDeclaration.replace(
'{{declaration}}',
name
),
});
break;
}
}
};

/**
* Checks if any error-level issues were found during linting
*
Expand All @@ -70,6 +159,7 @@ const createLinter = (dryRun, disabledRules) => {
return {
lintAll,
report,
parseLinterDeclaration,
hasError,
};
};
Expand Down
82 changes: 82 additions & 0 deletions src/linter/rules/deprecation-code-order.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
'use strict';

import { LINT_MESSAGES } from '../../constants.mjs';
import { buildHierarchy } from '../../utils/buildHierarchy.mjs';
import { DEPRECATION_HEADER_REGEX } from '../constants.mjs';
import getDeprecationEntries from './utils/getDeprecationEntries.mjs';

/**
* @param {ApiDocMetadataEntry} deprecation
* @param {number} expectedCode
* @returns {Array<import('../types').LintIssue>}
*/
function lintDeprecation(deprecation, expectedCode) {
// Try validating the header (`DEPXXXX: ...`) and extract the code for us to
// look at
const match = deprecation.heading.data.text.match(DEPRECATION_HEADER_REGEX);

if (!match) {
// Malformed header
return [
{
level: 'error',
location: {
path: deprecation.api_doc_source,
position: deprecation.yaml_position,
},
message: LINT_MESSAGES.malformedDeprecationHeader,
},
];
}

const code = Number(match[1]);

return code === expectedCode
? []
: [
{
level: 'error',
location: {
path: deprecation.api_doc_source,
position: deprecation.yaml_position,
},
message: LINT_MESSAGES.outOfOrderDeprecationCode
.replaceAll('{{code}}', match[1])
.replace('{{expectedCode}}', `${expectedCode}`.padStart(4, '0')),
},
];
}

/**
* Checks if any deprecation codes are out of order
*
* @type {import('../types').LintRule}
*/
export const deprecationCodeOrder = (entries, declarations) => {
if (entries.length === 0 || entries[0].api !== 'deprecations') {
// This is only relevant to doc/api/deprecations.md
return [];
}

const issues = [];

const hierarchy = buildHierarchy(entries);

hierarchy.forEach(root => {
const deprecations = getDeprecationEntries(root.hierarchyChildren);

let expectedCode = 1;

for (const deprecation of deprecations || []) {
while (declarations.skipDeprecation.includes(expectedCode)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably a nicer way to do this loop

expectedCode++;
}

issues.push(...lintDeprecation(deprecation, expectedCode));

expectedCode++;
}
});

return issues;
};
2 changes: 2 additions & 0 deletions src/linter/rules/index.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

import { deprecationCodeOrder } from './deprecation-code-order.mjs';
import { invalidChangeVersion } from './invalid-change-version.mjs';
import { missingChangeVersion } from './missing-change-version.mjs';
import { missingIntroducedIn } from './missing-introduced-in.mjs';
Expand All @@ -11,4 +12,5 @@ export default {
'invalid-change-version': invalidChangeVersion,
'missing-change-version': missingChangeVersion,
'missing-introduced-in': missingIntroducedIn,
'deprecation-code-order': deprecationCodeOrder,
};
2 changes: 2 additions & 0 deletions src/linter/rules/invalid-change-version.mjs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict';

import { LINT_MESSAGES } from '../../constants.mjs';
import { valid } from 'semver';

Expand Down
2 changes: 2 additions & 0 deletions src/linter/rules/missing-change-version.mjs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict';

/**
* Checks if any change version is missing
*
Expand Down
2 changes: 2 additions & 0 deletions src/linter/rules/missing-introduced-in.mjs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict';

import { LINT_MESSAGES } from '../../constants.mjs';

/**
Expand Down
15 changes: 15 additions & 0 deletions src/linter/rules/utils/getDeprecationEntries.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

/**
* @param {Array<import('../../../generators/legacy-json/types').HierarchizedEntry>} hierarchy
* @returns {Array<import('../../../generators/legacy-json/types').HierarchizedEntry> | undefined}
*/
export default function getDeprecationEntries(hierarchy) {
for (const child of hierarchy) {
if (child.slug === 'list-of-deprecated-apis') {
return child.hierarchyChildren;
}
}

return undefined;
}
9 changes: 8 additions & 1 deletion src/linter/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ export interface LintIssue {
location: LintIssueLocation;
}

type LintRule = (input: ApiDocMetadataEntry) => LintIssue[];
export interface LintDeclarations {
skipDeprecation: Array<number>;
}

type LintRule = (
input: Array<ApiDocMetadataEntry>,
declarations: LintDeclarations
) => LintIssue[];

export type Reporter = (msg: LintIssue) => void;
15 changes: 13 additions & 2 deletions src/parsers/markdown.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import { createNodeSlugger } from '../utils/slugger.mjs';
/**
* Creates an API doc parser for a given Markdown API doc file
*
* @param {import('./linter/index.mjs').Linter | undefined} linter
* @param {ReturnType<import('../linter/index.mjs').default> | undefined} linter
*/
const createParser = () => {
const createParser = linter => {
// Creates an instance of the Remark processor with GFM support
// which is used for stringifying the AST tree back to Markdown
const remarkProcessor = getRemark();
Expand Down Expand Up @@ -142,6 +142,16 @@ const createParser = () => {
addYAMLMetadata(node, apiEntryMetadata);
});

// Visits all HTML nodes from the current subtree to check for linter declarations.
// If there are, it gives them to the linter to parse and use.
visit(subTree, createQueries.UNIST.isLinterComment, node => {
if (linter) {
linter.parseLinterDeclaration(
node.value.match(createQueries.QUERIES.linterComment)[1]
);
}
});

// Visits all Text nodes from the current subtree and if there's any that matches
// any API doc type reference and then updates the type reference to be a Markdown link
visit(subTree, createQueries.UNIST.isTextWithType, (node, _, parent) =>
Expand All @@ -150,6 +160,7 @@ const createParser = () => {

// Removes already parsed items from the subtree so that they aren't included in the final content
remove(subTree, [createQueries.UNIST.isYamlNode]);
remove(subTree, [createQueries.UNIST.isLinterComment]);

// Applies the AST transformations to the subtree based on the API doc entry Metadata
// Note that running the transformation on the subtree isn't costly as it is a reduced tree
Expand Down
Loading
Loading