Skip to content

test: ensure that CLI options are alphabetical #55790

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.

Sign up for GitHub

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

Closed
avivkeller wants to merge 3 commits into from
Closed

test: ensure that CLI options are alphabetical #55790

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6369aa2
test: ensure that CLI options are alphabetical
avivkeller Nov 8, 2024
110545d
fixup! test: ensure that CLI options are alphabetical
avivkeller Nov 9, 2024
dd1d23e
fixup! fixup! test: ensure that CLI options are alphabetical
avivkeller Nov 9, 2024
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
Prev Previous commit
Next Next commit
fixup! test: ensure that CLI options are alphabetical
  • Loading branch information
@avivkeller
avivkeller committed Nov 9, 2024
commit 110545d520bd81e630af1e5a0d8f405be34599d0
3 changes: 2 additions & 1 deletion test/parallel/test-cli-node-options-docs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,10 @@ for (const [, envVar, config] of nodeOptionsCC.matchAll(addOptionRE)) {
const end = /^## Environment variables/m;
const filteredCLIText = cliText.slice(cliText.search(start), cliText.search(end)).trim();

@aduh95 aduh95 Nov 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The trimming is not useful, is it?

Suggested change
const filteredCLIText = cliText.slice(cliText.search(start), cliText.search(end)).trim();
const filteredCLIText = cliText.slice(cliText.search(start), cliText.search(end));

const cliOptionPattern = /^### `(--[a-zA-Z0-9-]+)`/mg;
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern)).map(match => match[1]);
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern)).map((match) => match[1]);

@aduh95 aduh95 Nov 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern)).map((match) => match[1]);
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern), (match) => match[1]);


const sortedOptions = [...options].sort();
/* eslint-disable no-restricted-syntax */
assert.deepStrictEqual(options, sortedOptions, 'CLI options are not in alphabetical order');
Comment on lines +134 to +136

@aduh95 aduh95 Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const sortedOptions = [...options].sort();
/* eslint-disable no-restricted-syntax */
assert.deepStrictEqual(options, sortedOptions, 'CLI options are not in alphabetical order');
assert.deepStrictEqual(options, options.toSorted());

@aduh95 aduh95 Nov 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not disable lint rules like that:


Comment on lines +127 to +137

@aduh95 aduh95 Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO we also want env variables and V8 options to be sorted, it makes little sense to reduce it to our CLI options

Suggested change
const start = /^## Options/m;
const end = /^## Environment variables/m;
const filteredCLIText = cliText.slice(cliText.search(start), cliText.search(end)).trim();
const cliOptionPattern = /^### `(--[a-zA-Z0-9-]+)`/mg;
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern)).map((match) => match[1]);
const sortedOptions = [...options].sort();
/* eslint-disable no-restricted-syntax */
assert.deepStrictEqual(options, sortedOptions, 'CLI options are not in alphabetical order');
const sections = /^## (.+)$/mg;
const cliOptionPattern = /^### (?:`-\w.*`, )?`([^`]+)`/mg;
let match;
let previousIndex = 0;
do {
const sectionTitle = match?.[1];
match = sections.exec(cliText);
const filteredCLIText = cliText.slice(previousIndex, match?.index);
const options = Array.from(filteredCLIText.matchAll(cliOptionPattern), (match) => match[1]);
assert.deepStrictEqual(options, options.toSorted(), `${sectionTitle} subsections are not in alphabetical order`);
previousIndex = match?.index;
} while (match);

I can also do that as part of a follow-up PR

}
Expand Down