chore: add python readme #1725
Open
chore: add python readme #1725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thulieblack
requested review from
Adi-204,
asyncapi-bot-eve,
derberg and
magicmatatjahu
as code owners
|
WalkthroughAdds a Python WebSocket README template and five shared generator components (BaseInfo, Overview, Usage, CoreMethods, Installation); refactors the JavaScript WebSocket README template to use these components and centralizes metadata extraction via BaseInfo. Also fixes React key usage in AvailableOperations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)packages/components/Overview.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (1)
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. Comment |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/templates/clients/websocket/python/template/README.md.js (3)
45-47: Document that connect() is async to match the example usage.The sample uses
await ws_client.connect()but the docs don't mention it's async.-#### `connect()` -Establishes a WebSocket connection to the server. +#### `connect()` (async) +Asynchronously establishes a WebSocket connection to the server.
87-93: Avoid referencing a likely non-existent method: send_echo_message.This name is too specific and may not exist in the generated client. Point users to actual operation methods.
- message = 'Hello, Echo!' + message = 'Hello, world!' while True: try: - await ws_client.send_echo_message(message) + await ws_client.<operation_method>(message) # Replace with a generated method from "Available Operations" above except Exception as error: print('Error while sending message:', error) await asyncio.sleep(interval)
77-77: Capitalize “WebSocket” correctly.- print('Errors from Websocket:', getattr(error, 'message', str(error))) + print('Errors from WebSocket:', getattr(error, 'message', str(error)))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/templates/clients/websocket/python/template/README.md.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/templates/clients/websocket/python/template/README.md.js (3)
packages/helpers/src/servers.js (2)
getServer(34-45)getServerUrl(8-22)packages/helpers/src/utils.js (3)
getInfo(9-21)getClientName(53-61)getTitle(32-43)packages/templates/clients/websocket/javascript/components/AvailableOperations.js (1)
AvailableOperations(5-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
packages/templates/clients/websocket/python/template/README.md.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/template/README.md.js
Outdated
Show resolved
Hide resolved
|
@Adi-204 I need to check how the readme renders but all the commands I have in my head are failing can you help me |
|
@thulieblack you can check the templates rendered content by running integration test we have new integration test setup https://github.com/asyncapi/generator/tree/master/packages/templates/clients/websocket/test#integration-testing So basically cd does it answers your question? |
|
Not really, I don't want to run the integration tests, but I want to generate the Python README code locally and see how it displays |
|
@thulieblack you want to see below README.md file right? 
Also when you added this new README.md their should be changes in snapshots which I don't see in this PR. I would suggest you to run |
|
Sorry Adi, could you please walk me through how you did this because I'm not reproducing this from my end. Also, I need to add the Readme to the snapshoot test as well? |
|
@thulieblack can you please share final logs when you run |
|
ok one thing I notice is in this PR ci logs test are failed still overall it is showing pass? 🤔 
cc @derberg can you also please look. |
|
In terms of temp content, this is what I see 
Logs after running After running Let me update my node version |
|
yeah please update the node and after running 
|
|
thank you so much Adi, it worked 
|
|
@thulieblack do you plan to add more commits to this PR? as for now I do not see it addressing the scope of the issue, the decomponentization |
|
Didn't you mention that I need to add create a PR for each component? https://asyncapi.slack.com/archives/C072JMTJ85Q/p1757513137652219?thread_ts=1757501093.164179&cid=C072JMTJ85Q Or do I add it on this PR alongside the new Python readme? |
|
@thulieblack yes, but in your PR you just add a new readme template, no components, and do not reuse from the other readme - so you do not identify common parts and do not create components |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/templates/clients/websocket/python/components/Overview.js (1)
10-12: Skip bullets when metadata is missingWhen the AsyncAPI document has no default server (or the helper fails to resolve one) the README currently prints
- **Server URL:** undefined, which looks broken and undermines the componentization effort. Same risk applies toversionif the input omits it. Better to add those bullets only when the values exist.Proposed tweak:
- {`# ${title} - -## Overview - -${description || `A WebSocket client for ${title}.`} -- **Version:** ${version} -- **Server URL:** ${serverUrl} -`} + {[ + `# ${title}`, + '', + '## Overview', + '', + description || `A WebSocket client for ${title}.`, + version ? `- **Version:** ${version}` : null, + serverUrl ? `- **Server URL:** ${serverUrl}` : null, + ] + .filter(Boolean) + .join('\n')}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/templates/clients/websocket/python/components/Overview.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - windows-latest
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/Usage.js (1)
7-19: Make the Python usage snippet runnable as-is.Line 14 defines
main()but never runs it, so the copied snippet does nothing. Importasyncioand drive the coroutine to give users a working example.Suggested diff:
```python -from ${clientFileName.replace('.py', '')} import ${clientName} - -ws_client = ${clientName}() - -async def main(): - await ws_client.connect() - # use ws_client to send/receive messages - await ws_client.close() +import asyncio + +from ${clientFileName.replace('.py', '')} import ${clientName} + +ws_client = ${clientName}() + +async def main(): + await ws_client.connect() + # use ws_client to send/receive messages + await ws_client.close() + +if __name__ == "__main__": + asyncio.run(main())</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between df08e03ba9f9992fca81361aaee7285ae1f806ec and 170a75f33980293caab144f73a6c6c0d8a65805f. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `packages/components/Overview.js` (1 hunks) * `packages/components/Usage.js` (1 hunks) </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)</summary> * GitHub Check: Acceptance tests for generated templates * GitHub Check: Test generator as dependency with Node 18 * GitHub Check: Test generator as dependency with Node 20 * GitHub Check: Test NodeJS PR - ubuntu-latest * GitHub Check: Test NodeJS PR - windows-latest * GitHub Check: Test NodeJS PR - macos-13 </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/templates/clients/websocket/javascript/template/README.md.js (4)
12-12: Make operations retrieval resilient to parser differences.Wrap
asyncapi.operations().all()to avoid hard failures with older parsers.- const operations = asyncapi.operations().all(); + let operations = []; + try { + operations = asyncapi.operations().all(); + } catch (e) { + operations = []; + }
4-4: Style nit: add a space before the closing brace.-import { CoreMethods} from '../../../../../components/CoreMethods'; +import { CoreMethods } from '../../../../../components/CoreMethods';
3-5: Consider adding Installation for parity with Python README.If the section is generic, import and render it after Overview.
import { Overview } from '../../../../../components/Overview'; -import { CoreMethods} from '../../../../../components/CoreMethods'; +import { Installation } from '../../../../../components/Installation'; +import { CoreMethods } from '../../../../../components/CoreMethods';
19-21: Consider adding Installation for parity with Python README (placement).<Overview info={info} title={title} serverUrl={serverUrl} /> + <Installation /> <Usage clientName={clientName} clientFileName={params.clientFileName} language="javascript" />packages/templates/clients/websocket/python/template/README.md.js (3)
6-6: Style nit: add a space before the closing brace.-import { CoreMethods} from '../../../../../components/CoreMethods'; +import { CoreMethods } from '../../../../../components/CoreMethods';
9-10: Provide a safe default for clientFileName.Avoid passing undefined to
<Usage>; set a Pythonic fallback.const { info, clientName, title, serverUrl } = Info(asyncapi, params); + const clientFileName = params?.clientFileName || 'client.py';
16-16: Use the computed clientFileName fallback.- <Usage clientName={clientName} clientFileName={params.clientFileName} language="python" /> + <Usage clientName={clientName} clientFileName={clientFileName} language="python" />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/templates/clients/websocket/javascript/template/README.md.js(1 hunks)packages/templates/clients/websocket/python/template/README.md.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
packages/templates/clients/websocket/python/template/README.md.js (1)
Info(9-9)
packages/templates/clients/websocket/python/template/README.md.js (1)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
Info(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
🔇 Additional comments (2)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
19-21: Confirm Usage can handle undefined clientFileName.If
params.clientFileNameis optional, ensure<Usage>has a sensible fallback for JS; otherwise precompute a default here.packages/templates/clients/websocket/python/template/README.md.js (1)
13-18: Nice move to componentize the README content.Good separation with Overview, Installation, Usage, and CoreMethods.
packages/templates/clients/websocket/javascript/template/README.md.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/javascript/template/README.md.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/template/README.md.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/components/CoreMethods.js(1 hunks)packages/components/Info.js(1 hunks)packages/components/Installation.js(1 hunks)packages/components/Overview.js(1 hunks)packages/components/Usage.js(1 hunks)packages/templates/clients/websocket/javascript/template/README.md.js(1 hunks)packages/templates/clients/websocket/python/template/README.md.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/templates/clients/websocket/python/template/README.md.js
- packages/components/Overview.js
🧰 Additional context used
🧬 Code graph analysis (3)
packages/components/Usage.js (1)
packages/components/Info.js (1)
clientName(7-7)
packages/components/Info.js (4)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
BaseInfo(9-9)packages/templates/clients/websocket/python/template/README.md.js (1)
BaseInfo(9-9)packages/helpers/src/servers.js (2)
getServer(34-45)getServerUrl(8-22)packages/helpers/src/utils.js (3)
getInfo(9-21)getClientName(53-61)getTitle(32-43)
packages/templates/clients/websocket/javascript/template/README.md.js (4)
packages/components/Info.js (5)
BaseInfo(4-12)title(8-8)info(6-6)serverUrl(9-9)clientName(7-7)packages/components/Overview.js (1)
Overview(3-15)packages/components/Usage.js (1)
Usage(3-46)packages/components/CoreMethods.js (1)
CoreMethods(3-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
packages/components/CoreMethods.js (1)
4-27: Language-specific handler names look good.The snake_case vs camelCase switch matches the Python and JavaScript clients’ APIs.
packages/components/Info.js (1)
4-11: Nice centralization of shared metadata.Pulling
server,info,clientName,title, andserverUrlintoBaseInforemoves duplication across the README templates.packages/templates/clients/websocket/javascript/template/README.md.js (1)
17-17: Trim the extra newline from the title.Line [17] still embeds
\n; withnewLines={2}you end up with one blank line too many in the rendered README.packages/components/Installation.js (1)
6-12: README instruction is valid:requirements.txttemplate exists
The Python WebSocket template includespackages/templates/clients/websocket/python/template/requirements.txt.js, which generatesrequirements.txtwith the listed dependencies.
Comment on lines
+6
to
+33
| if (language === 'python') { | ||
| snippet = ` | ||
| from ${clientFileName.replace('.py', '')} import ${clientName} | ||
| ws_client = ${clientName}() | ||
| async def main(): | ||
| await ws_client.connect() | ||
| # use ws_client to send/receive messages | ||
| await ws_client.close() | ||
| `; | ||
| } else if (language === 'javascript') { | ||
| snippet = ` | ||
| const ${clientName} = require('./${clientFileName.replace('.js', '')}'); | ||
| const wsClient = new ${clientName}(); | ||
| async function main() { | ||
| try { | ||
| await wsClient.connect(); | ||
| // use wsClient to send/receive messages | ||
| await wsClient.close(); | ||
| } catch (error) { | ||
| console.error('Failed to connect:', error); | ||
| } | ||
| } | ||
| main(); | ||
| `; |
There was a problem hiding this comment.
Python usage example never actually runs.
Line [12] defines async def main() but nothing ever drives the event loop—users copying the snippet get no connection attempt. Add the missing asyncio import plus asyncio.run(main()) (or equivalent) so the example is executable.
- snippet = `
-from ${clientFileName.replace('.py', '')} import ${clientName}
+ snippet = `
+import asyncio
+from ${clientFileName.replace('.py', '')} import ${clientName}
ws_client = ${clientName}()
async def main():
await ws_client.connect()
# use ws_client to send/receive messages
await ws_client.close()
+
+if __name__ == "__main__":
+ asyncio.run(main())
`;📝 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
| if (language === 'python') { | |
| snippet = ` | |
| from ${clientFileName.replace('.py', '')} import ${clientName} | |
| ws_client = ${clientName}() | |
| async def main(): | |
| await ws_client.connect() | |
| # use ws_client to send/receive messages | |
| await ws_client.close() | |
| `; | |
| } else if (language === 'javascript') { | |
| snippet = ` | |
| const ${clientName} = require('./${clientFileName.replace('.js', '')}'); | |
| const wsClient = new ${clientName}(); | |
| async function main() { | |
| try { | |
| await wsClient.connect(); | |
| // use wsClient to send/receive messages | |
| await wsClient.close(); | |
| } catch (error) { | |
| console.error('Failed to connect:', error); | |
| } | |
| } | |
| main(); | |
| `; | |
| if (language === 'python') { | |
| snippet = ` | |
| import asyncio | |
| from ${clientFileName.replace('.py', '')} import ${clientName} | |
| ws_client = ${clientName}() | |
| async def main(): | |
| await ws_client.connect() | |
| # use ws_client to send/receive messages | |
| await ws_client.close() | |
| if __name__ == "__main__": | |
| asyncio.run(main()) | |
| `; | |
| } else if (language === 'javascript') { | |
| snippet = ` | |
| const ${clientName} = require('./${clientFileName.replace('.js', '')}'); | |
| const wsClient = new ${clientName}(); | |
| async function main() { | |
| try { | |
| await wsClient.connect(); | |
| // use wsClient to send/receive messages | |
| await wsClient.close(); | |
| } catch (error) { | |
| console.error('Failed to connect:', error); | |
| } | |
| } | |
| main(); | |
| `; |
🤖 Prompt for AI Agents
In packages/components/Usage.js around lines 6 to 33, the Python example defines
async def main() but never runs it; update the snippet to import asyncio at the
top of the Python block and add a call to asyncio.run(main()) (or equivalent) at
the end so the example actually drives the event loop and performs the
connect/close sequence when copied by users.
|
Quick question, is that the correct folder where the components should be? |
|
@thulieblack yeah it should be inside |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of the componetization of the readme, this PR adds a readme to the python template.
relates #1524
Summary by CodeRabbit
New Features
Documentation
Bug Fixes