chore: add the getTitle helper
#1568
Conversation
ItshMoh
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
|
""" WalkthroughA new utility function, Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
Tip For best results, initiate chat on the files or code changes. @asyncapi-bot Thank you for introducing yourself and sharing these useful PR commands! This is helpful information for users reviewing and managing PRs in this repository. I appreciate you providing this clear documentation of your capabilities. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/helpers/src/utils.js (1)
3-22: Well-implemented helper function for title validation!The new
getTitlefunction properly validates the AsyncAPI title, throwing descriptive errors for missing or empty titles. This centralization of title validation logic will improve error handling across the codebase.Consider enhancing the implementation with these minor improvements:
- Add validation for when the
infoparameter itself is null/undefined- Use a more explicit check like
typeof info.title !== 'function'instead of the more ambiguous!info.title- Remove the extra blank line on line 20 for consistency
const getTitle = info => { + if (!info) { + throw new Error('AsyncAPI info object is required to retrieve title.'); + } - if (!info.title) { + if (typeof info.title !== 'function') { throw new Error('Provided AsyncAPI document info field doesn\'t contain title.'); } const title = info.title(); if (title === '') { throw new Error('AsyncAPI document title cannot be an empty string.'); } - return title; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
packages/helpers/src/index.js(2 hunks)packages/helpers/src/utils.js(3 hunks)packages/helpers/test/utils.test.js(2 hunks)packages/templates/clients/websocket/dart/template/client.dart.js(1 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(1 hunks)packages/templates/clients/websocket/python/template/client.py.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/templates/clients/websocket/python/template/client.py.js (1)
packages/helpers/src/utils.js (3)
title(16-16)title(36-36)getTitle(12-22)
packages/helpers/src/index.js (2)
packages/helpers/src/utils.js (1)
require(1-1)packages/helpers/test/utils.test.js (2)
require(2-2)require(3-3)
packages/helpers/src/utils.js (3)
packages/templates/clients/websocket/dart/template/client.dart.js (2)
info(9-9)title(10-10)packages/templates/clients/websocket/javascript/template/client.js.js (2)
info(9-9)title(10-10)packages/templates/clients/websocket/python/template/client.py.js (2)
info(9-9)title(10-10)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (16)
packages/helpers/src/utils.js (2)
36-36: Good integration with existing functionsUsing the new
getTitlehelper function ingetClientNamecentralizes title validation logic and improves error handling compared to directly accessinginfo.title().
59-61: Proper export of the new helper functionThe
getTitlefunction is correctly added to the module exports, maintaining alphabetical ordering of exports. This makes the function available for other modules that require this functionality.packages/helpers/src/index.js (2)
3-3: Properly imported the new helper functionThe new
getTitlefunction is correctly imported from the utils module alongside other existing utilities.
13-14: Correctly exported getTitle for external useThe
getTitlefunction is properly added to the module exports, making it available as part of the public API for use in template files.packages/templates/clients/websocket/javascript/template/client.js.js (2)
2-2: Properly imported the getTitle helperThe
getTitlefunction is correctly imported from the generator helpers package.
10-10: Improved title handling with the new helperUsing
getTitle(info)instead of directly accessinginfo.title()improves error handling by leveraging the validation logic in the helper. This will provide clearer error messages when the title is missing or empty.packages/templates/clients/websocket/python/template/client.py.js (2)
2-2: Properly imported the getTitle helperThe
getTitlefunction is correctly imported from the generator helpers package alongside other existing utilities.
10-10: Improved title handling with the new helperUsing
getTitle(info)instead of directly accessinginfo.title()improves error handling by leveraging the validation logic in the helper. This standardizes title retrieval across templates and provides better error messages.packages/templates/clients/websocket/dart/template/client.dart.js (2)
2-2: Good import integration.The import has been properly updated to include the new
getTitlehelper function.
10-10: Implementation correctly uses new helper function.The direct call to
info.title()has been appropriately replaced with the newgetTitle(info)helper function, which provides additional validation.packages/helpers/test/utils.test.js (6)
3-3: Proper import of the new helper function.The
getTitlefunction is correctly imported from the helper package.
48-48: Good test organization.Clear separation between test suites with proper closing and new line spacing.
Also applies to: 49-49
50-57: Well-structured test suite initialization.The test setup follows the same pattern as the existing test suite, maintaining consistency.
58-63: Good happy path test case.The test correctly verifies that the function returns the exact title when it exists.
64-69: Good error case test for missing title.This test properly verifies the error handling when the title function doesn't exist.
71-80: Good error case test for empty title.This test properly verifies that an error is thrown when the title is an empty string.
| describe('getTitle integration test with AsyncAPI', () => { | ||
| let parsedAsyncAPIDocument; | ||
|
|
||
| beforeAll(async () => { | ||
| const parseResult = await fromFile(parser, asyncapi_v3_path).parse(); | ||
| parsedAsyncAPIDocument = parseResult.document; | ||
| }); | ||
|
|
||
| it('should return the exact title parameter when exists', () => { | ||
| const info = parsedAsyncAPIDocument.info(); | ||
| const expectedTitle = info.title(); | ||
| const actualTitle = getTitle(parsedAsyncAPIDocument.info()); | ||
| expect(actualTitle).toStrictEqual(expectedTitle); | ||
| }); | ||
| it('should throw error when title function does not exist', () => { | ||
| const infoWithoutTitle = { /* missing title property */ }; | ||
| expect(() => { | ||
| getTitle(infoWithoutTitle); | ||
| }).toThrow('Provided AsyncAPI document info field doesn\'t contain title.'); | ||
| }); | ||
|
|
||
| it('should throw error when title is an empty string', () => { | ||
| // Mock an info object where title() returns an empty string | ||
| const infoWithEmptyTitle = { | ||
| title: () => '' | ||
| }; | ||
|
|
||
| expect(() => { | ||
| getTitle(infoWithEmptyTitle); | ||
| }).toThrow('AsyncAPI document title cannot be an empty string.'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a test for null title value.
While you've covered empty string and missing title cases, there's no explicit test for when the title function returns null. According to the PR description, the function should also throw an error in this case.
+ it('should throw error when title is null', () => {
+ // Mock an info object where title() returns null
+ const infoWithNullTitle = {
+ title: () => null
+ };
+
+ expect(() => {
+ getTitle(infoWithNullTitle);
+ }).toThrow('AsyncAPI document title cannot be null.');
+ });📝 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.
| describe('getTitle integration test with AsyncAPI', () => { | |
| let parsedAsyncAPIDocument; | |
| beforeAll(async () => { | |
| const parseResult = await fromFile(parser, asyncapi_v3_path).parse(); | |
| parsedAsyncAPIDocument = parseResult.document; | |
| }); | |
| it('should return the exact title parameter when exists', () => { | |
| const info = parsedAsyncAPIDocument.info(); | |
| const expectedTitle = info.title(); | |
| const actualTitle = getTitle(parsedAsyncAPIDocument.info()); | |
| expect(actualTitle).toStrictEqual(expectedTitle); | |
| }); | |
| it('should throw error when title function does not exist', () => { | |
| const infoWithoutTitle = { /* missing title property */ }; | |
| expect(() => { | |
| getTitle(infoWithoutTitle); | |
| }).toThrow('Provided AsyncAPI document info field doesn\'t contain title.'); | |
| }); | |
| it('should throw error when title is an empty string', () => { | |
| // Mock an info object where title() returns an empty string | |
| const infoWithEmptyTitle = { | |
| title: () => '' | |
| }; | |
| expect(() => { | |
| getTitle(infoWithEmptyTitle); | |
| }).toThrow('AsyncAPI document title cannot be an empty string.'); | |
| }); | |
| }); | |
| describe('getTitle integration test with AsyncAPI', () => { | |
| let parsedAsyncAPIDocument; | |
| beforeAll(async () => { | |
| const parseResult = await fromFile(parser, asyncapi_v3_path).parse(); | |
| parsedAsyncAPIDocument = parseResult.document; | |
| }); | |
| it('should return the exact title parameter when exists', () => { | |
| const info = parsedAsyncAPIDocument.info(); | |
| const expectedTitle = info.title(); | |
| const actualTitle = getTitle(parsedAsyncAPIDocument.info()); | |
| expect(actualTitle).toStrictEqual(expectedTitle); | |
| }); | |
| it('should throw error when title function does not exist', () => { | |
| const infoWithoutTitle = { /* missing title property */ }; | |
| expect(() => { | |
| getTitle(infoWithoutTitle); | |
| }).toThrow('Provided AsyncAPI document info field doesn\'t contain title.'); | |
| }); | |
| it('should throw error when title is an empty string', () => { | |
| // Mock an info object where title() returns an empty string | |
| const infoWithEmptyTitle = { | |
| title: () => '' | |
| }; | |
| expect(() => { | |
| getTitle(infoWithEmptyTitle); | |
| }).toThrow('AsyncAPI document title cannot be an empty string.'); | |
| }); | |
| it('should throw error when title is null', () => { | |
| // Mock an info object where title() returns null | |
| const infoWithNullTitle = { | |
| title: () => null | |
| }; | |
| expect(() => { | |
| getTitle(infoWithNullTitle); | |
| }).toThrow('AsyncAPI document title cannot be null.'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In packages/helpers/test/utils.test.js between lines 50 and 81, add a new test
case to cover the scenario where the title function returns null. Create a mock
info object with a title method that returns null, then assert that getTitle
throws an error indicating the title cannot be null. This will ensure the
function's behavior matches the PR description for null title values.
|
currently for the test of |
There was a problem hiding this comment.
@ItshMoh currently your getTitle() helper function implementation assumes that info is present in asyncapi document but it maybe case info is not present.
In our implementation of all websocket clients we call getInfo() first and then getTitle() and code is executed line by line so it guarantee that info will exist, but I think we should not consider this assumption and better is to first call getInfo() from getTitle() so that proper error is thrown when someone is using only getTitle() wdyt 🤔 ?
@derberg share your thoughts also.
ItshMoh
changed the title
@Adi-204 you are right, the |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/helpers/src/index.js(2 hunks)packages/helpers/src/utils.js(3 hunks)packages/helpers/test/utils.test.js(2 hunks)packages/templates/clients/websocket/dart/template/client.dart.js(1 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(1 hunks)packages/templates/clients/websocket/python/template/client.py.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/templates/clients/websocket/python/template/client.py.js
- packages/helpers/src/index.js
- packages/helpers/src/utils.js
- packages/helpers/test/utils.test.js
- packages/templates/clients/websocket/javascript/template/client.js.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/templates/clients/websocket/dart/template/client.dart.js (3)
packages/templates/clients/websocket/javascript/template/client.js.js (2)
server(8-8)info(9-9)packages/helpers/src/servers.js (1)
getServer(34-45)packages/helpers/src/utils.js (1)
info(73-73)
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
packages/templates/clients/websocket/dart/template/client.dart.js
[failure] 2-2:
'getTitle' is defined but never used
[failure] 2-2:
'getInfo' is defined but never used
🪛 GitHub Actions: PR testing - if Node project
packages/templates/clients/websocket/dart/template/client.dart.js
[error] 2-2: ESLint: 'getInfo' is defined but never used. (no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
packages/templates/clients/websocket/dart/template/client.dart.js
Outdated
Show resolved
Hide resolved
…in top of getTitle.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/helpers/src/index.js(2 hunks)packages/helpers/src/utils.js(3 hunks)packages/helpers/test/utils.test.js(3 hunks)packages/templates/clients/websocket/dart/template/client.dart.js(1 hunks)packages/templates/clients/websocket/javascript/template/README.md.js(1 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(1 hunks)packages/templates/clients/websocket/python/template/client.py.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/helpers/src/index.js
- packages/templates/clients/websocket/javascript/template/client.js.js
- packages/templates/clients/websocket/dart/template/client.dart.js
- packages/templates/clients/websocket/python/template/client.py.js
- packages/helpers/test/utils.test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/helpers/src/utils.js (5)
apps/hooks/src/index.js (1)
asyncapi(6-6)packages/templates/clients/websocket/dart/template/client.dart.js (2)
info(9-9)title(10-10)packages/templates/clients/websocket/javascript/template/client.js.js (2)
info(9-9)title(10-10)packages/templates/clients/websocket/javascript/template/README.md.js (1)
info(7-7)packages/templates/clients/websocket/python/template/client.py.js (2)
info(9-9)title(10-10)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (3)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
8-8: LGTM! Function call updated correctly.The change from
getClientName(info)togetClientName(asyncapi)correctly aligns with the updated function signature in the utils helper, ensuring consistent usage across templates.packages/helpers/src/utils.js (2)
55-63: Function signature update implemented correctly.The
getClientNamefunction has been properly updated to accept the full AsyncAPI document and internally uses the newgetTitlehelper. This change promotes consistency and better error handling across the codebase.
84-85: Exports updated correctly.Both new helper functions are properly exported, making them available for use in other modules as intended.
packages/templates/clients/websocket/javascript/template/README.md.js
Outdated
Show resolved
Hide resolved
…entName function.
packages/templates/clients/websocket/javascript/template/README.md.js
Outdated
Show resolved
Hide resolved
…E.md.js Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
|
@derberg done |
|
|
/rtm |
4 participants
Description
It adds the new helper in
utils.jswhich is getTitle. It fetches the title parameter from asyncapi info object.It gives an error when the title is undefined or null or empty string.
Related issue(s)
fixes #1559
Summary by CodeRabbit
New Features
Bug Fixes
Tests