-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
JSONL stream #39478
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 !
|
||
if decoder is not None: | ||
self._decoder = decoder | ||
elif self._response.headers.get("Content-Type", "").lower() == "application/jsonl": |
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 would fail if the service provided any parameters in the MIME type. Should we make the decoder a required parameter?
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.
You're right - making it required. The generated code should know which flavor of decoder to pass, anyway.
Part of #38806
Implements the design described in https://gist.github.com/kristapratico/d330af39962ea05b10384b865e37b36f
PR adds types for JSONL streaming, SSE will come later.