Skip to content

test: ensure that CLI options are alphabetical - #55790

Closed
avivkeller wants to merge 3 commits into
nodejs:mainfrom
avivkeller:alph-cli
Closed

test: ensure that CLI options are alphabetical#55790
avivkeller wants to merge 3 commits into
nodejs:mainfrom
avivkeller:alph-cli

Conversation

@avivkeller

@avivkelleravivkeller commented Nov 8, 2024

Copy link
Copy Markdown
Member

Blocked by #55788

This PR ensures that all options documented in CLI.md are in alphabetical order.

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 8, 2024
@avivkelleravivkeller added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. blocked PRs that are blocked by other issues or PRs. cli Issues and PRs related to the Node.js command line interface. labels Nov 8, 2024
@codecov

codecovBot commented Nov 8, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (58a8eb4) to head (dd1d23e).
Report is 235 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #55790 +/- ##
==========================================
- Coverage 88.40% 88.40% -0.01% 
==========================================
Files 654 654 Lines 187815 187815 Branches 36136 36137 +1 ==========================================
- Hits 166045 166043 -2 - Misses 15001 15017 +16 + Partials 6769 6755 -14 

see 38 files with indirect coverage changes

@avivkelleravivkeller removed the blocked PRs that are blocked by other issues or PRs. label Nov 9, 2024
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]);

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
constoptions=Array.from(filteredCLIText.matchAll(cliOptionPattern)).map((match)=>match[1]);
constoptions=Array.from(filteredCLIText.matchAll(cliOptionPattern),(match)=>match[1]);

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

@aduh95aduh95Nov 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
constsortedOptions=[...options].sort();
/* eslint-disable no-restricted-syntax */
assert.deepStrictEqual(options,sortedOptions,'CLI options are not in alphabetical order');
assert.deepStrictEqual(options,options.toSorted());

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:


const start = /^## Options/m;
const end = /^## Environment variables/m;
const filteredCLIText = cliText.slice(cliText.search(start), cliText.search(end)).trim();

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
constfilteredCLIText=cliText.slice(cliText.search(start),cliText.search(end)).trim();
constfilteredCLIText=cliText.slice(cliText.search(start),cliText.search(end));

Comment on lines +127 to +137

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');

@aduh95aduh95Nov 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.

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
conststart=/^##Options/m;
constend=/^##Environmentvariables/m;
constfilteredCLIText=cliText.slice(cliText.search(start),cliText.search(end)).trim();
constcliOptionPattern=/^###`(--[a-zA-Z0-9-]+)`/mg;
constoptions=Array.from(filteredCLIText.matchAll(cliOptionPattern)).map((match)=>match[1]);
constsortedOptions=[...options].sort();
/* eslint-disable no-restricted-syntax */
assert.deepStrictEqual(options,sortedOptions,'CLI options are not in alphabetical order');
constsections=/^##(.+)$/mg;
constcliOptionPattern=/^###(?:`-\w.*`,)?`([^`]+)`/mg;
letmatch;
letpreviousIndex=0;
do{
constsectionTitle=match?.[1];
match=sections.exec(cliText);
constfilteredCLIText=cliText.slice(previousIndex,match?.index);
constoptions=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

@aduh95

Copy link
Copy Markdown
Contributor

Superseded by #56025

@aduh95aduh95 closed this Dec 14, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cliIssues and PRs related to the Node.js command line interface.docIssues and PRs related to the documentations.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@avivkeller@aduh95@nodejs-github-bot