-
Notifications
You must be signed in to change notification settings - Fork 263
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
feat(core): add message endpoint for inspector #490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This topic currently needs more discussion so we'll postpone the review for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good Progress :)
I've added some small proposals and we still need to discuss message retention but I think we're pretty much there.
We can probably add a serialiser to the Envelope History List as well that automatically returns the sorted list when we call class EnvelopeHistory(BaseModel):
envelopes: List[EnvelopeHistoryEntry]
@field_serializer("envelopes", when_used="json")
def serialize_envelopes_in_order(
self, envelopes: List[EnvelopeHistoryEntry], _info
):
return sorted(envelopes, key=lambda e: e.timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to move the storage of received messages back into the asgi
module as it seems like it introduces unnecessary overhead by touching the variables twice when storing them.
How about we move the storing action to the asgi right in front of await dispatcher.dispatch(...)
?
And since the envelope is not needed afterwards it would be ok to do the same approach as you did in the dispenser by using the env_dict and reuse that from there on.
|
…ap more pydantic v1 methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good now and feels rounded 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! Just one question / suggestion about the data type for storing the history.
Received and sent local messages can now be retrieved through
http://localhost:<PORT>/messages
endpoint