fix: implement concurrency control for markdown file processing #3462
Conversation
Aditya0732
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces significant enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Aditya0732
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3462 +/- ##
==========================================
- Coverage 77.22% 76.64% -0.58%
==========================================
Files 21 21
Lines 663 668 +5
==========================================
Hits 512 512
- Misses 151 156 +5 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3462--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
scripts/markdown/check-markdown.js (5)
7-9: Simplify theCONCURRENCY_LIMITassignment formattingThe ternary operator can be formatted on a single line for better readability.
Apply this diff to adjust the formatting:
-const CONCURRENCY_LIMIT = process.env.CONCURRENCY_LIMIT - ? parseInt(process.env.CONCURRENCY_LIMIT, 10) - : 10; +const CONCURRENCY_LIMIT = process.env.CONCURRENCY_LIMIT ? parseInt(process.env.CONCURRENCY_LIMIT, 10) : 10;🧰 Tools
🪛 eslint
[error] 7-9: Replace
⏎··?·parseInt(process.env.CONCURRENCY_LIMIT,·10)·⏎·with?·parseInt(process.env.CONCURRENCY_LIMIT,·10)(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
Line range hint
109-148: Remove unnecessary outer try/catch blockThe try/catch block from lines 109 to 148 catches
filesErroronly to rethrow it without additional handling, which is unnecessary. Removing it simplifies the code without affecting functionality.Apply this diff to remove the redundant try/catch block:
- try { // Read directory contents const files = await fs.readdir(folderPath); // Process files with concurrency control const filePromises = files.map(file => limit(async () => { const filePath = path.join(folderPath, file); const relativeFilePath = path.join(relativePath, file); // Skip the folder 'docs/reference/specification' if (relativeFilePath.includes('reference/specification')) { return; } try { // Get file stats const stats = await fs.stat(filePath); // Recurse if directory, otherwise validate markdown file if (stats.isDirectory()) { await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); } else if (path.extname(file) === '.md') { const fileContent = await fs.readFile(filePath, 'utf-8'); const { data: frontmatter } = matter(fileContent); const errors = validateFunction(frontmatter); if (errors) { process.exitCode = 1; } } } catch (statError) { throw statError; } })); // Wait for all file promises to complete await Promise.all(filePromises); - } catch (filesError) { - throw filesError; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 147-147: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
🪛 eslint
[error] 120-120: Replace
················with··········(prettier/prettier)
[error] 121-121: Delete
····(prettier/prettier)
[error] 123-123: Delete
····(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace
················with··········(prettier/prettier)
[error] 125-125: Delete
······(prettier/prettier)
[error] 127-127: Replace
················with··········(prettier/prettier)
[error] 128-128: Delete
······(prettier/prettier)
[error] 129-129: Replace
····················with············(prettier/prettier)
[error] 130-130: Delete
······(prettier/prettier)
[error] 131-131: Replace
····················with············(prettier/prettier)
[error] 132-132: Replace
····················with············(prettier/prettier)
[error] 134-134: Delete
········(prettier/prettier)
[error] 135-135: Replace
····················with············(prettier/prettier)
[error] 136-136: Replace
························with··············(prettier/prettier)
[error] 137-137: Delete
········(prettier/prettier)
[error] 138-138: Replace
················with··········(prettier/prettier)
[error] 139-139: Delete
····(prettier/prettier)
[error] 140-140: Delete
······(prettier/prettier)
[error] 141-141: Delete
····(prettier/prettier)
[error] 142-143: Replace
··}));⏎with})(prettier/prettier)
[error] 144-144: Insert
);⏎⏎(prettier/prettier)
[error] 145-145: Delete
····(prettier/prettier)
[error] 146-146: Delete
··(prettier/prettier)
[error] 147-147: Delete
····(prettier/prettier)
[error] 148-148: Delete
··(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 145-145: scripts/markdown/check-markdown.js#L145
Added line #L145 was not covered by tests
[warning] 147-147: scripts/markdown/check-markdown.js#L147
Added line #L147 was not covered by tests
Line range hint
105-145: Re-evaluate asynchronous file operations in recursive processingBased on previous learnings:
"In
scripts/markdown/check-markdown.js, when processing large markdown files with recursion and loops, synchronous file reads (fs.readFileSync) are preferred over asynchronous reads."While the introduction of concurrency control with
p-limitmitigates the risk of resource exhaustion, consider whether using synchronous file operations could further simplify the recursion and handling of file descriptors.Let me know if you would like assistance in refactoring the code to use synchronous file operations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 147-147: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
🪛 eslint
[error] 120-120: Replace
················with··········(prettier/prettier)
[error] 121-121: Delete
····(prettier/prettier)
[error] 123-123: Delete
····(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace
················with··········(prettier/prettier)
[error] 125-125: Delete
······(prettier/prettier)
[error] 127-127: Replace
················with··········(prettier/prettier)
[error] 128-128: Delete
······(prettier/prettier)
[error] 129-129: Replace
····················with············(prettier/prettier)
[error] 130-130: Delete
······(prettier/prettier)
[error] 131-131: Replace
····················with············(prettier/prettier)
[error] 132-132: Replace
····················with············(prettier/prettier)
[error] 134-134: Delete
········(prettier/prettier)
[error] 135-135: Replace
····················with············(prettier/prettier)
[error] 136-136: Replace
························with··············(prettier/prettier)
[error] 137-137: Delete
········(prettier/prettier)
[error] 138-138: Replace
················with··········(prettier/prettier)
[error] 139-139: Delete
····(prettier/prettier)
[error] 140-140: Delete
······(prettier/prettier)
[error] 141-141: Delete
····(prettier/prettier)
[error] 142-143: Replace
··}));⏎with})(prettier/prettier)
[error] 144-144: Insert
);⏎⏎(prettier/prettier)
[error] 145-145: Delete
····(prettier/prettier)
[error] 146-146: Delete
··(prettier/prettier)
[error] 147-147: Delete
····(prettier/prettier)
[error] 148-148: Delete
··(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 145-145: scripts/markdown/check-markdown.js#L145
Added line #L145 was not covered by tests
[warning] 147-147: scripts/markdown/check-markdown.js#L147
Added line #L147 was not covered by tests
Line range hint
105-148: Add unit tests for asynchronous processing and concurrency controlThe new asynchronous logic and concurrency control are significant changes. Please add unit tests to cover these new code paths to maintain code coverage and ensure correct behavior.
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 147-147: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
🪛 eslint
[error] 120-120: Replace
················with··········(prettier/prettier)
[error] 121-121: Delete
····(prettier/prettier)
[error] 123-123: Delete
····(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace
················with··········(prettier/prettier)
[error] 125-125: Delete
······(prettier/prettier)
[error] 127-127: Replace
················with··········(prettier/prettier)
[error] 128-128: Delete
······(prettier/prettier)
[error] 129-129: Replace
····················with············(prettier/prettier)
[error] 130-130: Delete
······(prettier/prettier)
[error] 131-131: Replace
····················with············(prettier/prettier)
[error] 132-132: Replace
····················with············(prettier/prettier)
[error] 134-134: Delete
········(prettier/prettier)
[error] 135-135: Replace
····················with············(prettier/prettier)
[error] 136-136: Replace
························with··············(prettier/prettier)
[error] 137-137: Delete
········(prettier/prettier)
[error] 138-138: Replace
················with··········(prettier/prettier)
[error] 139-139: Delete
····(prettier/prettier)
[error] 140-140: Delete
······(prettier/prettier)
[error] 141-141: Delete
····(prettier/prettier)
[error] 142-143: Replace
··}));⏎with})(prettier/prettier)
[error] 144-144: Insert
);⏎⏎(prettier/prettier)
[error] 145-145: Delete
····(prettier/prettier)
[error] 146-146: Delete
··(prettier/prettier)
[error] 147-147: Delete
····(prettier/prettier)
[error] 148-148: Delete
··(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 145-145: scripts/markdown/check-markdown.js#L145
Added line #L145 was not covered by tests
[warning] 147-147: scripts/markdown/check-markdown.js#L147
Added line #L147 was not covered by tests
Line range hint
7-148: Adjust code formatting to meet style guidelinesSeveral formatting inconsistencies are detected by the linter, primarily related to indentation and line breaks. Running the project's code formatter (e.g., Prettier) will resolve these issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 147-147: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
🪛 eslint
[error] 120-120: Replace
················with··········(prettier/prettier)
[error] 121-121: Delete
····(prettier/prettier)
[error] 123-123: Delete
····(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace
················with··········(prettier/prettier)
[error] 125-125: Delete
······(prettier/prettier)
[error] 127-127: Replace
················with··········(prettier/prettier)
[error] 128-128: Delete
······(prettier/prettier)
[error] 129-129: Replace
····················with············(prettier/prettier)
[error] 130-130: Delete
······(prettier/prettier)
[error] 131-131: Replace
····················with············(prettier/prettier)
[error] 132-132: Replace
····················with············(prettier/prettier)
[error] 134-134: Delete
········(prettier/prettier)
[error] 135-135: Replace
····················with············(prettier/prettier)
[error] 136-136: Replace
························with··············(prettier/prettier)
[error] 137-137: Delete
········(prettier/prettier)
[error] 138-138: Replace
················with··········(prettier/prettier)
[error] 139-139: Delete
····(prettier/prettier)
[error] 140-140: Delete
······(prettier/prettier)
[error] 141-141: Delete
····(prettier/prettier)
[error] 142-143: Replace
··}));⏎with})(prettier/prettier)
[error] 144-144: Insert
);⏎⏎(prettier/prettier)
[error] 145-145: Delete
····(prettier/prettier)
[error] 146-146: Delete
··(prettier/prettier)
[error] 147-147: Delete
····(prettier/prettier)
[error] 148-148: Delete
··(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 145-145: scripts/markdown/check-markdown.js#L145
Added line #L145 was not covered by tests
[warning] 147-147: scripts/markdown/check-markdown.js#L147
Added line #L147 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/markdown/check-markdown.js(3 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🪛 eslint
scripts/markdown/check-markdown.js
[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] 7-9: Replace ⏎··?·parseInt(process.env.CONCURRENCY_LIMIT,·10)·⏎· with ?·parseInt(process.env.CONCURRENCY_LIMIT,·10)
(prettier/prettier)
[error] 106-106: Delete ··
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 109-109: Delete ··
(prettier/prettier)
[error] 110-110: Delete ····
(prettier/prettier)
[error] 111-111: Replace ········ with ····
(prettier/prettier)
[error] 113-113: Delete ····
(prettier/prettier)
[error] 114-114: Replace ····const·filePromises·=·files.map(file·=> with const·filePromises·=·files.map((file)·=>⏎·····
(prettier/prettier)
[error] 109-148: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 123-123: Delete ····
(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace ················ with ··········
(prettier/prettier)
[error] 125-125: Delete ······
(prettier/prettier)
[error] 127-127: Replace ················ with ··········
(prettier/prettier)
[error] 128-128: Delete ······
(prettier/prettier)
[error] 129-129: Replace ···················· with ············
(prettier/prettier)
[error] 130-130: Delete ······
(prettier/prettier)
[error] 131-131: Replace ···················· with ············
(prettier/prettier)
[error] 132-132: Replace ···················· with ············
(prettier/prettier)
[error] 134-134: Delete ········
(prettier/prettier)
[error] 135-135: Replace ···················· with ············
(prettier/prettier)
[error] 136-136: Replace ························ with ··············
(prettier/prettier)
[error] 137-137: Delete ········
(prettier/prettier)
[error] 138-138: Replace ················ with ··········
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 140-140: Delete ······
(prettier/prettier)
[error] 141-141: Delete ····
(prettier/prettier)
[error] 142-143: Replace ··}));⏎ with })
(prettier/prettier)
[error] 144-144: Insert );⏎⏎
(prettier/prettier)
[error] 145-145: Delete ····
(prettier/prettier)
[error] 146-146: Delete ··
(prettier/prettier)
[error] 147-147: Delete ····
(prettier/prettier)
[error] 148-148: Delete ··
(prettier/prettier)
🪛 GitHub Check: codecov/patch
scripts/markdown/check-markdown.js
[warning] 1-1: scripts/markdown/check-markdown.js#L1
Added line #L1 was not covered by tests
[warning] 4-4: scripts/markdown/check-markdown.js#L4
Added line #L4 was not covered by tests
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
[warning] 107-107: scripts/markdown/check-markdown.js#L107
Added line #L107 was not covered by tests
[warning] 109-109: scripts/markdown/check-markdown.js#L109
Added line #L109 was not covered by tests
[warning] 111-111: scripts/markdown/check-markdown.js#L111
Added line #L111 was not covered by tests
[warning] 114-114: scripts/markdown/check-markdown.js#L114
Added line #L114 was not covered by tests
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 145-145: scripts/markdown/check-markdown.js#L145
Added line #L145 was not covered by tests
[warning] 147-147: scripts/markdown/check-markdown.js#L147
Added line #L147 was not covered by tests
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 147-147: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
scripts/markdown/check-markdown.js
Outdated
| try { | ||
| // Get file stats | ||
| const stats = await fs.stat(filePath); | ||
|
|
||
| // Recurse if directory, otherwise validate markdown file | ||
| if (stats.isDirectory()) { | ||
| checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | ||
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | ||
| } else if (path.extname(file) === '.md') { | ||
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | ||
| const fileContent = await fs.readFile(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; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } catch (statError) { | ||
| throw statError; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary inner try/catch block
The try/catch block from lines 123 to 141 is unnecessary because it only rethrows the caught error without additional processing. Eliminating it will make the code cleaner.
Apply this diff to remove the unnecessary try/catch block:
- try {
// Get file stats
const stats = await fs.stat(filePath);
// Recurse if directory, otherwise validate markdown file
if (stats.isDirectory()) {
await checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
} else if (path.extname(file) === '.md') {
const fileContent = await fs.readFile(filePath, 'utf-8');
const { data: frontmatter } = matter(fileContent);
const errors = validateFunction(frontmatter);
if (errors) {
process.exitCode = 1;
}
}
- } catch (statError) {
- throw statError;
- }📝 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.
| try { | |
| // Get file stats | |
| const stats = await fs.stat(filePath); | |
| // Recurse if directory, otherwise validate markdown file | |
| if (stats.isDirectory()) { | |
| checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | |
| const fileContent = await fs.readFile(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; | |
| } | |
| } | |
| }); | |
| }); | |
| }); | |
| } catch (statError) { | |
| throw statError; | |
| } | |
| // Get file stats | |
| const stats = await fs.stat(filePath); | |
| // Recurse if directory, otherwise validate markdown file | |
| if (stats.isDirectory()) { | |
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = await fs.readFile(filePath, 'utf-8'); | |
| const { data: frontmatter } = matter(fileContent); | |
| const errors = validateFunction(frontmatter); | |
| if (errors) { | |
| process.exitCode = 1; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🪛 eslint
[error] 123-123: Delete ····
(prettier/prettier)
[error] 123-141: Unnecessary try/catch wrapper.
(no-useless-catch)
[error] 124-124: Replace ················ with ··········
(prettier/prettier)
[error] 125-125: Delete ······
(prettier/prettier)
[error] 127-127: Replace ················ with ··········
(prettier/prettier)
[error] 128-128: Delete ······
(prettier/prettier)
[error] 129-129: Replace ···················· with ············
(prettier/prettier)
[error] 130-130: Delete ······
(prettier/prettier)
[error] 131-131: Replace ···················· with ············
(prettier/prettier)
[error] 132-132: Replace ···················· with ············
(prettier/prettier)
[error] 134-134: Delete ········
(prettier/prettier)
[error] 135-135: Replace ···················· with ············
(prettier/prettier)
[error] 136-136: Replace ························ with ··············
(prettier/prettier)
[error] 137-137: Delete ········
(prettier/prettier)
[error] 138-138: Replace ················ with ··········
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 140-140: Delete ······
(prettier/prettier)
[error] 141-141: Delete ····
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 123-123: scripts/markdown/check-markdown.js#L123
Added line #L123 was not covered by tests
[warning] 125-125: scripts/markdown/check-markdown.js#L125
Added line #L125 was not covered by tests
[warning] 129-129: scripts/markdown/check-markdown.js#L129
Added line #L129 was not covered by tests
[warning] 131-131: scripts/markdown/check-markdown.js#L131
Added line #L131 was not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
scripts/markdown/check-markdown.js (3)
6-7: Consider adding validation for CONCURRENCY_LIMIT.While the default of 10 is reasonable, the environment variable should be validated to ensure it's a positive integer.
// Configuration -const CONCURRENCY_LIMIT = process.env.CONCURRENCY_LIMIT ? parseInt(process.env.CONCURRENCY_LIMIT, 10) : 10; +const parsedLimit = process.env.CONCURRENCY_LIMIT ? parseInt(process.env.CONCURRENCY_LIMIT, 10) : 10; +const CONCURRENCY_LIMIT = Number.isInteger(parsedLimit) && parsedLimit > 0 ? parsedLimit : 10;🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
15-20: Avoid side effects in URL validation.The current implementation creates a new URL object just for validation. Consider using the URL constructor's return value instead.
function isValidURL(str) { try { - new URL(str); - return true; + return !!new URL(str); } catch (err) { return false; } }🧰 Tools
🪛 eslint
[error] 16-16: Do not use 'new' for side effects.
(no-new)
🪛 GitHub Check: codecov/patch
[warning] 15-17: scripts/markdown/check-markdown.js#L15-L17
Added lines #L15 - L17 were not covered by tests
[warning] 19-19: scripts/markdown/check-markdown.js#L19
Added line #L19 was not covered by tests
135-136: Document the reason for skipping 'reference/specification'.The code skips the 'reference/specification' directory without explanation. Consider adding a comment explaining why this directory is excluded.
// Skip specific folder + // Skip reference/specification as it contains auto-generated markdown files if (relativeFilePath.includes('reference/specification')) {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-136: scripts/markdown/check-markdown.js#L135-L136
Added lines #L135 - L136 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)scripts/markdown/check-markdown.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 41-41: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
scripts/markdown/check-markdown.js
[error] 16-16: Do not use 'new' for side effects.
(no-new)
[error] 29-36: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 41-41: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 71-73: Replace ⏎············Invalid·URL·for·author·at·index·${index}:·${author.link},⏎·········· with Invalid·URL·for·author·at·index·${index}:·${author.link}
(prettier/prettier)
[error] 99-102: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 116-120: Replace ⏎··folderPath,⏎··validateFunction,⏎··relativePath·=·'',⏎ with folderPath,·validateFunction,·relativePath·=·''
(prettier/prettier)
[error] 144-148: Replace ⏎············filePath,⏎············validateFunction,⏎············relativeFilePath,⏎·········· with filePath,·validateFunction,·relativeFilePath
(prettier/prettier)
[error] 160-162: Replace ⏎··············Error·reading·file·${relativeFilePath}:·${readError.message},⏎············ with Error·reading·file·${relativeFilePath}:·${readError.message}
(prettier/prettier)
[error] 167-167: Delete ,
(prettier/prettier)
🪛 GitHub Check: codecov/patch
scripts/markdown/check-markdown.js
[warning] 3-4: scripts/markdown/check-markdown.js#L3-L4
Added lines #L3 - L4 were not covered by tests
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
[warning] 15-17: scripts/markdown/check-markdown.js#L15-L17
Added lines #L15 - L17 were not covered by tests
[warning] 19-19: scripts/markdown/check-markdown.js#L19
Added line #L19 was not covered by tests
[warning] 29-29: scripts/markdown/check-markdown.js#L29
Added line #L29 was not covered by tests
[warning] 37-37: scripts/markdown/check-markdown.js#L37
Added line #L37 was not covered by tests
[warning] 40-42: scripts/markdown/check-markdown.js#L40-L42
Added lines #L40 - L42 were not covered by tests
[warning] 47-48: scripts/markdown/check-markdown.js#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 52-53: scripts/markdown/check-markdown.js#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 57-58: scripts/markdown/check-markdown.js#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 62-64: scripts/markdown/check-markdown.js#L62-L64
Added lines #L62 - L64 were not covered by tests
[warning] 66-68: scripts/markdown/check-markdown.js#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 70-71: scripts/markdown/check-markdown.js#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 75-76: scripts/markdown/check-markdown.js#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 82-82: scripts/markdown/check-markdown.js#L82
Added line #L82 was not covered by tests
[warning] 91-91: scripts/markdown/check-markdown.js#L91
Added line #L91 was not covered by tests
[warning] 94-95: scripts/markdown/check-markdown.js#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 99-99: scripts/markdown/check-markdown.js#L99
Added line #L99 was not covered by tests
[warning] 103-103: scripts/markdown/check-markdown.js#L103
Added line #L103 was not covered by tests
[warning] 106-106: scripts/markdown/check-markdown.js#L106
Added line #L106 was not covered by tests
[warning] 121-122: scripts/markdown/check-markdown.js#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: scripts/markdown/check-markdown.js#L124
Added line #L124 was not covered by tests
[warning] 126-126: scripts/markdown/check-markdown.js#L126
Added line #L126 was not covered by tests
[warning] 129-132: scripts/markdown/check-markdown.js#L129-L132
Added lines #L129 - L132 were not covered by tests
[warning] 135-136: scripts/markdown/check-markdown.js#L135-L136
Added lines #L135 - L136 were not covered by tests
[warning] 140-140: scripts/markdown/check-markdown.js#L140
Added line #L140 was not covered by tests
[warning] 143-144: scripts/markdown/check-markdown.js#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 149-153: scripts/markdown/check-markdown.js#L149-L153
Added lines #L149 - L153 were not covered by tests
[warning] 155-157: scripts/markdown/check-markdown.js#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 160-160: scripts/markdown/check-markdown.js#L160
Added line #L160 was not covered by tests
🔇 Additional comments (3)
package.json (1)
93-93: LGTM! The addition of p-limit is appropriate.
The addition of p-limit aligns with the PR's objective of implementing concurrency control for markdown file processing.
scripts/markdown/check-markdown.js (2)
121-122: LGTM! Effective concurrency control implementation.
The use of p-limit with Promise.all effectively manages concurrent file operations while maintaining control over resource usage.
Also applies to: 170-171
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-122: scripts/markdown/check-markdown.js#L121-L122
Added lines #L121 - L122 were not covered by tests
151-163: 🛠️ Refactor suggestion
Consider using synchronous file operations consistently.
Based on the retrieved learning, synchronous file operations are preferred for processing large markdown files. The code already uses readdirSync and statSync, but mixes async/await with synchronous operations.
🧰 Tools
🪛 eslint
[error] 160-162: Replace ⏎··············Error·reading·file·${relativeFilePath}:·${readError.message},⏎············ with Error·reading·file·${relativeFilePath}:·${readError.message}
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 155-157: scripts/markdown/check-markdown.js#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 160-160: scripts/markdown/check-markdown.js#L160
Added line #L160 was not covered by tests
| if (!frontmatter.hasOwnProperty(attr)) { | ||
| errors.push(`${attr} is missing`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() instead of hasOwnProperty.
Direct use of hasOwnProperty is discouraged as it can be overridden. Use Object.hasOwn() for safer property checks.
- if (!frontmatter.hasOwnProperty(attr)) {
+ if (!Object.hasOwn(frontmatter, attr)) {📝 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.
| if (!frontmatter.hasOwnProperty(attr)) { | |
| errors.push(`${attr} is missing`); | |
| if (!Object.hasOwn(frontmatter, attr)) { | |
| errors.push(`${attr} is missing`); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
[error] 41-41: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
🪛 GitHub Check: codecov/patch
[warning] 40-42: scripts/markdown/check-markdown.js#L40-L42
Added lines #L40 - L42 were not covered by tests
…erformance with code improvisions
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
scripts/markdown/check-markdown.js (2)
111-111: Consider batching file operations for better performanceThe current implementation processes files one by one with concurrency control. Consider batching file operations to reduce overhead:
-const filePromises = files.map((file) => - limit(async () => { - const filePath = path.join(folderPath, file); - const relativeFilePath = path.join(relativePath, file); +const batchSize = 5; +const filePromises = []; +for (let i = 0; i < files.length; i += batchSize) { + const batch = files.slice(i, i + batchSize); + filePromises.push( + limit(async () => { + const results = await Promise.all( + batch.map(async (file) => { + const filePath = path.join(folderPath, file); + const relativeFilePath = path.join(relativePath, file);Also applies to: 119-122
🧰 Tools
🪛 eslint
[error] 111-111: Delete
··(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 111-112: scripts/markdown/check-markdown.js#L111-L112
Added lines #L111 - L112 were not covered by tests
125-126: Document the reference/specification exclusionThe code skips the 'reference/specification' folder without explanation. Consider adding a comment explaining why this folder is excluded.
+// Skip auto-generated specification documentation if (relativeFilePath.includes('reference/specification')) { return []; }🧰 Tools
🪛 eslint
[error] 125-125: Replace
················with········(prettier/prettier)
[error] 126-126: Replace
····················with··········(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 125-126: scripts/markdown/check-markdown.js#L125-L126
Added lines #L125 - L126 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/markdown/check-markdown.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (2)
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:31-31
Timestamp: 2024-11-12T06:59:33.852Z
Learning: In `scripts/markdown/check-markdown.js`, the `frontmatter` object cannot have a property named `hasOwnProperty`, so using `frontmatter.hasOwnProperty(attr)` is acceptable.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🪛 GitHub Check: codecov/patch
scripts/markdown/check-markdown.js
[warning] 3-4: scripts/markdown/check-markdown.js#L3-L4
Added lines #L3 - L4 were not covered by tests
[warning] 7-8: scripts/markdown/check-markdown.js#L7-L8
Added lines #L7 - L8 were not covered by tests
[warning] 111-112: scripts/markdown/check-markdown.js#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 114-114: scripts/markdown/check-markdown.js#L114
Added line #L114 was not covered by tests
[warning] 116-116: scripts/markdown/check-markdown.js#L116
Added line #L116 was not covered by tests
[warning] 119-122: scripts/markdown/check-markdown.js#L119-L122
Added lines #L119 - L122 were not covered by tests
[warning] 125-126: scripts/markdown/check-markdown.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 130-130: scripts/markdown/check-markdown.js#L130
Added line #L130 was not covered by tests
[warning] 134-134: scripts/markdown/check-markdown.js#L134
Added line #L134 was not covered by tests
[warning] 139-139: scripts/markdown/check-markdown.js#L139
Added line #L139 was not covered by tests
[warning] 141-143: scripts/markdown/check-markdown.js#L141-L143
Added lines #L141 - L143 were not covered by tests
[warning] 145-148: scripts/markdown/check-markdown.js#L145-L148
Added lines #L145 - L148 were not covered by tests
[warning] 151-151: scripts/markdown/check-markdown.js#L151
Added line #L151 was not covered by tests
[warning] 157-157: scripts/markdown/check-markdown.js#L157
Added line #L157 was not covered by tests
[warning] 162-162: scripts/markdown/check-markdown.js#L162
Added line #L162 was not covered by tests
[warning] 164-164: scripts/markdown/check-markdown.js#L164
Added line #L164 was not covered by tests
[warning] 166-167: scripts/markdown/check-markdown.js#L166-L167
Added lines #L166 - L167 were not covered by tests
[warning] 176-177: scripts/markdown/check-markdown.js#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-179: scripts/markdown/check-markdown.js#L179
Added line #L179 was not covered by tests
[warning] 181-185: scripts/markdown/check-markdown.js#L181-L185
Added lines #L181 - L185 were not covered by tests
[warning] 189-193: scripts/markdown/check-markdown.js#L189-L193
Added lines #L189 - L193 were not covered by tests
[warning] 196-196: scripts/markdown/check-markdown.js#L196
Added line #L196 was not covered by tests
[warning] 198-199: scripts/markdown/check-markdown.js#L198-L199
Added lines #L198 - L199 were not covered by tests
[warning] 204-204: scripts/markdown/check-markdown.js#L204
Added line #L204 was not covered by tests
🪛 eslint
scripts/markdown/check-markdown.js
[error] 106-110: Replace ⏎····folderPath,⏎····validateFunction,⏎····relativePath·=·'',⏎ with folderPath,·validateFunction,·relativePath·=·''
(prettier/prettier)
[error] 111-111: Delete ··
(prettier/prettier)
[error] 112-112: Delete ··
(prettier/prettier)
[error] 114-114: Delete ··
(prettier/prettier)
[error] 115-115: Delete ····
(prettier/prettier)
[error] 116-116: Delete ····
(prettier/prettier)
[error] 118-118: Delete ····
(prettier/prettier)
[error] 119-119: Delete ····
(prettier/prettier)
[error] 120-120: Delete ······
(prettier/prettier)
[error] 121-121: Replace ················ with ········
(prettier/prettier)
[error] 122-122: Replace ················ with ········
(prettier/prettier)
[error] 124-124: Delete ········
(prettier/prettier)
[error] 125-125: Replace ················ with ········
(prettier/prettier)
[error] 126-126: Replace ···················· with ··········
(prettier/prettier)
[error] 127-127: Delete ········
(prettier/prettier)
[error] 129-129: Replace ················ with ········
(prettier/prettier)
[error] 130-130: Replace ················ with ········
(prettier/prettier)
[error] 132-132: Delete ········
(prettier/prettier)
[error] 133-133: Replace ················ with ········
(prettier/prettier)
[error] 134-138: Replace ····················const·subdirErrors·=·await·checkMarkdownFiles(⏎························filePath,⏎························validateFunction,⏎························relativeFilePath,⏎···················· with ··········const·subdirErrors·=·await·checkMarkdownFiles(filePath,·validateFunction,·relativeFilePath
(prettier/prettier)
[error] 139-139: Delete ··········
(prettier/prettier)
[error] 140-140: Replace ················ with ········
(prettier/prettier)
[error] 141-141: Replace ···················· with ··········
(prettier/prettier)
[error] 142-142: Delete ············
(prettier/prettier)
[error] 143-143: Replace ························ with ············
(prettier/prettier)
[error] 145-145: Replace ························ with ············
(prettier/prettier)
[error] 146-146: Replace ························ with ············
(prettier/prettier)
[error] 147-147: Delete ··············
(prettier/prettier)
[error] 148-148: Replace ···························· with ··············
(prettier/prettier)
[error] 149-149: Delete ············
(prettier/prettier)
[error] 150-150: Replace ···················· with ··········
(prettier/prettier)
[error] 151-153: Replace ························allErrors.push(⏎····························Error·reading·file·${relativeFilePath}:·${readError.message},⏎························ with ············allErrors.push(Error·reading·file·${relativeFilePath}:·${readError.message}``
(prettier/prettier)
[error] 154-154: Replace ···················· with ··········
(prettier/prettier)
[error] 155-155: Replace ················ with ········
(prettier/prettier)
[error] 157-157: Delete ········
(prettier/prettier)
[error] 158-158: Replace ············}), with ······})
(prettier/prettier)
[error] 159-159: Replace ········ with ····
(prettier/prettier)
[error] 161-161: Delete ····
(prettier/prettier)
[error] 162-162: Delete ····
(prettier/prettier)
[error] 164-164: Replace ········ with ····
(prettier/prettier)
[error] 165-165: Replace ···· with ··
(prettier/prettier)
[error] 166-166: Replace ········ with ····
(prettier/prettier)
[error] 167-167: Delete ····
(prettier/prettier)
[error] 168-168: Replace ···· with ··
(prettier/prettier)
[error] 176-176: Delete ··
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 179-179: Replace ···· with ··
(prettier/prettier)
[error] 180-180: Delete ····
(prettier/prettier)
[error] 181-181: Delete ····
(prettier/prettier)
[error] 182-182: Replace ········ with ····
(prettier/prettier)
[error] 183-183: Delete ······
(prettier/prettier)
[error] 184-184: Replace ············ with ······
(prettier/prettier)
[error] 185-185: Delete ······
(prettier/prettier)
[error] 186-186: Delete ····
(prettier/prettier)
[error] 188-188: Replace ········ with ····
(prettier/prettier)
[error] 189-189: Delete ····
(prettier/prettier)
[error] 190-190: Delete ····
(prettier/prettier)
[error] 191-191: Replace ············ with ······
(prettier/prettier)
[error] 192-192: Delete ······
(prettier/prettier)
[error] 193-193: Replace ············ with ······
(prettier/prettier)
[error] 194-194: Delete ····
(prettier/prettier)
[error] 196-196: Delete ····
(prettier/prettier)
[error] 197-197: Delete ··
(prettier/prettier)
[error] 198-198: Delete ····
(prettier/prettier)
[error] 199-199: Delete ····
(prettier/prettier)
[error] 200-200: Delete ··
(prettier/prettier)
🔇 Additional comments (2)
scripts/markdown/check-markdown.js (2)
4-4: Add 'p-limit' to the project's dependencies
The p-limit library is being used but is not listed in the project's dependencies.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-4: scripts/markdown/check-markdown.js#L3-L4
Added lines #L3 - L4 were not covered by tests
7-8: LGTM: Robust CONCURRENCY_LIMIT implementation
The implementation includes proper parsing, validation, and fallback handling for the concurrency limit configuration.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 7-8: scripts/markdown/check-markdown.js#L7-L8
Added lines #L7 - L8 were not covered by tests
scripts/markdown/check-markdown.js
Outdated
| return; | ||
| try { | ||
| // Read directory contents synchronously for predictability | ||
| const files = fs.readdirSync(folderPath); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using async file operations consistently
The implementation mixes synchronous (readdirSync, statSync, readFileSync) and asynchronous operations. This can impact performance and block the event loop. Consider using the async versions consistently:
-const files = fs.readdirSync(folderPath);
+const files = await fs.promises.readdir(folderPath);
-const stats = fs.statSync(filePath);
+const stats = await fs.promises.stat(filePath);
-const fileContent = fs.readFileSync(filePath, 'utf-8');
+const fileContent = await fs.promises.readFile(filePath, 'utf-8');Also applies to: 130-130, 142-142
🧰 Tools
🪛 eslint
[error] 116-116: Delete ····
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 116-116: scripts/markdown/check-markdown.js#L116
Added line #L116 was not covered by tests
scripts/markdown/check-markdown.js
Outdated
| const errors = validateFunction(frontmatter); | ||
| if (errors.length > 0) { | ||
| console.log(`Errors in file ${relativeFilePath}:`); | ||
| allErrors.push(`File ${relativeFilePath}: ${errors.join('; ')}`); | ||
| } |
There was a problem hiding this comment.
Improve error handling consistency
The validation functions return null when there are no errors, but the code assumes an array is returned. This can lead to runtime errors.
-if (errors.length > 0) {
+if (errors && errors.length > 0) {Also, consider collecting all errors before exiting:
-if (errors && errors.length > 0) {
- console.log(`Errors in file ${relativeFilePath}:`);
- allErrors.push(`File ${relativeFilePath}: ${errors.join('; ')}`);
-}
+if (errors) {
+ const errorMessage = `File ${relativeFilePath}: ${errors.join('; ')}`;
+ console.log('Errors in file:', errorMessage);
+ allErrors.push(errorMessage);
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 145-145: Replace ························ with ············
(prettier/prettier)
[error] 146-146: Replace ························ with ············
(prettier/prettier)
[error] 147-147: Delete ··············
(prettier/prettier)
[error] 148-148: Replace ···························· with ··············
(prettier/prettier)
[error] 149-149: Delete ············
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 145-148: scripts/markdown/check-markdown.js#L145-L148
Added lines #L145 - L148 were not covered by tests
scripts/markdown/check-markdown.js
Outdated
| async function checkMarkdownFiles( | ||
| folderPath, | ||
| validateFunction, | ||
| relativePath = '', | ||
| ) { | ||
| const limit = pLimit(CONCURRENCY_LIMIT); | ||
| const allErrors = []; | ||
|
|
||
| fs.stat(filePath, (err, stats) => { | ||
| if (err) { | ||
| console.error('Error reading file stats:', err); | ||
| return; | ||
| try { | ||
| // Read directory contents synchronously for predictability | ||
| const files = fs.readdirSync(folderPath); | ||
|
|
||
| // Process files with concurrency control | ||
| const filePromises = files.map((file) => | ||
| limit(async () => { | ||
| const filePath = path.join(folderPath, file); | ||
| const relativeFilePath = path.join(relativePath, file); | ||
|
|
||
| // Skip the folder 'docs/reference/specification' | ||
| if (relativeFilePath.includes('reference/specification')) { | ||
| return []; | ||
| } | ||
|
|
||
| // Get file stats | ||
| const stats = fs.statSync(filePath); | ||
|
|
||
| // Recurse if directory, otherwise validate markdown file | ||
| if (stats.isDirectory()) { | ||
| checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | ||
| const subdirErrors = await checkMarkdownFiles( | ||
| filePath, | ||
| validateFunction, | ||
| relativeFilePath, | ||
| ); | ||
| allErrors.push(...subdirErrors); | ||
| } 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; | ||
| try { | ||
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | ||
| const { data: frontmatter } = matter(fileContent); | ||
|
|
||
| const errors = validateFunction(frontmatter); | ||
| if (errors.length > 0) { | ||
| console.log(`Errors in file ${relativeFilePath}:`); | ||
| allErrors.push(`File ${relativeFilePath}: ${errors.join('; ')}`); | ||
| } | ||
| } catch (readError) { | ||
| allErrors.push( | ||
| `Error reading file ${relativeFilePath}: ${readError.message}`, | ||
| ); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| return []; | ||
| }), | ||
| ); | ||
|
|
||
| // Wait for all file promises to complete | ||
| await Promise.all(filePromises); | ||
|
|
||
| return allErrors; | ||
| } catch (error) { | ||
| console.error('Error processing files:', error); | ||
| return [`Folder processing error: ${error.message}`]; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Test coverage needed for markdown validation functionality
The check-markdown.js script contains critical validation logic but lacks test coverage. Key areas requiring tests:
-
validateBlogsfunction:- Required attributes validation
- Date format validation
- Tags array validation
- Authors array validation
- URL validation for author links
-
validateDocsfunction:- Title validation
- Weight validation
-
checkMarkdownFilesfunction:- Directory traversal
- Concurrent file processing
- Error handling for invalid files
- Skipping 'reference/specification' folder
Suggested test setup:
- Mock
fsandgray-mattermodules - Create sample frontmatter fixtures
- Test both success and error scenarios
- Verify concurrency control with
pLimit
🔗 Analysis chain
Add test coverage for the validation process
The codecov report indicates missing test coverage for the validation logic. Consider adding unit tests to verify:
- Validation of frontmatter attributes
- Error handling for invalid files
- Concurrency control behavior
Would you like me to help create unit tests for this functionality?
Also applies to: 171-201
Let me check the content of test files to see if there's any existing test coverage for the markdown validation functionality.
Let me check if the markdown validation functions are exported and how they're used in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test files
fd -e test.js -e spec.js . tests/ scripts/
# Search for markdown validation related tests
rg -l "validateMarkdown|checkMarkdownFiles" tests/
# Look for any test utilities or helpers
rg -l "jest.mock|describe.*markdown" tests/
Length of output: 1029
Script:
#!/bin/bash
# Check content of test files for any markdown validation related tests
rg -A 5 "describe.*markdown|test.*markdown|validateMarkdown|checkMarkdownFiles" tests/
# Look for any mock implementations of fs or path modules in tests
rg -A 5 "jest.mock.*('fs')|jest.mock.*('path')" tests/
# Check the implementation file for any exported functions that might be tested
rg -l "module.exports.*checkMarkdownFiles|module.exports.*validateMarkdown" scripts/
Length of output: 530
Script:
#!/bin/bash
# Check the content of the markdown validation file
cat scripts/markdown/check-markdown.js
# Find any imports or usage of these validation functions
rg -l "require.*check-markdown|import.*check-markdown" .
# Look for any test setup files that might contain markdown-related mocks
fd -e js . tests/__mocks__/ tests/setup/ 2>/dev/null || true
Length of output: 7385
🧰 Tools
🪛 eslint
[error] 106-110: Replace ⏎····folderPath,⏎····validateFunction,⏎····relativePath·=·'',⏎ with folderPath,·validateFunction,·relativePath·=·''
(prettier/prettier)
[error] 111-111: Delete ··
(prettier/prettier)
[error] 112-112: Delete ··
(prettier/prettier)
[error] 114-114: Delete ··
(prettier/prettier)
[error] 115-115: Delete ····
(prettier/prettier)
[error] 116-116: Delete ····
(prettier/prettier)
[error] 118-118: Delete ····
(prettier/prettier)
[error] 119-119: Delete ····
(prettier/prettier)
[error] 120-120: Delete ······
(prettier/prettier)
[error] 121-121: Replace ················ with ········
(prettier/prettier)
[error] 122-122: Replace ················ with ········
(prettier/prettier)
[error] 124-124: Delete ········
(prettier/prettier)
[error] 125-125: Replace ················ with ········
(prettier/prettier)
[error] 126-126: Replace ···················· with ··········
(prettier/prettier)
[error] 127-127: Delete ········
(prettier/prettier)
[error] 129-129: Replace ················ with ········
(prettier/prettier)
[error] 130-130: Replace ················ with ········
(prettier/prettier)
[error] 132-132: Delete ········
(prettier/prettier)
[error] 133-133: Replace ················ with ········
(prettier/prettier)
[error] 134-138: Replace ····················const·subdirErrors·=·await·checkMarkdownFiles(⏎························filePath,⏎························validateFunction,⏎························relativeFilePath,⏎···················· with ··········const·subdirErrors·=·await·checkMarkdownFiles(filePath,·validateFunction,·relativeFilePath
(prettier/prettier)
[error] 139-139: Delete ··········
(prettier/prettier)
[error] 140-140: Replace ················ with ········
(prettier/prettier)
[error] 141-141: Replace ···················· with ··········
(prettier/prettier)
[error] 142-142: Delete ············
(prettier/prettier)
[error] 143-143: Replace ························ with ············
(prettier/prettier)
[error] 145-145: Replace ························ with ············
(prettier/prettier)
[error] 146-146: Replace ························ with ············
(prettier/prettier)
[error] 147-147: Delete ··············
(prettier/prettier)
[error] 148-148: Replace ···························· with ··············
(prettier/prettier)
[error] 149-149: Delete ············
(prettier/prettier)
[error] 150-150: Replace ···················· with ··········
(prettier/prettier)
[error] 151-153: Replace ························allErrors.push(⏎····························Error·reading·file·${relativeFilePath}:·${readError.message},⏎························ with ············allErrors.push(Error·reading·file·${relativeFilePath}:·${readError.message}``
(prettier/prettier)
[error] 154-154: Replace ···················· with ··········
(prettier/prettier)
[error] 155-155: Replace ················ with ········
(prettier/prettier)
[error] 157-157: Delete ········
(prettier/prettier)
[error] 158-158: Replace ············}), with ······})
(prettier/prettier)
[error] 159-159: Replace ········ with ····
(prettier/prettier)
[error] 161-161: Delete ····
(prettier/prettier)
[error] 162-162: Delete ····
(prettier/prettier)
[error] 164-164: Replace ········ with ····
(prettier/prettier)
[error] 165-165: Replace ···· with ··
(prettier/prettier)
[error] 166-166: Replace ········ with ····
(prettier/prettier)
[error] 167-167: Delete ····
(prettier/prettier)
[error] 168-168: Replace ···· with ··
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 111-112: scripts/markdown/check-markdown.js#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 114-114: scripts/markdown/check-markdown.js#L114
Added line #L114 was not covered by tests
[warning] 116-116: scripts/markdown/check-markdown.js#L116
Added line #L116 was not covered by tests
[warning] 119-122: scripts/markdown/check-markdown.js#L119-L122
Added lines #L119 - L122 were not covered by tests
[warning] 125-126: scripts/markdown/check-markdown.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 130-130: scripts/markdown/check-markdown.js#L130
Added line #L130 was not covered by tests
[warning] 134-134: scripts/markdown/check-markdown.js#L134
Added line #L134 was not covered by tests
[warning] 139-139: scripts/markdown/check-markdown.js#L139
Added line #L139 was not covered by tests
[warning] 141-143: scripts/markdown/check-markdown.js#L141-L143
Added lines #L141 - L143 were not covered by tests
[warning] 145-148: scripts/markdown/check-markdown.js#L145-L148
Added lines #L145 - L148 were not covered by tests
[warning] 151-151: scripts/markdown/check-markdown.js#L151
Added line #L151 was not covered by tests
[warning] 157-157: scripts/markdown/check-markdown.js#L157
Added line #L157 was not covered by tests
[warning] 162-162: scripts/markdown/check-markdown.js#L162
Added line #L162 was not covered by tests
[warning] 164-164: scripts/markdown/check-markdown.js#L164
Added line #L164 was not covered by tests
[warning] 166-167: scripts/markdown/check-markdown.js#L166-L167
Added lines #L166 - L167 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
scripts/markdown/check-markdown.js (3)
7-11: Consider adding input validation for CONCURRENCY_LIMITWhile the code handles non-integer and negative values, it should also handle:
- Very large numbers that could cause performance issues
- Non-numeric strings that would result in NaN
const parsedLimit = process.env.CONCURRENCY_LIMIT ? parseInt(process.env.CONCURRENCY_LIMIT, 10) : 10; const CONCURRENCY_LIMIT = - Number.isInteger(parsedLimit) && parsedLimit > 0 ? parsedLimit : 10; + Number.isInteger(parsedLimit) && parsedLimit > 0 && parsedLimit <= 100 ? parsedLimit : 10;🧰 Tools
🪛 eslint
[error] 7-9: Replace
⏎··?·parseInt(process.env.CONCURRENCY_LIMIT,·10)⏎·with·?·parseInt(process.env.CONCURRENCY_LIMIT,·10)(prettier/prettier)
[error] 10-11: Delete
⏎·(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
[warning] 11-11: scripts/markdown/check-markdown.js#L11
Added line #L11 was not covered by tests
19-24: Optimize URL validation to avoid object creationThe current implementation creates a new URL object even when validation succeeds. Consider using URL.canParse() for better performance.
function isValidURL(str) { - try { - new URL(str); - return true; - } catch (err) { - return false; - } + return URL.canParse(str); }🧰 Tools
🪛 eslint
[error] 20-20: Do not use 'new' for side effects.
(no-new)
🪛 GitHub Check: codecov/patch
[warning] 19-21: scripts/markdown/check-markdown.js#L19-L21
Added lines #L19 - L21 were not covered by tests
[warning] 23-23: scripts/markdown/check-markdown.js#L23
Added line #L23 was not covered by tests
87-87: Return empty array instead of null for consistencyBoth validation functions return null when there are no errors, but returning an empty array would be more consistent and avoid null checks.
- return errors.length ? errors : null; + return errors;Also applies to: 112-112
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-87: scripts/markdown/check-markdown.js#L87
Added line #L87 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/markdown/check-markdown.js(4 hunks)
🧰 Additional context used
🪛 eslint
scripts/markdown/check-markdown.js
[error] 7-9: Replace ⏎··?·parseInt(process.env.CONCURRENCY_LIMIT,·10)⏎· with ·?·parseInt(process.env.CONCURRENCY_LIMIT,·10)
(prettier/prettier)
[error] 10-11: Delete ⏎·
(prettier/prettier)
[error] 20-20: Do not use 'new' for side effects.
(no-new)
[error] 34-41: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 76-78: Replace ⏎············Invalid·URL·for·author·at·index·${index}:·${author.link},⏎·········· with Invalid·URL·for·author·at·index·${index}:·${author.link}
(prettier/prettier)
[error] 105-108: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 121-125: Replace ⏎··folderPath,⏎··validateFunction,⏎··relativePath·=·'',⏎ with folderPath,·validateFunction,·relativePath·=·''
(prettier/prettier)
[error] 144-144: 'err' is already declared in the upper scope on line 128 column 27.
(no-shadow)
[error] 165-165: Delete ,
(prettier/prettier)
🪛 GitHub Check: codecov/patch
scripts/markdown/check-markdown.js
[warning] 4-4: scripts/markdown/check-markdown.js#L4
Added line #L4 was not covered by tests
[warning] 7-7: scripts/markdown/check-markdown.js#L7
Added line #L7 was not covered by tests
[warning] 11-11: scripts/markdown/check-markdown.js#L11
Added line #L11 was not covered by tests
[warning] 19-21: scripts/markdown/check-markdown.js#L19-L21
Added lines #L19 - L21 were not covered by tests
[warning] 23-23: scripts/markdown/check-markdown.js#L23
Added line #L23 was not covered by tests
[warning] 34-34: scripts/markdown/check-markdown.js#L34
Added line #L34 was not covered by tests
[warning] 42-42: scripts/markdown/check-markdown.js#L42
Added line #L42 was not covered by tests
[warning] 45-47: scripts/markdown/check-markdown.js#L45-L47
Added lines #L45 - L47 were not covered by tests
[warning] 52-53: scripts/markdown/check-markdown.js#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 57-58: scripts/markdown/check-markdown.js#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 62-63: scripts/markdown/check-markdown.js#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-69: scripts/markdown/check-markdown.js#L67-L69
Added lines #L67 - L69 were not covered by tests
[warning] 71-73: scripts/markdown/check-markdown.js#L71-L73
Added lines #L71 - L73 were not covered by tests
[warning] 75-76: scripts/markdown/check-markdown.js#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 80-81: scripts/markdown/check-markdown.js#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-87: scripts/markdown/check-markdown.js#L87
Added line #L87 was not covered by tests
[warning] 97-97: scripts/markdown/check-markdown.js#L97
Added line #L97 was not covered by tests
[warning] 100-101: scripts/markdown/check-markdown.js#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 105-105: scripts/markdown/check-markdown.js#L105
Added line #L105 was not covered by tests
[warning] 109-109: scripts/markdown/check-markdown.js#L109
Added line #L109 was not covered by tests
[warning] 112-112: scripts/markdown/check-markdown.js#L112
Added line #L112 was not covered by tests
[warning] 126-126: scripts/markdown/check-markdown.js#L126
Added line #L126 was not covered by tests
[warning] 128-131: scripts/markdown/check-markdown.js#L128-L131
Added lines #L128 - L131 were not covered by tests
[warning] 134-137: scripts/markdown/check-markdown.js#L134-L137
Added lines #L134 - L137 were not covered by tests
[warning] 140-141: scripts/markdown/check-markdown.js#L140-L141
Added lines #L140 - L141 were not covered by tests
[warning] 144-147: scripts/markdown/check-markdown.js#L144-L147
Added lines #L144 - L147 were not covered by tests
[warning] 151-155: scripts/markdown/check-markdown.js#L151-L155
Added lines #L151 - L155 were not covered by tests
[warning] 157-161: scripts/markdown/check-markdown.js#L157-L161
Added lines #L157 - L161 were not covered by tests
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (3)
scripts/markdown/check-markdown.js (3)
4-4: Add 'p-limit' to the project's dependencies
The p-limit library is being used but is not listed in the project's dependencies.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 4-4: scripts/markdown/check-markdown.js#L4
Added line #L4 was not covered by tests
46-47:
Use Object.hasOwn() instead of hasOwnProperty
Direct use of hasOwnProperty is discouraged as it can be overridden.
- if (!frontmatter.hasOwnProperty(attr)) {
+ if (!Object.hasOwn(frontmatter, attr)) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
🪛 GitHub Check: codecov/patch
[warning] 45-47: scripts/markdown/check-markdown.js#L45-L47
Added lines #L45 - L47 were not covered by tests
Line range hint 1-167: Add test coverage for the validation functionality
The code lacks test coverage as indicated by the codecov report. Consider adding tests for:
- URL validation
- Blog frontmatter validation
- Docs frontmatter validation
- File processing with concurrency
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
[error] 34-41: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 46-46: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 76-78: Replace ⏎············Invalid·URL·for·author·at·index·${index}:·${author.link},⏎·········· with Invalid·URL·for·author·at·index·${index}:·${author.link}
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 34-34: scripts/markdown/check-markdown.js#L34
Added line #L34 was not covered by tests
[warning] 42-42: scripts/markdown/check-markdown.js#L42
Added line #L42 was not covered by tests
[warning] 45-47: scripts/markdown/check-markdown.js#L45-L47
Added lines #L45 - L47 were not covered by tests
[warning] 52-53: scripts/markdown/check-markdown.js#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 57-58: scripts/markdown/check-markdown.js#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 62-63: scripts/markdown/check-markdown.js#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-69: scripts/markdown/check-markdown.js#L67-L69
Added lines #L67 - L69 were not covered by tests
[warning] 71-73: scripts/markdown/check-markdown.js#L71-L73
Added lines #L71 - L73 were not covered by tests
[warning] 75-76: scripts/markdown/check-markdown.js#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 80-81: scripts/markdown/check-markdown.js#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-87: scripts/markdown/check-markdown.js#L87
Added line #L87 was not covered by tests
| 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); | ||
| files.map((file) => | ||
| limit(() => { | ||
| const filePath = path.join(folderPath, file); | ||
| const relativeFilePath = path.join(relativePath, file); | ||
|
|
||
| // Skip the folder 'docs/reference/specification' | ||
| if (relativeFilePath.includes('reference/specification')) { | ||
| return; | ||
| } | ||
| // Skip auto-generated specification documentation | ||
| if (relativeFilePath.includes('reference/specification')) { | ||
| 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; | ||
| } | ||
| } | ||
| }); | ||
| 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; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Promise-based API for consistent async operations
The current implementation mixes callbacks and async/await. Consider using the Promise-based fs API consistently.
- fs.readdir(folderPath, (err, files) => {
- if (err) {
- console.error('Error reading directory:', err);
- return;
- }
+ try {
+ const files = await fs.promises.readdir(folderPath);
+ await Promise.all(
+ files.map((file) =>
+ limit(async () => {
const filePath = path.join(folderPath, file);
const relativeFilePath = path.join(relativePath, file);
if (relativeFilePath.includes('reference/specification')) {
return;
}
- fs.stat(filePath, (err, stats) => {
- if (err) {
- console.error('Error reading file stats:', err);
- return;
- }
+ try {
+ const stats = await fs.promises.stat(filePath);
if (stats.isDirectory()) {
- checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
+ await checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
} else if (path.extname(file) === '.md') {
- const fileContent = fs.readFileSync(filePath, 'utf-8');
+ const fileContent = await fs.promises.readFile(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;
}
}
- });
+ } catch (error) {
+ console.error(`Error processing file ${relativeFilePath}:`, error);
+ }
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error reading directory:', 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.
| 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); | |
| files.map((file) => | |
| limit(() => { | |
| const filePath = path.join(folderPath, file); | |
| const relativeFilePath = path.join(relativePath, file); | |
| // Skip the folder 'docs/reference/specification' | |
| if (relativeFilePath.includes('reference/specification')) { | |
| return; | |
| } | |
| // Skip auto-generated specification documentation | |
| if (relativeFilePath.includes('reference/specification')) { | |
| 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; | |
| } | |
| } | |
| }); | |
| 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; | |
| } | |
| } | |
| }); | |
| }); | |
| }), | |
| ); | |
| }); | |
| try { | |
| const files = await fs.promises.readdir(folderPath); | |
| await Promise.all( | |
| files.map((file) => | |
| limit(async () => { | |
| const filePath = path.join(folderPath, file); | |
| const relativeFilePath = path.join(relativePath, file); | |
| if (relativeFilePath.includes('reference/specification')) { | |
| return; | |
| } | |
| try { | |
| const stats = await fs.promises.stat(filePath); | |
| if (stats.isDirectory()) { | |
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = await fs.promises.readFile(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; | |
| } | |
| } | |
| } catch (error) { | |
| console.error(`Error processing file ${relativeFilePath}:`, error); | |
| } | |
| }) | |
| ) | |
| ); | |
| } catch (error) { | |
| console.error('Error reading directory:', error); | |
| } |
🧰 Tools
🪛 eslint
[error] 144-144: 'err' is already declared in the upper scope on line 128 column 27.
(no-shadow)
[error] 165-165: Delete ,
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 128-131: scripts/markdown/check-markdown.js#L128-L131
Added lines #L128 - L131 were not covered by tests
[warning] 134-137: scripts/markdown/check-markdown.js#L134-L137
Added lines #L134 - L137 were not covered by tests
[warning] 140-141: scripts/markdown/check-markdown.js#L140-L141
Added lines #L140 - L141 were not covered by tests
[warning] 144-147: scripts/markdown/check-markdown.js#L144-L147
Added lines #L144 - L147 were not covered by tests
[warning] 151-155: scripts/markdown/check-markdown.js#L151-L155
Added lines #L151 - L155 were not covered by tests
[warning] 157-161: scripts/markdown/check-markdown.js#L157-L161
Added lines #L157 - L161 were not covered by tests
Summary
Add concurrency control to markdown file validation script using
p-limitto manage resource usage and improve performance.Fixes
Changes
CONCURRENCY_LIMITwith a default of 10 concurrent file processing operationsCONCURRENCY_LIMITp-limitto control the number of concurrent file processing tasksMotivation
Implementation Details
p-limitlibrary to manage concurrencyCONCURRENCY_LIMITenvironment variableEnvironment Configuration
Users can customize concurrency by setting the
CONCURRENCY_LIMITenvironment variable: