-
-
Notifications
You must be signed in to change notification settings - Fork 363
feat: add support for reply in python websocket client
#1809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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.
|
|



resolved #1547 that depends on #1548
Reply is possible in cool way with
correlationIdThis PR contains at the moment only changes in fixtures. Slack example nicely shows that it is recommended that for handling
replyit is critical to specifycorrelationIdso it is not only from the docs perspective but also from code generation perspective useful to know how incomming message is correlated with the reply message, with what properties.With
correlationIdin place, you can generate code that is aware of a specific correlationId property and enable access to it through dedicated arguments.Current Limitation
Why dependency?
Current problem is that for clients we generate a generic message handler for a given address. Looking at this from perspective of Slack client, it means we have one handler for all incomming messages,
onHelloMessage,onEvent,onDisconnectMessage. The user of the generated client on their own needs to write code that can help understand what type of message was received by the handler.So if
onEventis the only message that requiresreply, developer needs to write a code inside message handler that understands thatonEventmessage was received, and then reply.At the moment there is nothing useful that could be generated.
I could enable user to do
client.register_reply_handler('onEvent', reply_handler_with_correlation)but what is the value really?message handler could use some new function like:
But we are stuck with generic solutions again, generic message handler and generic reply handler - I don't like it at all.
Reply as standalone operation
Current slack example has:
We could do this to at least enable generation of standalone method to send reply"
But this is controversial and enables errors, for example I can have different channel in reply and a different channel specified in
sendEventAcknowledge. So to be honest best would be to enablereplyobject to simply haveoperationIdor I would just have to generateonEvent+Replyas standalone function.Some discussion under: asyncapi/spec#981
So again, we should first solve #1548 and then we can handle reply topic, because the best user experience would be if I get access to handler like
client.register_onevent_reply_handler(my_reply_method)that allows me to passmy_reply_methodthat would have access to incoming message and correlationId and it's goal would be to return object that would be sent to the server/broker