feat: added concurrency limit in markdown files processing to improve… #3475
Conversation
Aditya0732
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis pull request introduces concurrency management for markdown file processing in the AsyncAPI website project. The changes involve adding a new Changes
Sequence DiagramsequenceDiagram
participant Main as Main Script
participant Limit as Concurrency Limiter
participant Validator as Markdown Validator
Main->>Limit: Get Concurrency Limit
Limit-->>Main: Return Limit
Main->>Validator: Start Markdown Processing
Validator->>Limit: Request Processing Slot
Limit-->>Validator: Allow Processing
Validator->>Validator: Validate Markdown Files
Validator-->>Main: Return Validation Results
Possibly related issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3475--asyncapi-website.netlify.app/ |
Aditya0732
changed the title
![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
🧹 Nitpick comments (9)
tests/markdown/check-markdown.test.js (4)
40-40: Avoid using thedeleteoperator onprocess.envproperties.Using
deleteonprocess.envproperties can impact performance and is discouraged. Instead, assignundefinedto the property.Apply this diff to fix the issue:
- delete process.env.MARKDOWN_CONCURRENCY_LIMIT; + process.env.MARKDOWN_CONCURRENCY_LIMIT = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
85-86: Remove unnecessary trailing commas in theexpectarray.Trailing commas in array elements can cause issues and are flagged by the linter.
Apply this diff to fix the issue:
expect(errors).toEqual( expect.arrayContaining([ 'Author at index 0 is missing a photo', 'Author at index 1 is missing a name', 'Invalid URL for author at index 2: not-a-url' - ]), + ]) );🧰 Tools
🪛 eslint
[error] 85-85: Delete
,(prettier/prettier)
[error] 86-86: Delete
,(prettier/prettier)
94-97: Format theexpectarray correctly to match linter expectations.The linter recommends formatting the array without unnecessary line breaks and commas.
Apply this diff to fix the issue:
expect(errors).toEqual( - expect.arrayContaining([ - 'Title is missing or not a string', - 'Weight is missing or not a number', - ]), + expect.arrayContaining(['Title is missing or not a string', 'Weight is missing or not a number']) );🧰 Tools
🪛 eslint
[error] 94-97: Replace
⏎········'Title·is·missing·or·not·a·string',⏎········'Weight·is·missing·or·not·a·number',⏎······]),with'Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
170-171: Combine chained method calls into a single line for clarity.The linter recommends combining chained method calls when possible.
Apply this diff to fix the issue:
- const mockReadFile = jest - .spyOn(fs, 'readFile') + const mockReadFile = jest.spyOn(fs, 'readFile')🧰 Tools
🪛 eslint
[error] 169-171: Replace
⏎······.spyOn(fs,·'readFile')⏎······with.spyOn(fs,·'readFile')(prettier/prettier)
scripts/markdown/check-markdown.js (5)
23-25: Format template strings properly to satisfy linter rules.The linter flags unnecessary line breaks in template strings.
Apply this diff to fix the issue:
console.warn( - `Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Falling back to default of 10.`, + `Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Falling back to default of 10.` );🧰 Tools
🪛 eslint
[error] 23-25: Replace
⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.,⏎····withInvalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.(prettier/prettier)
32-32: Remove the unnecessary comma in the console warning.The trailing comma in the console warning is unnecessary and is flagged by the linter.
Apply this diff to fix the issue:
); - ); + )🧰 Tools
🪛 eslint
[error] 32-32: Delete
,(prettier/prettier)
170-171: Combine chained method calls for readability.The linter suggests combining chained method calls into a single line.
Apply this diff to fix the issue:
- const mockReadFile = jest - .spyOn(fs, 'readFile') + const mockReadFile = jest.spyOn(fs, 'readFile')🧰 Tools
🪛 eslint
[error] 170-175: Replace
⏎··········filePath,⏎··········validateFunction,⏎··········relativeFilePath,⏎··········limit,⏎········withfilePath,·validateFunction,·relativeFilePath,·limit(prettier/prettier)
212-212: Remove the unnecessary trailing comma in the function call array.Trailing commas in the last element of an array or function argument list are unnecessary and can be removed.
Apply this diff to fix the issue:
await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs, '', limit), - checkMarkdownFiles(blogsFolderPath, validateBlogs, '', limit), + checkMarkdownFiles(blogsFolderPath, validateBlogs, '', limit) ]);🧰 Tools
🪛 eslint
[error] 212-212: Delete
,(prettier/prettier)
231-231: Remove the unnecessary trailing comma in the exported object.Trailing commas in object literals can be omitted for cleaner code.
Apply this diff to fix the issue:
module.exports = { validateBlogs, validateDocs, checkMarkdownFiles, main, isValidURL, - getConcurrencyLimit, + getConcurrencyLimit };🧰 Tools
🪛 eslint
[error] 231-231: Delete
,(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
scripts/markdown/check-markdown.js
[error] 23-25: Replace ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.,⏎···· with Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.
(prettier/prettier)
[error] 32-32: Delete ,
(prettier/prettier)
[error] 47-47: Do not use 'new' for side effects.
(no-new)
[error] 61-68: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 73-73: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 103-105: Replace ⏎············Invalid·URL·for·author·at·index·${index}:·${author.link},⏎·········· with Invalid·URL·for·author·at·index·${index}:·${author.link}
(prettier/prettier)
[error] 132-135: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 149-154: Replace ⏎··folderPath,⏎··validateFunction,⏎··relativePath·=·'',⏎··limit,⏎ with folderPath,·validateFunction,·relativePath·=·'',·limit
(prettier/prettier)
[error] 152-152: Default parameters should be last.
(default-param-last)
[error] 170-175: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········relativeFilePath,⏎··········limit,⏎········ with filePath,·validateFunction,·relativeFilePath,·limit
(prettier/prettier)
[error] 212-212: Delete ,
(prettier/prettier)
[error] 231-231: Delete ,
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 11-11: Delete ,
(prettier/prettier)
[error] 73-77: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 85-85: Delete ,
(prettier/prettier)
[error] 86-86: Delete ,
(prettier/prettier)
[error] 94-97: Replace ⏎········'Title·is·missing·or·not·a·string',⏎········'Weight·is·missing·or·not·a·number',⏎······]), with 'Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 102-105: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 110-112: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎···· with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 123-123: Delete ,
(prettier/prettier)
[error] 131-131: Delete ,
(prettier/prettier)
[error] 138-140: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 148-151: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 158-160: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 169-171: Replace ⏎······.spyOn(fs,·'readFile')⏎······ with .spyOn(fs,·'readFile')
(prettier/prettier)
[error] 173-175: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 176-179: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 191-194: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 73-73: 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)
tests/markdown/check-markdown.test.js
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (8)
tests/markdown/check-markdown.test.js (3)
204-205: Ensure proper usage of the concurrency limiter in tests.
The concurrency limiter pLimit should be initialized correctly in test cases to mirror the actual usage.
Confirm that the concurrency limiter is used consistently in both the main application code and the tests.
152-152: 🛠️ Refactor suggestion
Adjust parameter order: move default parameters to the end.
Default parameters should be last according to best practices and linter rules.
Apply this diff to fix the issue:
async function checkMarkdownFiles(
folderPath,
validateFunction,
limit,
- relativePath = ''
+ relativePath = ''
) {Update the function calls accordingly.
Likely invalid or redundant comment.
73-73: 🛠️ Refactor suggestion
Use Object.hasOwn instead of hasOwnProperty for safer property checks.
Accessing hasOwnProperty directly on objects can lead to unexpected results if the object has a property with the same name. Use Object.hasOwn for a more reliable check.
Apply this diff to fix the issue:
- if (!frontmatter.hasOwnProperty(attr)) {
+ if (!Object.hasOwn(frontmatter, attr)) {⛔ Skipped due to learnings
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.
🧰 Tools
🪛 eslint
[error] 73-77: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
package.json (2)
93-93: Confirm the addition of p-limit in dependencies.
The p-limit library is crucial for concurrency control. Ensure that it is added to dependencies (not devDependencies) since it's required at runtime.
155-155: Verify the necessity of fast-xml-parser in devDependencies.
The fast-xml-parser was removed and re-added to devDependencies. Confirm whether this package is necessary for development and if the version is correct.
Run the following script to check for its usage:
✅ Verification successful
fast-xml-parser is only used in tests for RSS feed validation
The package is correctly placed in devDependencies as it's only used in tests/build-rss.test.js to validate the XML output of the RSS feed generator. The actual RSS feed generation uses jgexml/json2xml. The current version (4.5.0) is appropriate as it's a stable release.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'fast-xml-parser' in the project.
# Search for 'fast-xml-parser' in the source code
rg 'fast-xml-parser'
Length of output: 361
Script:
#!/bin/bash
# Check the content of the test file to understand the usage
cat tests/build-rss.test.js
Length of output: 5911
Script:
#!/bin/bash
# Check the RSS build script to understand how the XML parser is used
cat scripts/build-rss.js
Length of output: 3431
scripts/markdown/check-markdown.js (3)
4-4: Ensure that p-limit is correctly required and used.
The p-limit library is imported and should be used appropriately to control concurrency.
152-152: 🛠️ Refactor suggestion
Place default parameters after required parameters.
According to best practices, default parameters should be listed after all required parameters.
Apply this diff to fix the issue:
async function checkMarkdownFiles(
folderPath,
validateFunction,
limit,
- relativePath = ''
+ relativePath = ''
) {Update all function calls accordingly to reflect the parameter order change.
Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 152-152: Default parameters should be last.
(default-param-last)
73-73: 🛠️ Refactor suggestion
Use Object.hasOwn instead of hasOwnProperty for safer checks.
Directly calling hasOwnProperty on an object can be unreliable. Use Object.hasOwn or Object.prototype.hasOwnProperty.call instead.
Apply this diff to fix the issue:
- if (!frontmatter.hasOwnProperty(attr)) {
+ if (!Object.hasOwn(frontmatter, attr)) {⛔ Skipped due to learnings
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.
🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: 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] 73-73: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
| validateBlogs, | ||
| validateDocs, | ||
| checkMarkdownFiles, | ||
| getConcurrencyLimit, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unnecessary trailing comma in the import statement.
The trailing comma after the last property in the destructuring assignment is unnecessary and is flagged by the linter.
Apply this diff to fix the issue:
const {
isValidURL,
main,
validateBlogs,
validateDocs,
checkMarkdownFiles,
- getConcurrencyLimit,
+ getConcurrencyLimit
} = require('../../scripts/markdown/check-markdown');📝 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.
| getConcurrencyLimit, | |
| const { | |
| isValidURL, | |
| main, | |
| validateBlogs, | |
| validateDocs, | |
| checkMarkdownFiles, | |
| getConcurrencyLimit | |
| } = require('../../scripts/markdown/check-markdown'); |
🧰 Tools
🪛 eslint
[error] 11-11: Delete ,
(prettier/prettier)
scripts/markdown/check-markdown.js
Outdated
| return false; | ||
| } | ||
| try { | ||
| new URL(str); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using new for side effects without using the created object.
Using new URL(str) without assigning the result is unnecessary. To validate the URL, you can use URL(str) directly or assign it to a variable.
Apply this diff to fix the issue:
- new URL(str);
+ URL(str);Or assign it to a variable if needed:
- new URL(str);
+ const url = new URL(str);📝 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.
| new URL(str); | |
| URL(str); |
🧰 Tools
🪛 eslint
[error] 47-47: Do not use 'new' for side effects.
(no-new)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/markdown/check-markdown.js (1)
10-38: LGTM! Consider standardizing error message format.The
getConcurrencyLimitfunction is well-implemented with proper validation and error handling.Consider standardizing the error message format:
- console.warn( - `Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Falling back to default of 10.` - ); + console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT '${envLimit}' - falling back to default of 10`);🧰 Tools
🪛 eslint
[error] 23-25: Replace
⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.⏎····withInvalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.(prettier/prettier)
tests/markdown/check-markdown.test.js (1)
98-111: Consider adding concurrent execution test.While the current tests are good, consider adding a test that verifies the concurrency limit is actually enforced during file processing.
Here's a suggested test:
it('respects concurrency limit during file processing', async () => { const processingTimes = []; const mockValidateFunction = jest.fn().mockImplementation(() => { const startTime = Date.now(); processingTimes.push(startTime); return null; }); // Create multiple test files for (let i = 0; i < 20; i++) { await fs.writeFile( path.join(tempDir, `test${i}.md`), '---\ntitle: Test\n---' ); } const limit = pLimit(5); // Set limit to 5 await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); // Group processing times by 5 (our limit) and verify gaps between groups const sortedTimes = processingTimes.sort(); const groups = []; for (let i = 0; i < sortedTimes.length; i += 5) { groups.push(sortedTimes.slice(i, i + 5)); } // Verify that each group of 5 started processing together groups.forEach(group => { const groupSpread = Math.max(...group) - Math.min(...group); expect(groupSpread).toBeLessThan(50); // Should start within 50ms of each other }); });🧰 Tools
🪛 eslint
[error] 99-102: Replace
⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎····withpath.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``(prettier/prettier)
[error] 107-109: Replace
⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎····withexpect.stringContaining('Errors·in·file·invalid.md:')(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 73-77: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 85-85: Delete ,
(prettier/prettier)
[error] 94-94: Replace ········ with ······
(prettier/prettier)
[error] 99-102: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 107-109: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎···· with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 120-120: Delete ,
(prettier/prettier)
[error] 128-128: Delete ,
(prettier/prettier)
[error] 135-137: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 145-148: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 155-157: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 168-170: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 171-174: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 186-189: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 23-25: Replace ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.⏎···· with Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Falling·back·to·default·of·10.
(prettier/prettier)
[error] 61-68: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 73-73: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 103-105: Replace ⏎············Invalid·URL·for·author·at·index·${index}:·${author.link},⏎·········· with Invalid·URL·for·author·at·index·${index}:·${author.link}
(prettier/prettier)
[error] 132-135: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 149-154: Replace ⏎··folderPath,⏎··validateFunction,⏎··relativePath·=·'',⏎··limit,⏎ with folderPath,·validateFunction,·relativePath·=·'',·limit
(prettier/prettier)
[error] 152-152: Default parameters should be last.
(default-param-last)
[error] 170-175: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········relativeFilePath,⏎··········limit,⏎········ with filePath,·validateFunction,·relativeFilePath,·limit
(prettier/prettier)
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 73-73: 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 (5)
scripts/markdown/check-markdown.js (3)
46-51: LGTM! URL validation implementation is clean and efficient.
The function correctly implements URL validation without unnecessary object creation.
203-217: LGTM! Clean implementation with proper error handling.
The main function properly initializes the concurrency limiter and handles errors appropriately.
73-74:
Use Object.hasOwn() for safer property checks.
Using hasOwnProperty directly on objects can be unsafe due to potential prototype pollution attacks.
Apply this change:
- if (!frontmatter.hasOwnProperty(attr)) {
+ if (!Object.hasOwn(frontmatter, attr)) {⛔ Skipped due to learnings
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.
🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: 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] 73-73: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
tests/markdown/check-markdown.test.js (2)
20-36: LGTM! Excellent test environment management.
The setup and teardown properly handle environment variables and cleanup.
38-64: LGTM! Comprehensive test coverage for concurrency limit validation.
The tests thoroughly cover default values, invalid inputs, and custom configurations.
scripts/markdown/check-markdown.js
Outdated
| async function checkMarkdownFiles( | ||
| folderPath, | ||
| validateFunction, | ||
| relativePath = '', | ||
| limit, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reorder parameters to follow best practices.
Default parameters should be last in the parameter list.
Apply this change:
-async function checkMarkdownFiles(
- folderPath,
- validateFunction,
- relativePath = '',
- limit,
-) {
+async function checkMarkdownFiles(
+ folderPath,
+ validateFunction,
+ limit,
+ relativePath = '',
+) {Remember to update all call sites accordingly.
📝 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.
| async function checkMarkdownFiles( | |
| folderPath, | |
| validateFunction, | |
| relativePath = '', | |
| limit, | |
| ) { | |
| async function checkMarkdownFiles( | |
| folderPath, | |
| validateFunction, | |
| limit, | |
| relativePath = '', | |
| ) { |
🧰 Tools
🪛 eslint
[error] 149-154: Replace ⏎··folderPath,⏎··validateFunction,⏎··relativePath·=·'',⏎··limit,⏎ with folderPath,·validateFunction,·relativePath·=·'',·limit
(prettier/prettier)
[error] 152-152: Default parameters should be last.
(default-param-last)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/markdown/check-markdown.js (3)
10-36: Enhance warning messages for better clarity.The warning messages could be more informative by including the valid range of values.
- console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Falling back to default of 10.`); + console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); - console.warn( - `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer. Received: ${parsedLimit}. Falling back to default of 10.` - ); + console.warn( + `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` + );
174-183: Add error handling for the limit function.The limit function execution should be wrapped in try-catch to handle potential errors during file processing.
- await limit(async () => { - 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; - } - }); + try { + await limit(async () => { + 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 (error) { + console.error(`Error processing file ${relativeFilePath}:`, error); + throw error; + }
200-205: Add logging for concurrency limit configuration.Consider logging the configured concurrency limit for better debugging and monitoring.
// Get concurrency limit from environment or use default const concurrencyLimit = getConcurrencyLimit(); + console.log(`Configured markdown processing with concurrency limit: ${concurrencyLimit}`); // Create a concurrency limiter const limit = pLimit(concurrencyLimit);tests/markdown/check-markdown.test.js (2)
29-36: Add test for environment variable restoration.Ensure that environment variables are properly restored after each test.
afterEach(async () => { // Restore original environment variables process.env = originalEnv; + + // Verify environment restoration + expect(process.env).toEqual(originalEnv); mockConsoleError.mockRestore(); mockProcessExit.mockRestore(); await fs.rm(tempDir, { recursive: true, force: true }); });
38-101: Add JSDoc documentation for test suites.Consider adding JSDoc comments to describe the test suites and their purposes.
+/** + * Test suite for concurrency limit validation. + * Verifies the behavior of concurrency limits in markdown processing. + */ describe('Concurrency Limit Validation', () => {🧰 Tools
🪛 eslint
[error] 71-71: Replace
resolvewith(resolve)(prettier/prettier)
[error] 71-71: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 75-75: Unary operator '++' used.
(no-plusplus)
[error] 76-79: Unexpected
awaitinside a loop.(no-await-in-loop)
[error] 76-79: Replace
⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········withpath.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'(prettier/prettier)
[error] 93-93: Replace
groupwith(group)(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
scripts/markdown/check-markdown.js
[error] 71-71: 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] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 71-71: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 145-150: Replace ⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎ with folderPath,·validateFunction,·limit,·relativePath·=·''
(prettier/prettier)
[error] 166-171: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········ with filePath,·validateFunction,·limit,·relativeFilePath
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 71-71: Replace resolve with (resolve)
(prettier/prettier)
[error] 71-71: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 75-75: Unary operator '++' used.
(no-plusplus)
[error] 76-79: Unexpected await inside a loop.
(no-await-in-loop)
[error] 76-79: Replace ⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········ with path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 93-93: Replace group with (group)
(prettier/prettier)
[error] 110-114: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 122-122: Delete ,
(prettier/prettier)
[error] 131-131: Delete ··
(prettier/prettier)
[error] 136-139: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 144-146: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎···· with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 157-157: Delete ,
(prettier/prettier)
[error] 165-165: Delete ,
(prettier/prettier)
[error] 172-174: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 182-185: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 192-194: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 205-207: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 208-211: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 223-226: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 243-243: Insert ⏎
(prettier/prettier)
🔇 Additional comments (1)
scripts/markdown/check-markdown.js (1)
44-49: LGTM! Clean and efficient URL validation.
The implementation correctly uses the URL constructor for validation without unnecessary object creation.
scripts/markdown/check-markdown.js
Outdated
| if (!frontmatter.hasOwnProperty(attr)) { | ||
| errors.push(`${attr} is missing`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() instead of hasOwnProperty.
Using hasOwnProperty directly on objects can be unsafe as it might be overridden. Use Object.hasOwn() instead.
- 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] 71-71: 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] 71-71: Do not access Object.prototype method 'hasOwnProperty' from target object.
(no-prototype-builtins)
| it('respects concurrency limit during file processing', async () => { | ||
| const processingTimes = []; | ||
| const mockValidateFunction = jest.fn().mockImplementation(() => { | ||
| const startTime = Date.now(); | ||
| processingTimes.push(startTime); | ||
| // Simulate some processing time | ||
| return new Promise(resolve => setTimeout(resolve, 50)); | ||
| }); | ||
|
|
||
| // Create multiple test files | ||
| for (let i = 0; i < 20; i++) { | ||
| await fs.writeFile( | ||
| path.join(tempDir, `test${i}.md`), | ||
| '---\ntitle: Test\n---' | ||
| ); | ||
| } | ||
|
|
||
| const limit = pLimit(5); // Set limit to 5 | ||
| await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); | ||
|
|
||
| // Group processing times by 5 (our limit) and verify gaps between groups | ||
| const sortedTimes = processingTimes.sort(); | ||
| const groups = []; | ||
| for (let i = 0; i < sortedTimes.length; i += 5) { | ||
| groups.push(sortedTimes.slice(i, i + 5)); | ||
| } | ||
|
|
||
| // Verify that each group of 5 started processing together | ||
| groups.forEach(group => { | ||
| const groupSpread = Math.max(...group) - Math.min(...group); | ||
| expect(groupSpread).toBeLessThan(50); // Should start within 50ms of each other | ||
| }); | ||
|
|
||
| // Verify that the mock validate function was called for all files | ||
| expect(mockValidateFunction).toHaveBeenCalledTimes(20); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve reliability of concurrent processing test.
The current test might be flaky due to timing assumptions. Consider making it more robust:
- const groupSpread = Math.max(...group) - Math.min(...group);
- expect(groupSpread).toBeLessThan(50); // Should start within 50ms of each other
+ // Track concurrent executions instead of timing
+ const concurrentExecutions = mockValidateFunction.mock.calls.filter(
+ call => Math.abs(call[0] - group[0]) < 10
+ ).length;
+ expect(concurrentExecutions).toBeLessThanOrEqual(5); // Should not exceed our limitCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 71-71: Replace resolve with (resolve)
(prettier/prettier)
[error] 71-71: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 75-75: Unary operator '++' used.
(no-plusplus)
[error] 76-79: Unexpected await inside a loop.
(no-await-in-loop)
[error] 76-79: Replace ⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········ with path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 93-93: Replace group with (group)
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/markdown/check-markdown.js (1)
10-36: Consider extracting the magic number as a constant.The default value of 10 is repeated multiple times in the code. Consider extracting it as a named constant at the top of the file for better maintainability.
+const DEFAULT_CONCURRENCY_LIMIT = 10; + function getConcurrencyLimit() { const envLimit = process.env.MARKDOWN_CONCURRENCY_LIMIT; if (envLimit === undefined) { - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } const parsedLimit = parseInt(envLimit, 10); if (Number.isNaN(parsedLimit)) { - console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); - return 10; + console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.`); + return DEFAULT_CONCURRENCY_LIMIT; } if (parsedLimit <= 0) { console.warn( - `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` + `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.` ); - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } return parsedLimit; }🧰 Tools
🪛 eslint
[error] 23-23: Replace
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.with⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····(prettier/prettier)
tests/markdown/check-markdown.test.js (1)
176-221: Consider adding test for process.exitCode.While the error handling tests are comprehensive, consider adding a test to verify that
process.exitCodeis set to 1 when validation errors occur.it('logs error to console when an error occurs in checkMarkdownFiles', async () => { const invalidFolderPath = path.join(tempDir, 'non-existent-folder'); await expect( checkMarkdownFiles(invalidFolderPath, validateBlogs, '', pLimit(10)), ).rejects.toThrow('ENOENT'); expect(mockConsoleError.mock.calls[0][0]).toContain('Error in directory'); }); + + it('sets process.exitCode when validation errors occur', async () => { + const originalExitCode = process.exitCode; + await fs.writeFile( + path.join(tempDir, 'invalid.md'), + '---\ntitle: Invalid Blog\n---' + ); + + await checkMarkdownFiles(tempDir, validateBlogs, '', pLimit(10)); + + expect(process.exitCode).toBe(1); + process.exitCode = originalExitCode; + });🧰 Tools
🪛 eslint
[error] 179-181: Replace
⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····withcheckMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))(prettier/prettier)
[error] 189-192: Replace
⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎····withpath.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``(prettier/prettier)
[error] 199-201: Replace
⏎········'Errors·in·file·reference/specification/skipped.md',⏎······),with'Errors·in·file·reference/specification/skipped.md')(prettier/prettier)
[error] 212-214: Replace
⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎····withcheckMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))(prettier/prettier)
[error] 215-218: Replace
⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎····withexpect.stringContaining(Error·in·directory),·expect.any(Error)(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.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: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.
🪛 eslint
scripts/markdown/check-markdown.js
[error] 23-23: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10. with ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 145-150: Replace ⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎ with folderPath,·validateFunction,·limit,·relativePath·=·''
(prettier/prettier)
[error] 166-171: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········ with filePath,·validateFunction,·limit,·relativeFilePath
(prettier/prettier)
[error] 173-173: Insert ·
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 185-185: Insert ·
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 78-78: Replace resolve with (resolve)
(prettier/prettier)
[error] 78-78: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 82-82: Unary operator '++' used.
(no-plusplus)
[error] 83-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 83-86: Replace ⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········ with path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 100-100: Replace group with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10 with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
[error] 117-121: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 129-129: Delete ,
(prettier/prettier)
[error] 138-138: Delete ··
(prettier/prettier)
[error] 143-146: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 151-153: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎···· with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 164-164: Delete ,
(prettier/prettier)
[error] 172-172: Delete ,
(prettier/prettier)
[error] 179-181: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 189-192: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 199-201: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 212-214: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 215-218: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 230-233: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 250-250: Insert ⏎
(prettier/prettier)
🔇 Additional comments (4)
scripts/markdown/check-markdown.js (3)
44-49: LGTM! Good improvement in URL validation.
The change to use URL(str) instead of new URL(str) is a good improvement, following best practices for URL validation.
70-71: LGTM! Good use of Object.hasOwn.
The change to use Object.hasOwn() instead of hasOwnProperty is a good improvement, following modern JavaScript best practices.
204-215: LGTM! Good implementation of concurrency control.
The implementation of concurrency control in the main function is well-structured. The logging of the configured limit is a good practice for observability.
tests/markdown/check-markdown.test.js (1)
20-39: LGTM! Good test isolation practices.
The implementation properly handles test isolation by storing and restoring environment variables. The verification of environment restoration is a good practice.
| try{ | ||
| await limit(async () => { | ||
| 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 (error) { | ||
| console.error(`Error processing file ${relativeFilePath}:`, error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Improve error handling in file processing.
The current error handling could lead to unhandled promise rejections. Consider propagating the error without throwing it after logging:
try{
await limit(async () => {
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 (error) {
console.error(`Error processing file ${relativeFilePath}:`, error);
- throw error;
+ return Promise.reject(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.
| try{ | |
| await limit(async () => { | |
| 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 (error) { | |
| console.error(`Error processing file ${relativeFilePath}:`, error); | |
| throw error; | |
| } | |
| try{ | |
| await limit(async () => { | |
| 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 (error) { | |
| console.error(`Error processing file ${relativeFilePath}:`, error); | |
| return Promise.reject(error); | |
| } |
🧰 Tools
🪛 eslint
[error] 173-173: Insert ·
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 185-185: Insert ·
(prettier/prettier)
| it('respects concurrency limit during file processing', async () => { | ||
| const processingTimes = []; | ||
| const mockValidateFunction = jest.fn().mockImplementation(() => { | ||
| const startTime = Date.now(); | ||
| processingTimes.push(startTime); | ||
| // Simulate some processing time | ||
| return new Promise(resolve => setTimeout(resolve, 50)); | ||
| }); | ||
|
|
||
| // Create multiple test files | ||
| for (let i = 0; i < 20; i++) { | ||
| await fs.writeFile( | ||
| path.join(tempDir, `test${i}.md`), | ||
| '---\ntitle: Test\n---' | ||
| ); | ||
| } | ||
|
|
||
| const limit = pLimit(5); // Set limit to 5 | ||
| await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); | ||
|
|
||
| // Group processing times by 5 (our limit) and verify gaps between groups | ||
| const sortedTimes = processingTimes.sort(); | ||
| const groups = []; | ||
| for (let i = 0; i < sortedTimes.length; i += 5) { | ||
| groups.push(sortedTimes.slice(i, i + 5)); | ||
| } | ||
|
|
||
| // Verify that each group of 5 started processing together | ||
| groups.forEach(group => { | ||
| const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length; | ||
| expect(concurrentExecutions).toBeLessThanOrEqual(5); | ||
| }); | ||
|
|
||
| // Verify that the mock validate function was called for all files | ||
| expect(mockValidateFunction).toHaveBeenCalledTimes(20); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the concurrency test more reliable.
The current test relies on timing assumptions which could make it flaky. Consider using a more deterministic approach:
it('respects concurrency limit during file processing', async () => {
+ let activeProcesses = 0;
+ let maxActiveProcesses = 0;
const processingTimes = [];
const mockValidateFunction = jest.fn().mockImplementation(() => {
- const startTime = Date.now();
- processingTimes.push(startTime);
+ activeProcesses++;
+ maxActiveProcesses = Math.max(maxActiveProcesses, activeProcesses);
// Simulate some processing time
- return new Promise(resolve => setTimeout(resolve, 50));
+ return new Promise(resolve => setTimeout(() => {
+ activeProcesses--;
+ resolve();
+ }, 50));
});
// Create multiple test files
for (let i = 0; i < 20; i++) {
await fs.writeFile(
path.join(tempDir, `test${i}.md`),
'---\ntitle: Test\n---'
);
}
const limit = pLimit(5); // Set limit to 5
await checkMarkdownFiles(tempDir, mockValidateFunction, limit, '');
- // Group processing times by 5 (our limit) and verify gaps between groups
- const sortedTimes = processingTimes.sort();
- const groups = [];
- for (let i = 0; i < sortedTimes.length; i += 5) {
- groups.push(sortedTimes.slice(i, i + 5));
- }
-
- // Verify that each group of 5 started processing together
- groups.forEach(group => {
- const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length;
- expect(concurrentExecutions).toBeLessThanOrEqual(5);
- });
+ // Verify that we never exceeded our concurrency limit
+ expect(maxActiveProcesses).toBeLessThanOrEqual(5);
// Verify that the mock validate function was called for all files
expect(mockValidateFunction).toHaveBeenCalledTimes(20);
});📝 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.
| it('respects concurrency limit during file processing', async () => { | |
| const processingTimes = []; | |
| const mockValidateFunction = jest.fn().mockImplementation(() => { | |
| const startTime = Date.now(); | |
| processingTimes.push(startTime); | |
| // Simulate some processing time | |
| return new Promise(resolve => setTimeout(resolve, 50)); | |
| }); | |
| // Create multiple test files | |
| for (let i = 0; i < 20; i++) { | |
| await fs.writeFile( | |
| path.join(tempDir, `test${i}.md`), | |
| '---\ntitle: Test\n---' | |
| ); | |
| } | |
| const limit = pLimit(5); // Set limit to 5 | |
| await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); | |
| // Group processing times by 5 (our limit) and verify gaps between groups | |
| const sortedTimes = processingTimes.sort(); | |
| const groups = []; | |
| for (let i = 0; i < sortedTimes.length; i += 5) { | |
| groups.push(sortedTimes.slice(i, i + 5)); | |
| } | |
| // Verify that each group of 5 started processing together | |
| groups.forEach(group => { | |
| const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length; | |
| expect(concurrentExecutions).toBeLessThanOrEqual(5); | |
| }); | |
| // Verify that the mock validate function was called for all files | |
| expect(mockValidateFunction).toHaveBeenCalledTimes(20); | |
| }); | |
| it('respects concurrency limit during file processing', async () => { | |
| let activeProcesses = 0; | |
| let maxActiveProcesses = 0; | |
| const processingTimes = []; | |
| const mockValidateFunction = jest.fn().mockImplementation(() => { | |
| activeProcesses++; | |
| maxActiveProcesses = Math.max(maxActiveProcesses, activeProcesses); | |
| // Simulate some processing time | |
| return new Promise(resolve => setTimeout(() => { | |
| activeProcesses--; | |
| resolve(); | |
| }, 50)); | |
| }); | |
| // Create multiple test files | |
| for (let i = 0; i < 20; i++) { | |
| await fs.writeFile( | |
| path.join(tempDir, `test${i}.md`), | |
| '---\ntitle: Test\n---' | |
| ); | |
| } | |
| const limit = pLimit(5); // Set limit to 5 | |
| await checkMarkdownFiles(tempDir, mockValidateFunction, limit, ''); | |
| // Verify that we never exceeded our concurrency limit | |
| expect(maxActiveProcesses).toBeLessThanOrEqual(5); | |
| // Verify that the mock validate function was called for all files | |
| expect(mockValidateFunction).toHaveBeenCalledTimes(20); | |
| }); |
🧰 Tools
🪛 eslint
[error] 78-78: Replace resolve with (resolve)
(prettier/prettier)
[error] 78-78: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 82-82: Unary operator '++' used.
(no-plusplus)
[error] 83-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 83-86: Replace ⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········ with path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 100-100: Replace group with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10 with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
|
closing as there is already a PR for this issue |
Summary
Improve markdown file validation script performance by adding concurrency control using
p-limit. This enhancement prevents resource exhaustion and provides a configurable mechanism to manage system resource usage during file processing.Motivation
Changes
MARKDOWN_CONCURRENCY_LIMITp-limitto control the number of concurrent file processing tasksImplementation Details
p-limitlibrary to manage concurrencyMARKDOWN_CONCURRENCY_LIMITenvironment variableEnvironment Configuration
Users can customize concurrency by setting the
MARKDOWN_CONCURRENCY_LIMITenvironment variable: