Skip to content
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

Add Concurrency Limit to Markdown File Processing for Improved Performance Fixes #3429 #3446

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
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
60 changes: 37 additions & 23 deletions scripts/markdown/check-markdown.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const fs = require('fs');
const matter = require('gray-matter');
const path = require('path');
const pLimit = require('p-limit'); // Import the p-limit package

Check warning on line 4 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L4

Added line #L4 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add 'p-limit' to the project's dependencies

The p-limit module is imported but not listed in your project's dependencies. Please add it to your package.json to ensure the module is available during runtime.

🧰 Tools
🪛 eslint

[error] 4-4: 'p-limit' should be listed in the project's dependencies. Run 'npm i -S p-limit' to add it

(import/no-extraneous-dependencies)


[error] 4-4: Delete ·

(prettier/prettier)

🪛 GitHub Check: codecov/patch

[warning] 4-4: scripts/markdown/check-markdown.js#L4
Added line #L4 was not covered by tests


🛠️ Refactor suggestion

Increase test coverage for the new code

The new code added in these lines is not covered by tests, as indicated by Codecov. Please add tests to ensure the new concurrency control functionality is properly tested.

Also applies to: 104-105, 122-125, 128-132, 134-138, 141-141, 147-147, 151-151, 159-160

🧰 Tools
🪛 eslint

[error] 4-4: 'p-limit' should be listed in the project's dependencies. Run 'npm i -S p-limit' to add it

(import/no-extraneous-dependencies)


[error] 4-4: Delete ·

(prettier/prettier)

🪛 GitHub Check: codecov/patch

[warning] 4-4: scripts/markdown/check-markdown.js#L4
Added line #L4 was not covered by tests


/**
* Checks if a given string is a valid URL.
Expand Down Expand Up @@ -93,12 +94,16 @@
}

/**
* Recursively checks markdown files in a folder and validates their frontmatter.
* Recursively checks markdown files in a folder and validates their frontmatter with concurrency control.
* @param {string} folderPath - The path to the folder to check.
* @param {Function} validateFunction - The function used to validate the frontmatter.
* @param {string} [relativePath=''] - The relative path of the folder for logging purposes.
* @param {number} concurrencyLimit - The maximum number of files to process concurrently.
*/
function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') {
function checkMarkdownFiles(folderPath, validateFunction, relativePath = '', concurrencyLimit = 5) {
const limit = pLimit(concurrencyLimit); // Limit the concurrency
const tasks = [];

Check warning on line 105 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L104-L105

Added lines #L104 - L105 were not covered by tests

fs.readdir(folderPath, (err, files) => {
if (err) {
console.error('Error reading directory:', err);
Expand All @@ -114,33 +119,42 @@
return;
}

fs.stat(filePath, (err, stats) => {
if (err) {
console.error('Error reading file stats:', err);
return;
}

// Recurse if directory, otherwise validate markdown file
if (stats.isDirectory()) {
checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
} else if (path.extname(file) === '.md') {
const fileContent = fs.readFileSync(filePath, 'utf-8');
const { data: frontmatter } = matter(fileContent);

const errors = validateFunction(frontmatter);
if (errors) {
console.log(`Errors in file ${relativeFilePath}:`);
errors.forEach(error => console.log(` - ${error}`));
process.exitCode = 1;
const task = new Promise((resolve, reject) => {
fs.stat(filePath, (err, stats) => {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid variable shadowing of 'err'

The variable err is already declared in the upper scope in the fs.readdir callback. Renaming the parameter in the fs.stat callback will prevent variable shadowing, reducing potential confusion and bugs.

Apply this diff to fix the issue:

- fs.stat(filePath, (err, stats) => {
+ fs.stat(filePath, (statErr, stats) => {

Also, update the reference to err inside this callback:

- if (err) {
-     reject('Error reading file stats:', err);
+ if (statErr) {
+     reject(new Error(`Error reading file stats: ${statErr.message}`));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fs.stat(filePath, (err, stats) => {
fs.stat(filePath, (statErr, stats) => {
🧰 Tools
🪛 eslint

[error] 123-123: Delete ········

(prettier/prettier)


[error] 123-123: 'err' is already declared in the upper scope on line 107 column 29.

(no-shadow)

if (err) {
reject('Error reading file stats:', err);

Check warning on line 125 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L122-L125

Added lines #L122 - L125 were not covered by tests
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use Error object when rejecting promises

When rejecting a promise, the rejection reason should be an Error object. Modify the rejection to pass an Error containing the error message.

Apply this diff to fix the issue:

- reject('Error reading file stats:', err);
+ reject(new Error(`Error reading file stats: ${statErr.message}`));

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 125-125: Delete ············

(prettier/prettier)


[error] 125-125: Expected the Promise rejection reason to be an Error.

(prefer-promise-reject-errors)

🪛 GitHub Check: codecov/patch

[warning] 122-125: scripts/markdown/check-markdown.js#L122-L125
Added lines #L122 - L125 were not covered by tests

} else {
// Recurse if directory, otherwise validate markdown file
if (stats.isDirectory()) {
checkMarkdownFiles(filePath, validateFunction, relativeFilePath, concurrencyLimit);
} else if (path.extname(file) === '.md') {
const fileContent = fs.readFileSync(filePath, 'utf-8');
const { data: frontmatter } = matter(fileContent);

Check warning on line 132 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L128-L132

Added lines #L128 - L132 were not covered by tests

const errors = validateFunction(frontmatter);
if (errors) {
console.log(`Errors in file ${relativeFilePath}:`);
errors.forEach(error => console.log(` - ${error}`));
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix missing parentheses in arrow function

There is a missing set of parentheses around the parameter in the arrow function, which can cause syntax errors.

Apply this diff to fix the issue:

- errors.forEach(error => console.log(` - ${error}`));
+ errors.forEach((error) => console.log(` - ${error}`));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
errors.forEach(error => console.log(` - ${error}`));
errors.forEach((error) => console.log(` - ${error}`));
🧰 Tools
🪛 eslint

[error] 137-137: Replace ································errors.forEach(error with ················errors.forEach((error)

(prettier/prettier)

process.exitCode = 1;

Check warning on line 138 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L134-L138

Added lines #L134 - L138 were not covered by tests
}
}
resolve();

Check warning on line 141 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L141

Added line #L141 was not covered by tests
}
}
});
});

// Add task with concurrency limit
tasks.push(limit(() => task));

Check warning on line 147 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L147

Added line #L147 was not covered by tests
});

// Wait for all tasks to complete
Promise.all(tasks).then(() => console.log('All files processed.'));

Check warning on line 151 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L151

Added line #L151 was not covered by tests
});
}

const docsFolderPath = path.resolve(__dirname, '../../markdown/docs');
const blogsFolderPath = path.resolve(__dirname, '../../markdown/blog');

checkMarkdownFiles(docsFolderPath, validateDocs);
checkMarkdownFiles(blogsFolderPath, validateBlogs);
// Call the function with concurrency control
checkMarkdownFiles(docsFolderPath, validateDocs, '', 5); // Limit concurrency to 5
checkMarkdownFiles(blogsFolderPath, validateBlogs, '', 5); // Limit concurrency to 5

Check warning on line 160 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L159-L160

Added lines #L159 - L160 were not covered by tests
Loading