-
Notifications
You must be signed in to change notification settings - Fork 3k
JSONL stream #39478
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: main
Are you sure you want to change the base?
JSONL stream #39478
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.
overall looks great, thanks @kristapratico !
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.
Looks good to me. Can you also add a changelog entry?
def json(self) -> Any: | ||
"""Parse the event data as JSON. | ||
|
||
:return: The parsed JSON data. |
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 had to look up how json.load
documents this (because "The parsed JSON data" is somewhat ambiguous and the return type hint isn't that helpful here :) and it says:
Deserialize fp to a Python object using the JSON-to-Python conversion table.
I think what we have here is more of an as_dict
method.
But having that on the base EventType
is probably wrong, because what if the event is not some form of structured data? E.g. SSE with just vanilla strings (and not json strings - which have to be quoted/escaped)
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.
Removed the EventTypes / assumptions of JSON being the only type of event data.
…t kw on Stream (more of an sse thing)
Part of #38806
Implements the design described in https://gist.github.com/kristapratico/d330af39962ea05b10384b865e37b36f
PR adds types for JSONL streaming, SSE will come later.