Skip to content

FIX(regex): @W-18696814@: Lower p-limit for windows and only read fil…#305

Merged
stephen-carter-at-sf merged 2 commits into
devfrom
sc/W-18696814
Jun 5, 2025
Merged

FIX(regex): @W-18696814@: Lower p-limit for windows and only read fil…#305
stephen-carter-at-sf merged 2 commits into
devfrom
sc/W-18696814

Conversation

@stephen-carter-at-sf

Copy link
Copy Markdown
Contributor

…e once

private async scanFile(fileName: string, ruleName: string): Promise<Violation[]> {
private async scanFileContents(fileName: string, fileContents: string, metaDataContents: string, ruleName: string): Promise<Violation[]> {
const violations: Violation[] = [];
let fileContents: string = await fs.promises.readFile(fileName, {encoding: 'utf8'})

@stephen-carter-at-sf stephen-carter-at-sf Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Before we were reading the file for every single rule. Now I changed it so that we only read a file once and then run the regex for each of its rules the on the file contents.

const violationPromiseFunctions: (() => Promise<Violation[]>)[] = rulesToRun.map(
ruleName => () => this.scanFile(file, ruleName));
return (await this.promiseLimiter.execute(violationPromiseFunctions)).flat();
private async runRulesForFile(fileName: string, ruleNames: string[]): Promise<Violation[]>{

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.

I'm surprised there weren't any test changes for this fix. Are there any that make sense to add as a result of this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? These changes are implementation detail only changes for performance boost. I wouldn't expect a test to fail.

workspace: new engApi.Workspace('FixedId', [path.resolve('src'), path.resolve('test')])
};
const stubEngine1: stubs.StubEngine1 = plugin.getCreatedEngine('stubEngine1') as stubs.StubEngine1;
expect(stubEngine1.describeRulesCallHistory).toEqual([{describeOptions: expectedDescribeOptions}]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With cache warmed up, this test failed because it was doing an exact check on the workspace object. So reworked this test to just look to see if it has the workspace by id.

@stephen-carter-at-sf

Copy link
Copy Markdown
Contributor Author

fixes forcedotcom/code-analyzer#1832

@stephen-carter-at-sf stephen-carter-at-sf merged commit d409af9 into dev Jun 5, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants