fix: consolidates utility functions by merging into a single utils.ts file. #3996
Conversation
…ut cleanup in the processBatch function in check-edit-links.ts
Siddhant-Pragyan-Sinha
requested review from
derberg,
akshatnema,
anshgoyalevil,
sambhavgupta0705,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis pull request consolidates several utility functions by updating import paths across various script and test files. Specifically, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Utils as Utils Module (writeJSON)
participant FS as File System
Caller->>Utils: call writeJSON(readPath, writePath)
Utils->>FS: readFile(readPath)
FS-->>Utils: return file content
Utils->>Utils: convertToJson(content)
Utils->>FS: writeFile(writePath, JSON content)
FS-->>Utils: acknowledge write completion
Utils-->>Caller: return result (or handle error)
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tools/extract-tools-github.ts(1 hunks)tests/tools/extract-tools-github.test.js(1 hunks)
🧰 Additional context used
🪛 ESLint
scripts/tools/extract-tools-github.ts
[error] 8-8: '/home/jailuser/git/scripts/utils.ts' imported multiple times.
(import/no-duplicates)
tests/tools/extract-tools-github.test.js
[error] 3-3: Import in body of module; reorder to top.
(import/first)
[error] 3-3: ../../scripts/utils.ts import should occur before import of axios
(import/order)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/tools/extract-tools-github.ts (1)
7-7: Combine utility imports: resolved duplicate import issue.Great consolidation of the
pauseandloggerimports into a single statement, which prevents duplicate-import ESLint errors and simplifies module dependencies.🧰 Tools
🪛 ESLint
[error] 2-7: Run autofix to sort these imports!
(simple-import-sort/imports)
🧹 Nitpick comments (4)
scripts/tools/extract-tools-github.ts (2)
28-30: RenamePerPageparameter to follow camelCase.The
getReqUrlhelper uses aPerPageparameter, which violates the standard camelCase naming convention in TypeScript. Consider renaming it toperPagefor consistency:- const getReqUrl = (PerPage: number, pageNo: number) => - `...?per_page=${PerPage}&page=${pageNo}`; + const getReqUrl = (perPage: number, pageNo: number) => + `...?per_page=${perPage}&page=${pageNo}`;
18-19: Remove redundanttry/catch: no additional error handling added.The current
try { ... } catch (err) { throw err; }block simply rethrows the caught error without augmenting it. You can safely eliminate thetry/catch(or remove the ESLint disable forno-useless-catch) to clean up the code.tests/tools/extract-tools-github.test.js (2)
1-3: Fix import formatting for consistency.Add a space in the named import and ensure imports are grouped consistently:
-import { logger} from '../../scripts/utils'; +import { logger } from '../../scripts/utils';🧰 Tools
🪛 ESLint
[error] 2-2: Missing file extension "ts" for "../../scripts/tools/extract-tools-github"
(import/extensions)
[error] 3-3: Insert
·(prettier/prettier)
[error] 3-3: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
5-8: Verify mocking ofpauseinvocation.The test correctly mocks both
logger.infoandpause, but there's no assertion thatpauseis invoked when fetching additional pages. Consider adding:expect(pause).toHaveBeenCalledWith(1000);in the multi-page test to ensure your pagination delay logic is covered.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tools/extract-tools-github.ts(1 hunks)tests/tools/extract-tools-github.test.js(1 hunks)
🧰 Additional context used
🪛 ESLint
scripts/tools/extract-tools-github.ts
[error] 2-7: Run autofix to sort these imports!
(simple-import-sort/imports)
tests/tools/extract-tools-github.test.js
[error] 2-2: Missing file extension "ts" for "../../scripts/tools/extract-tools-github"
(import/extensions)
[error] 3-3: Insert ·
(prettier/prettier)
[error] 3-3: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
🔇 Additional comments (1)
tests/tools/extract-tools-github.test.js (1)
1-11: ESM import strategy and mock ordering is correct.The test migrates from CommonJS to ESM for
axios,getData, and mocks the entireutilsmodule before exercisinggetData. Thanks to Jest’s hoisting ofjest.mock, the mocks are applied in time to intercept thepauseandloggerimports used bygetData, and the tests cover success, pagination, and error cases.🧰 Tools
🪛 ESLint
[error] 2-2: Missing file extension "ts" for "../../scripts/tools/extract-tools-github"
(import/extensions)
[error] 3-3: Insert
·(prettier/prettier)
[error] 3-3: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/tools/extract-tools-github.ts (1)
2-7: Consider sorting importsThe static analysis tool flagged that these imports could be sorted more consistently.
import axios from 'axios'; import dotenv from 'dotenv'; import type { ToolsData } from '@/types/scripts/tools'; -import { pause, logger } from '../utils'; +import { logger, pause } from '../utils';🧰 Tools
🪛 ESLint
[error] 2-7: Run autofix to sort these imports!
(simple-import-sort/imports)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tools/extract-tools-github.ts(2 hunks)tests/tools/extract-tools-github.test.js(1 hunks)
🧰 Additional context used
🪛 ESLint
scripts/tools/extract-tools-github.ts
[error] 2-7: Run autofix to sort these imports!
(simple-import-sort/imports)
tests/tools/extract-tools-github.test.js
[error] 2-2: Missing file extension "ts" for "../../scripts/tools/extract-tools-github"
(import/extensions)
[error] 3-3: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (4)
scripts/tools/extract-tools-github.ts (2)
7-7: Import consolidation looks goodThe consolidation of utility imports into a single statement is a positive improvement that aligns with the PR's objective of centralizing utility functions in a single
utils.tsfile.🧰 Tools
🪛 ESLint
[error] 2-7: Run autofix to sort these imports!
(simple-import-sort/imports)
28-29: Parameter naming improvementChanging from
PerPagetoperPageimproves consistency by following JavaScript/TypeScript camelCase naming conventions.tests/tools/extract-tools-github.test.js (2)
1-3: Import syntax modernization looks goodThe switch from CommonJS
require()to ES moduleimportsyntax is a positive change that makes the test file more consistent with modern JavaScript practices and matches the implementation file's syntax.🧰 Tools
🪛 ESLint
[error] 2-2: Missing file extension "ts" for "../../scripts/tools/extract-tools-github"
(import/extensions)
[error] 3-3: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
5-7: Comprehensive mocking implementationThe Jest mock has been properly updated to include both the
loggerandpausefunctions from the consolidated utils module, which is necessary for the tests to run correctly after the utility consolidation.
|
Hey @vishvamsinh28 @sambhavgupta0705 please take a look at this PR and provide feedback regarding it. |
Siddhant-Pragyan-Sinha
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/adopters/index.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
scripts/adopters/index.ts
[error] 14-14: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 14-15: Delete ··⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
scripts/adopters/index.ts (1)
4-4: Import path successfully updated.The import path has been correctly updated to use the consolidated
utils.tsfile, which aligns with the PR's objective of centralizing utility functions.
|
Hey @akshatnema , the PR testing for macos is failing again and again due to a test failing in build-tools.test.js, a file that has not been modified. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/check-locales.ts (1)
185-185: Add a newline at the end of the fileThe file is missing a newline at the end, which is causing linting errors. This is a minor issue but should be fixed to maintain consistent formatting.
if (process.argv[1] === fileURLToPath(import.meta.url)) { validateLocales(); -} +} +🧰 Tools
🪛 ESLint
[error] 185-185: Newline required at end of file but not found.
(eol-last)
[error] 185-185: Insert
⏎(prettier/prettier)
scripts/utils.ts (3)
70-76: Add blank lines for better readability in writeJSON functionTo improve code readability and follow consistent formatting practices, add blank lines after variable declarations and before return statements.
export async function writeJSON(readPath: string, writePath: string) { try { const readContent = await readFile(readPath, 'utf-8'); const jsonContent = convertToJson(readContent); + await writeFile(writePath, JSON.stringify(jsonContent)); + return jsonContent; } catch (err: any) {🧰 Tools
🪛 ESLint
[error] 73-73: Expected blank line after variable declarations.
(newline-after-var)
[error] 74-74: Expected blank line before this statement.
(padding-line-between-statements)
[error] 75-75: Expected blank line before this statement.
(padding-line-between-statements)
82-85: Fix indentation in error conditionThe indentation in the error condition is inconsistent and causing linting errors. Let's fix the formatting to maintain consistent code style.
} else if (err.message.includes('write')) { throw new Error(`Error while writing file\nError: ${err}`); - } else if (err.message.includes('Invalid content') || - err.message.includes('JSON') || - err.message.includes('YAML')) { + } else if ( + err.message.includes('Invalid content') || + err.message.includes('JSON') || + err.message.includes('YAML') + ) { throw new Error(`Error while conversion\nError: ${err}`);🧰 Tools
🪛 ESLint
[error] 82-82: Insert
⏎······(prettier/prettier)
[error] 83-83: Delete
·········(prettier/prettier)
[error] 84-84: Replace
·········err.message.includes('YAML')witherr.message.includes('YAML')⏎····(prettier/prettier)
90-90: Add a newline at the end of the fileThe file is missing a newline at the end, which is causing linting errors. This is a minor issue but should be fixed to maintain consistent formatting.
} } -} +} +🧰 Tools
🪛 ESLint
[error] 90-90: Newline required at end of file but not found.
(eol-last)
[error] 90-90: Insert
⏎(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)scripts/check-locales.ts(2 hunks)scripts/utils.ts(3 hunks)tests/check-locales.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- tests/check-locales.test.js
🧰 Additional context used
🪛 ESLint
scripts/utils.ts
[error] 73-73: Expected blank line after variable declarations.
(newline-after-var)
[error] 74-74: Expected blank line before this statement.
(padding-line-between-statements)
[error] 75-75: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-82: Insert ⏎······
(prettier/prettier)
[error] 83-83: Delete ·········
(prettier/prettier)
[error] 84-84: Replace ·········err.message.includes('YAML') with err.message.includes('YAML')⏎····
(prettier/prettier)
[error] 90-90: Newline required at end of file but not found.
(eol-last)
[error] 90-90: Insert ⏎
(prettier/prettier)
scripts/check-locales.ts
[error] 185-185: Newline required at end of file but not found.
(eol-last)
[error] 185-185: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (3)
scripts/check-locales.ts (1)
6-6: Import path update looks goodChanging the import path from './logger' to './utils' aligns with the PR goal of consolidating utility functions into a single utils.ts file.
scripts/utils.ts (2)
2-5: Good job consolidating utility functionsThe consolidation of imports, logger configuration, and utility functions into a single file improves code organization and maintainability. The exports for
convertToJsonandpausefunctions, along with the new winston logger configuration, align well with the PR objectives.Also applies to: 15-15, 45-45, 51-65
67-90: Comprehensive error handling in writeJSON functionThe error handling in the writeJSON function is well-implemented, with specific error messages for different types of failures (read errors, write errors, conversion errors). This will make debugging much easier.
🧰 Tools
🪛 ESLint
[error] 73-73: Expected blank line after variable declarations.
(newline-after-var)
[error] 74-74: Expected blank line before this statement.
(padding-line-between-statements)
[error] 75-75: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-82: Insert
⏎······(prettier/prettier)
[error] 83-83: Delete
·········(prettier/prettier)
[error] 84-84: Replace
·········err.message.includes('YAML')witherr.message.includes('YAML')⏎····(prettier/prettier)
[error] 90-90: Newline required at end of file but not found.
(eol-last)
[error] 90-90: Insert
⏎(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/utils.ts (1)
76-76: Remove trailing whitespace.There is trailing whitespace on this line that should be removed.
- await writeFile(writePath, JSON.stringify(jsonContent)); - + await writeFile(writePath, JSON.stringify(jsonContent));🧰 Tools
🪛 ESLint
[error] 76-76: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 76-76: Delete
····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/if-nodejs-pr-testing.yml(1 hunks)scripts/check-locales.ts(1 hunks)scripts/utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/check-locales.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/utils.ts (2)
tests/utils.test.js (2)
readPath(24-24)writePath(25-25)tests/readAndWriteJson.test.js (2)
readPath(18-18)writePath(19-19)
🪛 ESLint
scripts/utils.ts
[error] 76-76: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 76-76: Delete ····
(prettier/prettier)
🔇 Additional comments (5)
scripts/utils.ts (4)
2-3: Appropriate imports added for new functionality.The imports for Winston and file operations are correctly added to support the newly consolidated functionality from separate files.
🧰 Tools
🪛 ESLint
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
15-15: Good export pattern for existing utility functions.Making these previously internal utility functions (
convertToJsonandpause) available as exports improves reusability across the codebase.Also applies to: 45-45
54-65: Well-configured Winston logger.The logger implementation is properly configured with:
- Environment variable controlled log level
- Colorized output for better readability
- Consistent timestamp format
- Aligned formatting for easier scanning
70-94: Comprehensive error handling in writeJSON function.The function effectively distinguishes between different error types:
- Read errors (including file not found)
- Write errors
- Conversion errors (JSON/YAML parsing)
- Generic errors
This will make debugging issues much clearer for developers.
🧰 Tools
🪛 ESLint
[error] 76-76: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 76-76: Delete
····(prettier/prettier)
.github/workflows/if-nodejs-pr-testing.yml (1)
136-136: Improved timestamp filtering in locale checks.The updated regex pattern now correctly filters out log lines with both AM and PM timestamps, providing cleaner output for locale check errors.
|
Fixed with #4049 |
fixes issue #3956
Describe the bug
BEFORE:-
AFTER:-
Summary of Changes by File
utils.test.js
convertToJsonfunction to handle JSON parsing, YAML parsing, object handling, and invalid input scenariospausefunction, verifying timeout behavior, resolution after specified time, and handling of zero/negative timeout valueswriteJSONfunction, covering specific error scenarios:check-edit-links.test.js
Implemented test coverage for
pauseandloggerfunctions.Changed the import statement to match the new configuration.
tools-object.test.js
Implemented test coverage for
convertToJsonalong withlogger.Changed the import statement to match the new file configuration.
##readAndWriteJson.test.js
Implemented test coverage for
writeJSONalong withconvertToJson.Used a simple mock implementation for
writeJSONthat matches the original test.##extract-tools-github.test.js
Implemented test coverage for
convertToJsonalong withlogger.Changed the import statement to match the new file configuration.
build-tools.test.js
utils.ts
No direct changes to the implementation code were made
The logger.ts file and the readAndWriteJson.ts file components were added to it without any change in their code.
build-dashboard.test.js
These changes collectively achieved 100% test coverage across the codebase while addressing specific issues with hanging processes and test output cleanliness.
How to test:-
npm testto see the outputChecklist:-
No error was encountered when executing
tsc --noEmit.No error was encountered when executing
npm run build.No error was encountered when executing
npm run lint.Feedback from maintainers and reviewers is requested, to review and do the needful changes, if any.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor