-
Notifications
You must be signed in to change notification settings - Fork 88
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: add support for asynchronous rest streaming #686
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.
Could you "fork" the file you're refactoring from into the file you're refacotring "to", adapting the dirctions in https://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 ? It will make it easier to review, and preserve git history for the future.
29d662b
to
fdeb437
Compare
…gleapis/python-api-core into add-support-for-async-rest-streaming
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!
I will move |
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.
Please address the comments, but no blockers. Thanks for doing this!
google/api_core/rest_streaming.py
Outdated
ValueError: If `response_message_cls` is not a subclass of `proto.Message` or `google.protobuf.message.Message`. | ||
ValueError: | ||
- If `response_message_cls` is not a subclass of `proto.Message` or `google.protobuf.message.Message`. | ||
- If `response` is not an instance of `requests.Response`. |
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.
Where is this exception raised? Line 46 will succeed with a matching method of any class, right?
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 think it's reasonable to not raise an error for this. We've added the relevant type hints. Cleaned up the docstring.
…gleapis/python-api-core into add-support-for-async-rest-streaming
This PR adds support for asynchronous rest streaming in core to be leveraged in GAPICs.
The changes in this PR can be tested against: googleapis/google-auth-library-python#1577.