-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
const limit = pLimit(5); // Limit the number of concurrent tasks | ||
|
||
/** | ||
* Checks if a given string is a valid URL. | ||
|
@@ -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'); | ||
|
@@ -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'); | ||
} | ||
|
@@ -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}`)); | ||
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); | ||
|
||
// 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; | ||
} | ||
resolve(); | ||
}); | ||
}); | ||
} | ||
|
||
/** | ||
* 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); | ||
|
||
if (relativeFilePath.includes('reference/specification')) { | ||
return; // Skip the specified folder | ||
} | ||
|
||
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)); | ||
} | ||
}); | ||
|
||
await Promise.all(tasks); | ||
} | ||
Comment on lines
+123
to
+142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
🧰 Tools🪛 eslint[error] 124-124: Delete (prettier/prettier) [error] 125-125: Delete (prettier/prettier) [error] 126-126: Replace (prettier/prettier) [error] 127-127: Replace (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 (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 [warning] 129-130: scripts/markdown/check-markdown.js#L129-L130 [warning] 133-137: scripts/markdown/check-markdown.js#L133-L137 [warning] 141-141: scripts/markdown/check-markdown.js#L141 |
||
|
||
// 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([ | ||
processMarkdownFolder(docsFolderPath, validateDocs), | ||
processMarkdownFolder(blogsFolderPath, validateBlogs), | ||
]); | ||
} catch (err) { | ||
console.error(err.message); | ||
process.exitCode = 1; | ||
} | ||
})(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
🧰 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
witherrors.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