Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sarvesh2783
Copy link

@Sarvesh2783 Sarvesh2783 commented Dec 4, 2024

Description

  • Implemented concurrency limit: Introduced the p-limit package to restrict the number of concurrently processed markdown files, improving resource management.
  • Optimized file handling: Ensured only a specified number of files are processed in parallel, preventing potential file handle exhaustion and enhancing performance.
  • Retained error handling: Maintained comprehensive error logging and validation for markdown frontmatter, ensuring smooth execution and clear reporting of issues.

Related issue(s)

#<#3429> - "Limit concurrency in markdown file processing to improve performance and prevent potential resource exhaustion"

Summary by CodeRabbit

  • New Features
    • Enhanced markdown file validation with improved concurrency control, allowing for faster processing of multiple files.
  • Bug Fixes
    • Updated error handling to effectively manage file reading failures during validation.
  • Documentation
    • Clarified function signatures to reflect new processing functions and concurrency management.

Copy link

coderabbitai bot commented Dec 4, 2024

Walkthrough

The changes in this pull request enhance the check-markdown.js script by implementing concurrency control for markdown file validation. A new import for the p-limit package manages concurrent tasks, and the script introduces two new functions: processMarkdownFile for individual file processing and processMarkdownFolder for recursive folder processing. Error handling is improved, and the script concludes with an async function that processes both documentation and blog folders concurrently.

Changes

File Path Change Summary
scripts/markdown/check-markdown.js - Added p-limit package for concurrency control.
- Replaced checkMarkdownFiles with processMarkdownFolder and processMarkdownFile functions.
- Updated error handling in processMarkdownFile to reject promises on failure.
- Maintained recursive structure in processMarkdownFolder for directory traversal.

Possibly related issues

Possibly related PRs

  • test: add markdown tests #3301: This PR adds tests for markdown files, which directly relates to the changes made in the main PR that enhance the processing and validation of markdown files.

Suggested labels

bounty

Suggested reviewers

  • derberg
  • magicmatatjahu
  • Mayaleeeee
  • asyncapi-bot-eve
  • devilkiller-ag
  • sambhavgupta0705
  • akshatnema

Poem

🐰 In the land of markdown, we hop and play,
With limits on tasks, we brighten the day.
No more chaos, just order we seek,
As files are processed, quick and sleek!
Hooray for the changes, let’s cheer and rejoice,
For smoother validations, we all have a voice! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Dec 4, 2024

We require all PRs to follow Conventional Commits specification.
More details 👇🏼

 No release type found in pull request title "Add Concurrency Limit to Markdown File Processing for Improved Performance Fixes #3429". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c65c5e2
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/6752b2b004f9f400081cdd57
😎 Deploy Preview https://deploy-preview-3446--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 67.16%. Comparing base (72c8dd8) to head (c65c5e2).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
scripts/markdown/check-markdown.js 0.00% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3446      +/-   ##
==========================================
- Coverage   67.77%   67.16%   -0.61%     
==========================================
  Files          21       21              
  Lines         664      670       +6     
==========================================
  Hits          450      450              
- Misses        214      220       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Dec 4, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 47
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-3446--asyncapi-website.netlify.app/

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (2)
scripts/markdown/check-markdown.js (2)

4-4: Fix code formatting issues as indicated by static analysis

Prettier has reported multiple formatting issues in the modified code. Please run the code through Prettier to fix these issues.

Also applies to: 104-152, 159-160

🧰 Tools
🪛 eslint

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

(import/no-extraneous-dependencies)


[error] 4-4: Delete ·

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


122-141: Consider using fs.promises API for cleaner asynchronous code

Instead of manually wrapping fs.stat in a new Promise, consider using the fs.promises API to handle asynchronous file system operations more cleanly.

Apply this refactor to simplify the code:

