refactor: improve getClientName helper
#1513
Conversation
Adi-204
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
## Walkthrough
The updates introduce enhanced configurability for client class naming in JavaScript, Python, and Dart WebSocket client templates. The utility function `getClientName` now accepts parameters to control suffix appending and to specify a custom client name, returning the custom name immediately if provided. Corresponding generator parameters (`appendClientSuffix` and `customClientName`) are added to the template configurations. Template files and their test suites are updated to utilize these new options, including new test cases verifying custom naming behavior. Additionally, example imports and README instructions were updated to reflect changes in default client names and testing services.
## Changes
| Files/Paths | Change Summary |
|---------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `packages/helpers/src/utils.js` | Updated `getClientName` to accept `appendClientSuffix` and `customClientName` parameters, altering client name generation logic accordingly. |
| `packages/helpers/test/utils.test.js` | Expanded tests for `getClientName` to cover scenarios with/without suffix and with a custom client name. |
| `packages/templates/clients/websocket/javascript/package.json`<br>`.../python/package.json`<br>`.../dart/package.json` | Added generator parameters: `appendClientSuffix` (boolean, default false) and `customClientName` (optional string) to JavaScript, Python, and Dart client template configurations. |
| `packages/templates/clients/websocket/javascript/template/client.js.js`<br>`.../python/template/client.py.js`<br>`.../dart/template/client.dart.js` | Updated calls to `getClientName` to pass new parameters from template configuration. |
| `packages/templates/clients/websocket/javascript/test/integration.test.js` | Modified and added tests to pass `appendClientSuffix` and test custom client name generation for JavaScript client template. |
| `packages/templates/clients/websocket/python/test/integration.test.js` | Modified and added tests to pass `appendClientSuffix` and test custom client name generation for Python client template. |
| `packages/templates/clients/websocket/dart/test/integration.test.js` | Added tests for Dart client template to verify `appendClientSuffix` and `customClientName` parameters, including new output path and shared output files constant. |
| `packages/templates/clients/websocket/javascript/README.md` | Updated default echo service reference from Postman to Hoppscotch and reversed instructions for switching clients accordingly. |
| `packages/templates/clients/websocket/python/README.md` | Updated dependency installation path and default echo service to Hoppscotch; adjusted instructions for switching to Postman client. |
| `packages/templates/clients/websocket/dart/README.md` | Changed default generated client folder and testing service from Postman to Hoppscotch; updated example modification instructions to switch from Hoppscotch to Postman. |
| `packages/templates/clients/websocket/javascript/example.js` | Changed import path to Hoppscotch client and modified `sendEchoMessage` call from instance method to static method with explicit websocket parameter. |
| `packages/templates/clients/websocket/python/example.py` | Replaced import and instantiation of Postman client with Hoppscotch client. |
| `packages/templates/clients/websocket/dart/example.dart` | Changed import and client instantiation from Postman Echo client to Hoppscotch client. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Generator
participant Template
participant Utils
Generator->>Template: Passes params (appendClientSuffix, customClientName)
Template->>Utils: getClientName(info, appendClientSuffix, customClientName)
Utils-->>Template: Returns client name (custom or generated)
Template-->>Generator: Uses client name in generated codeSuggested labels
Suggested reviewers
Poem
|
|
Hello, @changeset-bot[bot]! 👋🏼 |
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
Tip For best results, initiate chat on the files or code changes. @asyncapi-bot Thanks for the information about your available commands! I appreciate you sharing these helpful PR workflow options. These commands look really useful for managing pull requests efficiently. |
![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
🔭 Outside diff range comments (2)
packages/templates/clients/websocket/python/test/integration.test.js (2)
79-79:⚠️ Potential issueFix incorrect path in the custom client test
The path used to read the generated files is incorrect. You're using
testResultPathHoppscotchinstead oftestResultPathCustomHoppscotch.- const filePath = path.join(testResultPathHoppscotch, testOutputFile); + const filePath = path.join(testResultPathCustomHoppscotch, testOutputFile);
20-20: 💡 Verification agent🧩 Analysis chain
Consider extracting duplicated string literals
There's a linting error about duplicating the client.py string literal. Consider defining a constant for this.
+ const defaultClientFile = 'client.py'; + const defaultRequirementsFile = 'requirements.txt'; + it('generate simple client for postman echo', async () => { - const testOutputFiles = ['client.py', 'requirements.txt']; + const testOutputFiles = [defaultClientFile, defaultRequirementsFile];Similarly, update the other occurrences in the file.
🏁 Script executed:
#!/bin/bash # Find all occurrences of client.py and requirements.txt literals rg "'client.py'" --type js --context 2 rg "'requirements.txt'" --type js --context 2Length of output: 3458
Extract duplicated file name literals into constants
The
integration.test.jsfile reuses the['client.py', 'requirements.txt']array in three separate tests. This triggers a lint error for duplicate string literals and makes future updates harder. Extract these strings into constants at the top of the file and reference them in each test.• File: packages/templates/clients/websocket/python/test/integration.test.js
• Tests to update:
- generate simple client for postman echo
- generate simple client for hoppscotch echo
- generate simple client for hoppscotch echo with custom client name
Proposed changes:
+// At the top of the file +const defaultClientFile = 'client.py'; +const defaultRequirementsFile = 'requirements.txt'; + it('generate simple client for postman echo', async () => { - const testOutputFiles = ['client.py', 'requirements.txt']; + const testOutputFiles = [defaultClientFile, defaultRequirementsFile]; // ... }); it('generate simple client for hoppscotch echo', async () => { - const testOutputFiles = ['client.py', 'requirements.txt']; + const testOutputFiles = [defaultClientFile, defaultRequirementsFile]; // ... }); it('generate simple client for hoppscotch echo with custom client name', async () => { - const testOutputFiles = ['client.py', 'requirements.txt']; + const testOutputFiles = [defaultClientFile, defaultRequirementsFile]; // ... });🧰 Tools
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
[failure] 20-20:
Define a constant instead of duplicating this literal 3 times🪛 GitHub Actions: PR testing - if Node project
[error] 20-20: ESLint: Define a constant instead of duplicating this literal 3 times (sonarjs/no-duplicate-string)
🧹 Nitpick comments (2)
packages/templates/clients/websocket/python/template/client.py.js (1)
7-23:❓ Verification inconclusive
Consider implementing a default value for appendClientSuffix
Similar to the JavaScript template, the code doesn't handle the case where
params.appendClientSuffixis undefined. Consider providing a default value to ensure consistent behavior.- const clientName = getClientName(info, params.appendClientSuffix, params.customClientName); + const clientName = getClientName(info, params.appendClientSuffix ?? false, params.customClientName);
🏁 Script executed:
#!/bin/bash # Check if default values are defined in package.json echo "Checking for default params in Python template package.json" fd --glob "package.json" --path packages/templates/clients/websocket/python | xargs cat | grep -A 10 "generatorParameters"Length of output: 472
#!/bin/bash echo "Checking for package.json in Python WebSocket template directories" for dir in packages/templates/clients/websocket/python packages/templates/clients/websocket/python/template; do if [ -f "$dir/package.json" ]; then echo "----- Found $dir/package.json -----" echo "appendClientSuffix entries:" grep -R "appendClientSuffix" -n "$dir/package.json" || echo " (none)" echo echo "generatorParameters section:" grep -R "generatorParameters" -n "$dir/package.json" -C 5 || echo " (none)" echo else echo "No package.json at $dir" fi done echo "Searching all package.json under packages/templates for appendClientSuffix defaults" find packages/templates -type f -name "package.json" -exec grep -R "appendClientSuffix" -n {} \; || echo "(no occurrences)"
#!/bin/bash echo "Scanning all package.json under packages/templates for default appendClientSuffix settings..." # Find package.json files and inspect generatorParameters and appendClientSuffix find packages/templates -type f -name "package.json" -print | while read file; do echo "----- $file -----" echo "generatorParameters section (if any):" grep -n '"generatorParameters"' -C2 "$file" || echo " (none)" echo echo "appendClientSuffix default (if defined):" grep -n '"appendClientSuffix"' -C2 "$file" || echo " (none)" echo done echo "Also searching template code for usage of default values:" rg --color=never "appendClientSuffix" -n packages/templates/clients/websocket/python
#!/bin/bash echo "Scanning entire repository for 'appendClientSuffix' occurrences and defaults" rg --color=never -n '"appendClientSuffix"' .
Ensure appendClientSuffix defaults to false
The Python WebSocket template currently passes
params.appendClientSuffixdirectly, which can be undefined and lead to inconsistent client names. To align with the JavaScript template, default it tofalse:• File: packages/templates/clients/websocket/python/template/client.py.js
- const clientName = getClientName(info, params.appendClientSuffix, params.customClientName); + const clientName = getClientName(info, params.appendClientSuffix ?? false, params.customClientName);Please verify that the template’s parameter definitions (e.g., in package.json or your generator schema) don’t already supply a default for
appendClientSuffix.packages/helpers/test/utils.test.js (1)
17-21: Consider adding test for undefined parametersThe tests don't cover cases where parameters might be undefined or null. Consider adding tests for these edge cases to ensure the function handles them gracefully.
it('should handle undefined parameters gracefully', () => { const info = parsedAsyncAPIDocument.info(); // Don't pass appendClientSuffix and customClientName const clientName = getClientName(info); // Should default to a sensible behavior expect(clientName).toBe('GeminiMarketDataWebsocketAPI'); // or whatever is expected });Also applies to: 28-32, 39-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/templates/clients/websocket/javascript/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/helpers/src/utils.js(1 hunks)packages/helpers/test/utils.test.js(1 hunks)packages/templates/clients/websocket/javascript/package.json(1 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(1 hunks)packages/templates/clients/websocket/javascript/test/integration.test.js(3 hunks)packages/templates/clients/websocket/python/package.json(1 hunks)packages/templates/clients/websocket/python/template/client.py.js(1 hunks)packages/templates/clients/websocket/python/test/integration.test.js(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR testing - if Node project
packages/templates/clients/websocket/python/test/integration.test.js
[error] 20-20: ESLint: Define a constant instead of duplicating this literal 3 times (sonarjs/no-duplicate-string)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (19)
packages/helpers/src/utils.js (3)
2-3: JSDoc updated correctly to reflect new parametersThe documentation has been properly updated to describe the new functionality: custom client naming and optional suffix appending.
Also applies to: 5-6, 8-8
10-13: Implementation of custom client name support looks goodThe new conditional logic correctly prioritizes the custom client name when provided.
18-18: Good implementation of conditional suffixThe ternary operator provides a clean way to conditionally append the "Client" suffix.
packages/templates/clients/websocket/javascript/template/client.js.js (2)
11-11: Correctly updated to pass new parameters to getClientNameThe function call has been properly updated to pass the new parameters from the template configuration to the helper function.
7-23:❌ Incorrect review comment
Consider implementing a default value for appendClientSuffix
The code doesn't handle the case where
params.appendClientSuffixis undefined. Consider providing a default value to ensure consistent behavior even if the parameter is not explicitly set.- const clientName = getClientName(info, params.appendClientSuffix, params.customClientName); + const clientName = getClientName(info, params.appendClientSuffix ?? false, params.customClientName);
🏁 Script executed:
#!/bin/bash # Check if default values are defined in package.json echo "Checking for default params in JavaScript template package.json" fd --glob "package.json" --path packages/templates/clients/websocket/javascript | xargs cat | grep -A 10 "generatorParameters"Length of output: 484
🏁 Script executed:
#!/bin/bash # List files in the JavaScript template directory and its first-level subdirectories echo "Files in packages/templates/clients/websocket/javascript:" find packages/templates/clients/websocket/javascript -maxdepth 2 -type f echo # Search for references to appendClientSuffix echo "Searching for 'appendClientSuffix' references..." rg "appendClientSuffix" -n packages/templates/clients/websocket/javascript || echo "No occurrences found" echo # Search for default value declarations in JSON/YAML files echo "Searching for 'default' properties in template definition files..." rg '"default"' -n packages/templates/clients/websocket/javascript || echo "No default declarations found"Length of output: 2737
(Note: Adjusted line numbers to capture parameter definitions.)
🏁 Script executed:
#!/bin/bash # Display lines around the appendClientSuffix parameter in package.json echo "Context for 'appendClientSuffix' in package.json:" sed -n ' sixty, ninetyp' packages/templates/clients/websocket/javascript/package.jsonLength of output: 259
🏁 Script executed:
#!/bin/bash # Show context for appendClientSuffix in package.json with surrounding lines rg -n -C 5 '"appendClientSuffix"' packages/templates/clients/websocket/javascript/package.jsonLength of output: 471
No change needed for appendClientSuffix default
The
appendClientSuffixparameter already has a default value offalsedefined in
packages/templates/clients/websocket/javascript/package.json(lines 71–74). The template will always receive a boolean forparams.appendClientSuffix, so no additional default handling is required in the code.Likely an incorrect or invalid review comment.
packages/templates/clients/websocket/python/template/client.py.js (1)
11-11: Correctly updated to pass new parameters to getClientNameThe function call has been properly updated to pass the new parameters from the template configuration to the helper function.
packages/helpers/test/utils.test.js (3)
16-25: Good test for client name without suffixThis test properly verifies the new functionality of generating a client name without appending the "Client" suffix.
27-36: Good test for client name with suffixThis test properly verifies the original functionality of generating a client name with the "Client" suffix.
38-47: Good test for custom client nameThis test properly verifies the new functionality of using a custom client name.
packages/templates/clients/websocket/python/package.json (1)
71-79: Great addition of configurable naming parameters!The new parameters
appendClientSuffixandcustomClientNameprovide excellent flexibility for client class naming. The descriptions are clear, and the default values are appropriately set.packages/templates/clients/websocket/javascript/package.json (1)
71-79: Consistent implementation across templates!The parameter additions match those in the Python template, maintaining consistency across different language implementations.
packages/templates/clients/websocket/javascript/test/integration.test.js (4)
24-24: Explicitly setting the new parameterGood job explicitly setting
appendClientSuffix: falseto match the default value in the package.json, making the test intent clear.
41-43: Explicitly setting the new parameterGood job explicitly setting
appendClientSuffix: falseto match the default value in the package.json, making the test intent clear.
52-62: Good test coverage for the new custom client name parameterThe new test case properly verifies the custom client name functionality, ensuring that when
customClientNameis provided, it is correctly used in the generated output.
78-78: Explicitly setting the new parameterGood job explicitly setting
appendClientSuffix: falseto match the default value in the package.json, making the test intent clear.packages/templates/clients/websocket/python/test/integration.test.js (4)
13-13: Well-defined constant for the new test caseGood practice defining a separate path constant for the custom client name test to avoid file conflicts.
27-28: Explicitly setting the new parameterGood job explicitly setting
appendClientSuffix: falseto match the default value in the package.json, making the test intent clear.
48-50: Explicitly setting the new parameterGood job explicitly setting
appendClientSuffix: falseto match the default value in the package.json, making the test intent clear.
63-73: Good test coverage for the new custom client name parameterAdding a dedicated test case for custom client name functionality ensures proper coverage of the new feature.
|
@derberg any more suggestion? |
| "default": "client.js" | ||
| }, | ||
| "appendClientSuffix": { | ||
| "description": "Add Client at the end of the class name", |
There was a problem hiding this comment.
| "description": "Add Client at the end of the class name", | |
| "description": "Add 'Client' suffix at the end of the class name", |
I think the original version could be confuse people in the sense that they may not quickly understand that "Client" here refers to the english word, and not a code object.
Same for the python package.
packages/helpers/test/utils.test.js
Outdated
| const clientName = getClientName(info, appendClientSuffix, customClientName); | ||
|
|
||
| // Example assertion: Check if the name is formatted correctly | ||
| expect(clientName).toBe('GeminiClient'); |
There was a problem hiding this comment.
| expect(clientName).toBe('GeminiClient'); | |
| expect(clientName).toBe(customClientName); |
Using the constant instead of a hard-coded value, in case someone changes the constant's value.
|
@nightknighto changes done! @derberg any suggestion from your side? |
packages/templates/clients/websocket/javascript/test/integration.test.js
Outdated
Show resolved
Hide resolved
|
@derberg yes I agree it make sense 👍. |
|
can you update this PR then? or you prefer a followup? |
|
@derberg yes I can make your changes and can I also make channges in dart client adding this 2 parameters in that one as it is merged. wdyt 🤔? If it is easy to review for you then only. |
|
sure btw, another thing that must be replicated in 3 different places - something to think about |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/templates/clients/websocket/javascript/README.md (1)
9-9: Default testing service updated to HoppscotchThe documentation has been correctly updated to reflect the change from Postman to Hoppscotch as the default echo service for testing.
Consider these minor grammatical improvements:
-> By default this is testing against Hoppscotch echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change first line to `const WSClient = require('./tests/temp/snapshotTestResult/client-postman.js');` and run `node example.js` again. You will see example still works but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. +> By default, this is testing against the Hoppscotch echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change the first line to `const WSClient = require('./tests/temp/snapshotTestResult/client-postman.js');` and run `node example.js` again. You will see the example still works, but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test:node example.js> By default this is testing against Hoppscotch echo...(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...(COMMA_COMPOUND_SENTENCE)
packages/templates/clients/websocket/python/README.md (1)
12-12: Fix grammatical issues in the instructionsThe instructions have been updated to correctly reflect the change from Postman to Hoppscotch as the default echo service. However, there are a few minor grammatical issues to address.
-> By default this is testing against Hoppscotch echo service. You can modify `packages/templates/clients/websocket/python/example.py` and change line 7 to `from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient` and line 16 `client = PostmanEchoWebSocketClientClient()` and run `python example.py` again. You will see example still works but agains different API. This is possible as both AsyncAPI documents have the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. +> By default, this is testing against Hoppscotch echo service. You can modify `packages/templates/clients/websocket/python/example.py` and change line 7 to `from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient` and line 16 `client = PostmanEchoWebSocketClientClient()` and run `python example.py` again. You will see the example still works but against a different API. This is possible as both AsyncAPI documents have the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Did you mean: “By default,”?
Context: ...ted by the test:python example.py> By default this is testing against Hoppscotch echo...(BY_DEFAULT_COMMA)
[uncategorized] ~12-~12: You might be missing the article “the” here.
Context: ...python example.pyagain. You will see example still works but agains different API. T...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/dart/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
packages/templates/clients/websocket/dart/README.md(1 hunks)packages/templates/clients/websocket/dart/example.dart(2 hunks)packages/templates/clients/websocket/dart/package.json(1 hunks)packages/templates/clients/websocket/dart/template/client.dart.js(1 hunks)packages/templates/clients/websocket/dart/test/integration.test.js(2 hunks)packages/templates/clients/websocket/javascript/README.md(1 hunks)packages/templates/clients/websocket/javascript/example.js(2 hunks)packages/templates/clients/websocket/python/README.md(1 hunks)packages/templates/clients/websocket/python/example.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/templates/clients/websocket/dart/README.md
🧰 Additional context used
🪛 LanguageTool
packages/templates/clients/websocket/javascript/README.md
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test: node example.js > By default this is testing against Hoppscotch echo...
(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...
(COMMA_COMPOUND_SENTENCE)
packages/templates/clients/websocket/python/README.md
[uncategorized] ~10-~10: You might be missing the article “an” here.
Context: ...t_hoppscotch/requirements.txt` 6. Start example script that uses a client library gener...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~12-~12: Did you mean: “By default,”?
Context: ...ted by the test: python example.py > By default this is testing against Hoppscotch echo...
(BY_DEFAULT_COMMA)
[uncategorized] ~12-~12: You might be missing the article “the” here.
Context: ...python example.py again. You will see example still works but agains different API. T...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (11)
packages/templates/clients/websocket/javascript/example.js (1)
1-1: Import path updated to use custom Hoppscotch clientThe import has been changed from Postman to a custom Hoppscotch client, which aligns with the PR objective of providing customization options for client names.
packages/templates/clients/websocket/dart/template/client.dart.js (1)
11-11: Well-implemented enhancement to the client name generationThe updated
getClientNamecall properly passes the new parameters (appendClientSuffixandcustomClientName) from the template configuration, enabling greater flexibility in customizing the generated client class name as intended by the PR objectives.packages/templates/clients/websocket/dart/example.dart (2)
2-2: Import path updated to reflect custom client exampleThe import path has been correctly updated to use the custom Hoppscotch client, which aligns with the integration tests and demonstrates the new client naming functionality.
15-15: Client instantiation updated to use the new custom clientThe client instantiation has been properly updated to use
HoppscotchClient()which corresponds to the custom client name specified in the integration tests.packages/templates/clients/websocket/dart/test/integration.test.js (4)
13-13: Path constant added for custom client testGood addition of a dedicated path constant for the custom client test, maintaining consistency with the existing path constants structure.
16-17: Improved code maintainability through constant extractionExcellent refactoring to extract the output files into a reusable constant, which reduces duplication and improves maintainability.
27-27: Postman client test now verifies suffix appendingThe test has been correctly updated to include the
appendClientSuffixparameter, ensuring that this new functionality is properly tested.
60-77: Comprehensive test added for custom client name functionalityThis new test case thoroughly verifies the
customClientNameparameter functionality by generating a Hoppscotch client with a custom name and validating the output against snapshots. The test structure follows the established pattern and properly exercises the new functionality.packages/templates/clients/websocket/dart/package.json (1)
71-79: Well-defined parameters for enhanced client naming functionalityThe new parameters are properly defined with clear descriptions, appropriate defaults, and explicit precedence rules. They directly implement the PR objectives of:
appendClientSuffix- Allowing the "Client" suffix to be appendedcustomClientName- Enabling a completely custom client class nameThe description correctly notes that the suffix option has no effect when a custom name is specified, establishing a clear precedence order.
packages/templates/clients/websocket/python/example.py (1)
3-3: Consistent update for the client migration from Postman to HoppscotchThe changes properly update the imports and client instantiation to use the new Hoppscotch client instead of the Postman client. This aligns with the PR objective of improving the
getClientNamehelper function to support custom client names.Also applies to: 7-7, 21-21
packages/templates/clients/websocket/python/README.md (1)
9-9: Path correctly updated to match the new client locationThe requirements.txt path has been properly updated to reflect the new location of the generated client files.
| while (true) { | ||
| try { | ||
| await wsClient.sendEchoMessage(message); | ||
| await WSClient.sendEchoMessage(message, wsClient.websocket); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Method call changed from instance to static
The method call for sendEchoMessage has been changed from an instance method to a static method requiring the websocket instance as a parameter. This represents a significant API change.
Please verify that this API change is intentional and documented elsewhere. Static methods provide different benefits but changing from instance to static methods can break backward compatibility for existing users.
🏁 Script executed:
#!/bin/bash
# Check if this API change is documented in changelog or other documentation
fd -t f "CHANGELOG.md|BREAKING.md" --exec grep -l "sendEchoMessage" {} \;
# Check if there are any other example files that might need updating
fd -t f "example" -e js --exec grep -l "sendEchoMessage" {} \;
# Check implementation of sendEchoMessage in both Postman and Hoppscotch clients
fd -t f "client-postman.js|custom-client-hoppscotch.js" --exec grep -A 10 "sendEchoMessage" {} \;Length of output: 302
Document Breaking Change for sendEchoMessage
The call to sendEchoMessage was converted from an instance method (wsClient.sendEchoMessage(message)) to a static method (WSClient.sendEchoMessage(message, wsClient.websocket)). No changelog entry or documentation update was found.
Please add or update the following:
- CHANGELOG.md (or BREAKING.md) under “Breaking Changes” to note the new static signature and deprecation of the old instance API.
- Library README or API docs with the updated usage example.
- All other client templates/examples (e.g. Postman, Hoppscotch) to use the new static call.
- (Optionally) A deprecation warning in code for the old instance method if we want to maintain backward‐compatibility.
|
@derberg summary of my last commit changes -
Didn't change anything in microcks for now because I think we should wait to solve issue - #1507 so that we can check properly the new changes done are working or not. I hope this comment makes your review life easy 😄! |
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/templates/clients/websocket/python/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/templates/clients/websocket/python/README.md
[uncategorized] ~10-~10: You might be missing the article “an” here.
Context: ...t_hoppscotch/requirements.txt` 6. Start example script that uses a client library gener...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~12-~12: Did you mean: “By default,”?
Context: ...ted by the test: python example.py > By default this is testing against Hoppscotch echo...
(BY_DEFAULT_COMMA)
[uncategorized] ~12-~12: You might be missing the article “the” here.
Context: ...python example.py again. You will see example still works but agains different API. T...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ 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: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (1)
packages/templates/clients/websocket/python/README.md (1)
9-9: Approve updated dependency path
The change to referencetemp/snapshotTestResult/custom_client_hoppscotch/requirements.txtaligns with the new output directory structure for custom client names.
| 6. Start example script that uses a client library generated by the test: `python example.py` | ||
|
|
||
| > By default this is testing against Postman echo service. You can modify `packages/templates/clients/websocket/python/example.py` and change first line to `from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient` and line 16 `client = PostmanEchoWebSocketClientClient()` and run `python example.py` again. You will see example still works but agains different API. This is possible as both AsyncAPI documents have the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. | ||
| > By default this is testing against Hoppscotch echo service. You can modify `packages/templates/clients/websocket/python/example.py` and change line 7 to `from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient` and line 21 `client = PostmanEchoWebSocketClientClient()` and run `python example.py` again. You will see example still works but agains different API. This is possible as both AsyncAPI documents have the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. |
There was a problem hiding this comment.
Fix typos and improve clarity in the example instructions
There are a few minor grammar and spelling issues in the blockquote (missing comma after “By default”, typo “agains”, and a missing “that”/“the” for clarity). Please apply the following diff to correct these:
- > By default this is testing against Hoppscotch echo service. You can modify `packages/templates/clients/websocket/python/example.py` and change line 7 to `from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient` and line 21 `client = PostmanEchoWebSocketClientClient()` and run `python example.py` again. You will see example still works but agains different API. This is possible as both AsyncAPI documents have the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API.
+ > By default, this is testing against the Hoppscotch echo service. You can modify `packages/templates/clients/websocket/python/example.py` by updating line 7 to:
+ > ```python
+ > from temp.snapshotTestResult.client_postman.client import PostmanEchoWebSocketClientClient
+ > ```
+ > and line 21 to:
+ > ```python
+ > client = PostmanEchoWebSocketClientClient()
+ > ```
+ > Then run `python example.py` again. You will see that the example still works but against a different API. This is possible because both AsyncAPI documents share the same operation name for sending messages (`sendEchoMessage`), so each client generated has the same API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Did you mean: “By default,”?
Context: ...ted by the test: python example.py > By default this is testing against Hoppscotch echo...
(BY_DEFAULT_COMMA)
[uncategorized] ~12-~12: You might be missing the article “the” here.
Context: ...python example.py again. You will see example still works but agains different API. T...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
| while (true) { | ||
| try { | ||
| await wsClient.sendEchoMessage(message); | ||
| await WSClient.sendEchoMessage(message, wsClient.websocket); |
There was a problem hiding this comment.
can you explain why this changed if the implementation of send message picks websocket instance if one is not provided
There was a problem hiding this comment.
@derberg
we set this.websocket on the instance (wsClient.websocket) when call connect(), but we never set it on the class itself. In a static method this refers to the HoppscotchClient itself, not to wsClient instance.
So basically HoppscotchClient.websocket = undefined but wsClient.websocket is not undefined.
Basically this in static method works differently.
There was a problem hiding this comment.
so | | operator doesn't really make sense because this.websocket will always be undefined and if socket not passed it will also be undefined. Should we remove this.websocket or some other approach to take we can discuss?
wdyt 🤔?
There was a problem hiding this comment.
we could do something like
static sendEchoMessage(message, socket) {
socket.send(JSON.stringify(message));
}
sendEchoMessage(message) {
PostmanEchoWebSocketClientClient.sendEchoMessage(message, this.websocket);
}
but then you need to make sure PostmanEchoWebSocketClientClient value is generated.
let us leave what you have already and please just create a followup issue
|
/rtm |
|
@allcontributors please add @Adi-204 for bug, code, ideas, infra, review, test, doc |
|
I've put up a pull request to add @Adi-204! 🎉 |
4 participants
Description
improve
getClientNamehelper by adding 2 new params in template generation process.Related issue(s)
Fixes - #1510
Summary by CodeRabbit