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 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
110 changes: 62 additions & 48 deletions scripts/markdown/check-markdown.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
const fs = require('fs');
const matter = require('gray-matter');
const path = require('path');
const pLimit = require('p-limit'); // Import p-limit for concurrency control

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

const limit = pLimit(5); // Limit the number of concurrent tasks

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L6

Added line #L6 was not covered by tests

/**
* Checks if a given string is a valid URL.
Expand All @@ -19,36 +22,30 @@
/**
* Validates the frontmatter of a blog post.
* @param {object} frontmatter - The frontmatter object to validate.
* @param {string} filePath - The path to the file being validated.
* @returns {string[]|null} An array of validation error messages, or null if no errors.
*/
function validateBlogs(frontmatter) {
const requiredAttributes = ['title', 'date', 'type', 'tags', 'cover', 'authors'];
const errors = [];

// Check for required attributes
requiredAttributes.forEach(attr => {
if (!frontmatter.hasOwnProperty(attr)) {
errors.push(`${attr} is missing`);
}
});

// Validate date format
if (frontmatter.date && Number.isNaN(Date.parse(frontmatter.date))) {
errors.push(`Invalid date format: ${frontmatter.date}`);
}

// Validate tags format (must be an array)
if (frontmatter.tags && !Array.isArray(frontmatter.tags)) {
errors.push(`Tags should be an array`);
}

// Validate cover is a string
if (frontmatter.cover && typeof frontmatter.cover !== 'string') {
errors.push(`Cover must be a string`);
}

// Validate authors (must be an array with valid attributes)
if (frontmatter.authors) {
if (!Array.isArray(frontmatter.authors)) {
errors.push('Authors should be an array');
Expand All @@ -73,18 +70,14 @@
/**
* Validates the frontmatter of a documentation file.
* @param {object} frontmatter - The frontmatter object to validate.
* @param {string} filePath - The path to the file being validated.
* @returns {string[]|null} An array of validation error messages, or null if no errors.
*/
function validateDocs(frontmatter) {
const errors = [];

// Check if title exists and is a string
if (!frontmatter.title || typeof frontmatter.title !== 'string') {
errors.push('Title is missing or not a string');
}

// Check if weight exists and is a number
if (frontmatter.weight === undefined || typeof frontmatter.weight !== 'number') {
errors.push('Weight is missing or not a number');
}
Expand All @@ -93,54 +86,75 @@
}

/**
* Recursively checks markdown files in a folder and validates their frontmatter.
* @param {string} folderPath - The path to the folder to check.
* Processes a single Markdown file for validation.
* @param {string} filePath - The full path to the Markdown file.
* @param {Function} validateFunction - The function used to validate the frontmatter.
* @param {string} [relativePath=''] - The relative path of the folder for logging purposes.
* @param {string} relativePath - The relative path for logging purposes.
* @returns {Promise<void>}
*/
function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') {
fs.readdir(folderPath, (err, files) => {
if (err) {
console.error('Error reading directory:', err);
return;
}

files.forEach(file => {
const filePath = path.join(folderPath, file);
const relativeFilePath = path.join(relativePath, file);

// Skip the folder 'docs/reference/specification'
if (relativeFilePath.includes('reference/specification')) {
function processMarkdownFile(filePath, validateFunction, relativePath) {
return new Promise((resolve, reject) => {
fs.readFile(filePath, 'utf-8', (err, content) => {
if (err) {
reject(new Error(`Error reading file ${filePath}: ${err.message}`));

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L96-L99

Added lines #L96 - L99 were not covered by tests
return;
}

fs.stat(filePath, (err, stats) => {
if (err) {
console.error('Error reading file stats:', err);
return;
}
const { data: frontmatter } = matter(content);
const errors = validateFunction(frontmatter);

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L103 - L104 were not covered by tests

// 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;
}
}
});
if (errors) {
console.log(`Errors in file ${relativePath}:`);
errors.forEach(error => console.log(` - ${error}`));
process.exitCode = 1;

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L106-L109

Added lines #L106 - L109 were not covered by tests
}
resolve();

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L111

Added line #L111 was not covered by tests
});
});
Comment on lines +95 to 113
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 test coverage for the processMarkdownFile function

The new function lacks test coverage, which is crucial for ensuring reliability.

Would you like me to help generate unit tests for this function? Key test cases should include:

  • Successful file processing
  • File read errors
  • Invalid frontmatter
  • Various validation error scenarios
🧰 Tools
🪛 eslint

[error] 96-96: Delete ··

(prettier/prettier)


[error] 97-97: Delete ····

(prettier/prettier)


[error] 98-98: Replace ············ with ······

(prettier/prettier)


[error] 99-99: Delete ········

(prettier/prettier)


[error] 100-100: Replace ················ with ········

(prettier/prettier)


[error] 101-101: Replace ············ with ······

(prettier/prettier)


[error] 103-103: Delete ······

(prettier/prettier)


[error] 104-104: Replace ············ with ······

(prettier/prettier)


[error] 106-106: Delete ······

(prettier/prettier)


[error] 107-107: Replace ················ with ········

(prettier/prettier)


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

(prettier/prettier)


[error] 109-109: Delete ········

(prettier/prettier)


[error] 110-110: Replace ············ with ······

(prettier/prettier)


[error] 111-111: Delete ······

(prettier/prettier)


[error] 112-112: Replace ········ with ····

(prettier/prettier)


[error] 113-113: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

[warning] 96-99: scripts/markdown/check-markdown.js#L96-L99
Added lines #L96 - L99 were not covered by tests


[warning] 103-104: scripts/markdown/check-markdown.js#L103-L104
Added lines #L103 - L104 were not covered by tests


[warning] 106-109: scripts/markdown/check-markdown.js#L106-L109
Added lines #L106 - L109 were not covered by tests


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

}

/**
* Recursively processes Markdown files in a folder with a concurrency limit.
* @param {string} folderPath - The path to the folder to process.
* @param {Function} validateFunction - The function used to validate the frontmatter.
* @param {string} [relativePath=''] - The relative path for logging purposes.
* @returns {Promise<void>}
*/
async function processMarkdownFolder(folderPath, validateFunction, relativePath = '') {
const files = await fs.promises.readdir(folderPath);
const tasks = files.map(async (file) => {
const filePath = path.join(folderPath, file);
const relativeFilePath = path.join(relativePath, file);

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L124-L127

Added lines #L124 - L127 were not covered by tests

if (relativeFilePath.includes('reference/specification')) {
return; // Skip the specified folder

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L129-L130

Added lines #L129 - L130 were not covered by tests
}

const stats = await fs.promises.stat(filePath);
if (stats.isDirectory()) {
await processMarkdownFolder(filePath, validateFunction, relativeFilePath);
} else if (path.extname(file) === '.md') {
await limit(() => processMarkdownFile(filePath, validateFunction, relativeFilePath));

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L133-L137

Added lines #L133 - L137 were not covered by tests
}
});

await Promise.all(tasks);

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
}
Comment on lines +123 to +142
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 test coverage for the processMarkdownFolder function

The folder processing logic lacks test coverage, which is essential for ensuring proper handling of different scenarios.

Would you like me to help generate unit tests? Key test cases should include:

  • Processing empty folders
  • Processing nested folders
  • Handling excluded paths
  • Concurrent file processing
  • Error scenarios
🧰 Tools
🪛 eslint

[error] 124-124: Delete ··

(prettier/prettier)


[error] 125-125: Delete ··

(prettier/prettier)


[error] 126-126: Replace ········ with ····

(prettier/prettier)


[error] 127-127: Replace ········ with ····

(prettier/prettier)


[error] 129-129: Delete ····

(prettier/prettier)


[error] 130-130: Delete ······

(prettier/prettier)


[error] 131-131: Delete ····

(prettier/prettier)


[error] 133-133: Delete ····

(prettier/prettier)


[error] 134-134: Delete ····

(prettier/prettier)


[error] 135-135: Delete ······

(prettier/prettier)


[error] 136-136: Delete ····

(prettier/prettier)


[error] 137-137: Replace ············ with ······

(prettier/prettier)


[error] 138-138: Delete ····

(prettier/prettier)


[error] 139-139: Delete ··

(prettier/prettier)


[error] 141-141: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

[warning] 124-127: scripts/markdown/check-markdown.js#L124-L127
Added lines #L124 - L127 were not covered by tests


[warning] 129-130: scripts/markdown/check-markdown.js#L129-L130
Added lines #L129 - L130 were not covered by tests


[warning] 133-137: scripts/markdown/check-markdown.js#L133-L137
Added lines #L133 - L137 were not covered by tests


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


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

checkMarkdownFiles(docsFolderPath, validateDocs);
checkMarkdownFiles(blogsFolderPath, validateBlogs);
// Process folders with concurrency
(async () => {
try {
await Promise.all([

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#L149-L151

Added lines #L149 - L151 were not covered by tests
processMarkdownFolder(docsFolderPath, validateDocs),
processMarkdownFolder(blogsFolderPath, validateBlogs),
]);
} catch (err) {
console.error(err.message);
process.exitCode = 1;

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

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L156-L157

Added lines #L156 - L157 were not covered by tests
}
})();

Loading