- const task = new Promise((resolve, reject) => {
-     fs.stat(filePath, (statErr, stats) => {
-         if (statErr) {
-             reject(new Error(`Error reading file stats: ${statErr.message}`));
-         } else {
-             // Recurse if directory, otherwise validate markdown file
-             if (stats.isDirectory()) {
-                 checkMarkdownFiles(filePath, validateFunction, relativeFilePath, concurrencyLimit);
-             } else if (path.extname(file) === '.md') {
-                 const fileContent = fs.readFileSync(filePath, 'utf-8');
-                 const { data: frontmatter } = matter(fileContent);
- 
-                 const errors = validateFunction(frontmatter);
-                 if (errors) {
-                     console.log(`Errors in file ${relativeFilePath}:`);
-                     errors.forEach(error => console.log(` - ${error}`));
-                     process.exitCode = 1;
-                 }
-             }
-             resolve();
-         }
-     });
- });
+ const task = fs.promises.stat(filePath)
+     .then(stats => {
+         // Recurse if directory, otherwise validate markdown file
+         if (stats.isDirectory()) {
+             checkMarkdownFiles(filePath, validateFunction, relativeFilePath, concurrencyLimit);
+         } else if (path.extname(file) === '.md') {
+             const fileContent = fs.readFileSync(filePath, 'utf-8');
+             const { data: frontmatter } = matter(fileContent);
+ 
+             const errors = validateFunction(frontmatter);
+             if (errors) {
+                 console.log(`Errors in file ${relativeFilePath}:`);
+                 errors.forEach(error => console.log(` - ${error}`));
+                 process.exitCode = 1;
+             }
+         }
+     })
+     .catch(statErr => {
+         console.error(`Error reading file stats: ${statErr.message}`);
+     });
🧰 Tools
🪛 eslint

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

(prettier/prettier)


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

(prettier/prettier)


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

(no-shadow)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prefer-promise-reject-errors)


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

(prettier/prettier)


[error] 127-127: Delete ············

(prettier/prettier)


[error] 128-128: Delete ············

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 139-139: Delete ··············

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


[warning] 128-132: scripts/markdown/check-markdown.js#L128-L132
Added lines #L128 - L132 were not covered by tests


[warning] 134-138: scripts/markdown/check-markdown.js#L134-L138
Added lines #L134 - L138 were not covered by tests


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

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 72c8dd8 and 6865988.

📒 Files selected for processing (1)
  • scripts/markdown/check-markdown.js (3 hunks)
🧰 Additional context used
🪛 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] 4-4: Delete ·

(prettier/prettier)


[error] 104-104: Replace ····const·limit·=·pLimit(concurrencyLimit);·· with ··const·limit·=·pLimit(concurrencyLimit);·

(prettier/prettier)


[error] 105-105: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(no-shadow)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prefer-promise-reject-errors)


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

(prettier/prettier)


[error] 127-127: Delete ············

(prettier/prettier)


[error] 128-128: Delete ············

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(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] 144-144: Replace ············ with ······

(prettier/prettier)


[error] 146-146: Delete ······

(prettier/prettier)


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

(prettier/prettier)


[error] 148-148: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 151-151: Delete ····

(prettier/prettier)


[error] 152-152: Delete ··

(prettier/prettier)


[error] 159-159: Delete ·

(prettier/prettier)


[error] 160-160: 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] 104-105: scripts/markdown/check-markdown.js#L104-L105
Added lines #L104 - L105 were not covered by tests


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


[warning] 128-132: scripts/markdown/check-markdown.js#L128-L132
Added lines #L128 - L132 were not covered by tests


[warning] 134-138: scripts/markdown/check-markdown.js#L134-L138
Added lines #L134 - L138 were not covered by tests


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


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


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


[warning] 159-160: scripts/markdown/check-markdown.js#L159-L160
Added lines #L159 - L160 were not covered by tests

@@ -1,6 +1,7 @@
const fs = require('fs');
const matter = require('gray-matter');
const path = require('path');
const pLimit = require('p-limit'); // Import the p-limit package
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

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

🧰 Tools
🪛 eslint

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

(import/no-extraneous-dependencies)


[error] 4-4: Delete ·

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


🛠️ Refactor suggestion

Increase test coverage for the new code

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

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

🧰 Tools
🪛 eslint

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

(import/no-extraneous-dependencies)


[error] 4-4: Delete ·

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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

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

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix missing parentheses in arrow function

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

Apply this diff to fix the issue:

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

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

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

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

(prettier/prettier)

const task = new Promise((resolve, reject) => {
fs.stat(filePath, (err, stats) => {
if (err) {
reject('Error reading file stats:', err);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use Error object when rejecting promises

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

Apply this diff to fix the issue:

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

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

🧰 Tools
🪛 eslint

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

(prettier/prettier)


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

(prefer-promise-reject-errors)

🪛 GitHub Check: codecov/patch

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

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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid variable shadowing of 'err'

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

Apply this diff to fix the issue:

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

Also, update the reference to err inside this callback:

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

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

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

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

(prettier/prettier)


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

(no-shadow)

@Sarvesh2783
Copy link
Author

I am working on the tests that are failing, will try to resolve the issue !!!!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
scripts/markdown/check-markdown.js (3)

6-6: Consider making the concurrency limit configurable

The hardcoded limit of 5 concurrent tasks could be made configurable through environment variables or command line arguments for better flexibility.

Example implementation:

- const limit = pLimit(5); // Limit the number of concurrent tasks
+ const CONCURRENT_LIMIT = process.env.MARKDOWN_CONCURRENT_LIMIT || 5;
+ const limit = pLimit(parseInt(CONCURRENT_LIMIT, 10));
🧰 Tools
🪛 GitHub Check: codecov/patch

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


129-131: Make excluded paths configurable

The hardcoded path exclusion for 'reference/specification' should be configurable for better maintainability.

+ const EXCLUDED_PATHS = process.env.MARKDOWN_EXCLUDED_PATHS 
+   ? process.env.MARKDOWN_EXCLUDED_PATHS.split(',')
+   : ['reference/specification'];
+
- if (relativeFilePath.includes('reference/specification')) {
+ if (EXCLUDED_PATHS.some(path => relativeFilePath.includes(path))) {
🧰 Tools
🪛 eslint

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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


155-158: Enhance error handling with detailed logging

The catch block could provide more detailed error information for debugging.

- console.error(err.message);
+ console.error('Error processing markdown files:', {
+   message: err.message,
+   stack: err.stack,
+   timestamp: new Date().toISOString()
+ });
🧰 Tools
🪛 eslint

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

(prettier/prettier)


[error] 156-156: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 158-158: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

[warning] 156-157: scripts/markdown/check-markdown.js#L156-L157
Added lines #L156 - L157 were not covered by tests

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6865988 and c65c5e2.

📒 Files selected for processing (1)
  • scripts/markdown/check-markdown.js (2 hunks)
🧰 Additional context used
🪛 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] 96-96: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 113-113: Delete ··

(prettier/prettier)


[error] 124-124: Delete ··

(prettier/prettier)


[error] 125-125: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 139-139: Delete ··

(prettier/prettier)


[error] 141-141: Delete ··

(prettier/prettier)


[error] 150-150: Delete ··

(prettier/prettier)


[error] 151-151: Delete ····

(prettier/prettier)


[error] 152-152: Delete ······

(prettier/prettier)


[error] 153-153: Replace ······processMarkdownFolder(blogsFolderPath,·validateBlogs), with processMarkdownFolder(blogsFolderPath,·validateBlogs)

(prettier/prettier)


[error] 154-154: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 156-156: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 158-158: 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] 6-6: scripts/markdown/check-markdown.js#L6
Added line #L6 was not covered by tests


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


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


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


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


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


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


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


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


[warning] 149-151: scripts/markdown/check-markdown.js#L149-L151
Added lines #L149 - L151 were not covered by tests


[warning] 156-157: scripts/markdown/check-markdown.js#L156-L157
Added lines #L156 - L157 were not covered by tests

🔇 Additional comments (3)
scripts/markdown/check-markdown.js (3)

108-108: Fix arrow function parentheses

The arrow function in forEach is missing parentheses around the parameter.

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

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

(prettier/prettier)


4-4: ⚠️ Potential issue

Add p-limit to package.json dependencies

The p-limit package needs to be added to the project dependencies.

Run the following command to add it:

npm install --save p-limit
🧰 Tools
🪛 eslint

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

(import/no-extraneous-dependencies)

🪛 GitHub Check: codecov/patch

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


Line range hint 4-159: Verify performance impact of concurrent processing

Let's verify the performance impact of the concurrent processing implementation.

✅ Verification successful

Concurrent processing implementation is appropriate for the workload

The current implementation using p-limit with a concurrency of 5 is well-suited for the codebase's characteristics:

  • Total of 251 markdown files, which is a moderate size
  • Largest files are under 10KB, indicating lightweight processing
  • Frontmatter sections are relatively small (max 20 lines)
  • File reading and validation operations are I/O bound, making concurrent processing beneficial
  • The limit of 5 concurrent operations provides a good balance between performance and resource usage
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the number and size of markdown files to assess processing load

# Count total markdown files
echo "Total markdown files:"
fd -e md | wc -l

# Show largest markdown files
echo -e "\nLargest markdown files:"
fd -e md -x ls -lh {} | sort -rh | head -n 5

# Check for potential memory usage by looking for large frontmatter
echo -e "\nFiles with large frontmatter:"
fd -e md -x sh -c 'head -n 20 {} | grep -A 20 "^---" | grep -B 20 "^---" | wc -l | tr -d "\n" && echo " {}"' | sort -rn | head -n 5

Length of output: 1130

🧰 Tools
🪛 eslint

[error] 96-96: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 113-113: Delete ··

(prettier/prettier)


[error] 124-124: Delete ··

(prettier/prettier)


[error] 125-125: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 139-139: Delete ··

(prettier/prettier)


[error] 141-141: Delete ··

(prettier/prettier)


[error] 150-150: Delete ··

(prettier/prettier)


[error] 151-151: Delete ····

(prettier/prettier)


[error] 152-152: Delete ······

(prettier/prettier)


[error] 153-153: Replace ······processMarkdownFolder(blogsFolderPath,·validateBlogs), with processMarkdownFolder(blogsFolderPath,·validateBlogs)

(prettier/prettier)


[error] 154-154: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 156-156: Delete ····

(prettier/prettier)


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

(prettier/prettier)


[error] 158-158: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


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


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


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


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


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


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


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


[warning] 149-151: scripts/markdown/check-markdown.js#L149-L151
Added lines #L149 - L151 were not covered by tests


[warning] 156-157: scripts/markdown/check-markdown.js#L156-L157
Added lines #L156 - L157 were not covered by tests

Comment on lines +95 to 113
function processMarkdownFile(filePath, validateFunction, relativePath) {
return new Promise((resolve, reject) => {
fs.readFile(filePath, 'utf-8', (err, content) => {
if (err) {
reject(new Error(`Error reading file ${filePath}: ${err.message}`));
return;
}

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

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

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

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test coverage for the processMarkdownFile function

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

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

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

[error] 96-96: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 113-113: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


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


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


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

Comment on lines +123 to +142
async function processMarkdownFolder(folderPath, validateFunction, relativePath = '') {
const files = await fs.promises.readdir(folderPath);
const tasks = files.map(async (file) => {
const filePath = path.join(folderPath, file);
const relativeFilePath = path.join(relativePath, file);

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

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

await Promise.all(tasks);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test coverage for the processMarkdownFolder function

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

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

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

[error] 124-124: Delete ··

(prettier/prettier)


[error] 125-125: Delete ··

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


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

(prettier/prettier)


[error] 139-139: Delete ··

(prettier/prettier)


[error] 141-141: Delete ··

(prettier/prettier)

🪛 GitHub Check: codecov/patch

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


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


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


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

@sambhavgupta0705
Copy link
Member

@Sarvesh2783 are you working on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants