FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: add support for asynchronous rest streaming by ohmayr · Pull Request #686 · googleapis/python-api-core · GitHub

This repository was archived by the owner on Feb 23, 2026. It is now read-only.
/ python-api-core Public archive

feat: add support for asynchronous rest streaming - #686

Merged
ohmayr merged 30 commits into
mainfrom
add-support-for-async-rest-streaming
Sep 18, 2024
ConversationCommits (30)ChecksFiles changed
Merged

feat: add support for asynchronous rest streaming#686
ohmayr merged 30 commits into
mainfrom
add-support-for-async-rest-streaming

Conversation

ohmayr commented Aug 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

product-auto-label Bot added the size: l Pull request size is large. label Aug 11, 2024
ohmayr marked this pull request as ready for review August 20, 2024 15:45
ohmayr requested review from a team August 20, 2024 15:45
ohmayr changed the base branch from async-rest-support-in-core to main August 22, 2024 17:20
vchudnov-g reviewed Aug 22, 2024
View reviewed changes

vchudnov-g left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ohmayr force-pushed the add-support-for-async-rest-streaming branch from 29d662b to fdeb437 Compare August 23, 2024 15:26
vchudnov-g reviewed Aug 23, 2024
View reviewed changes

vchudnov-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good!

Comment thread google/api_core/_rest_streaming_base.py Outdated
Comment thread google/api_core/_rest_streaming_base.py Outdated
Comment thread google/api_core/_rest_streaming_base.py Outdated
Comment thread google/api_core/rest_streaming.py
Comment thread google/api_core/rest_streaming_async.py Outdated
Comment thread tests/unit/test_rest_streaming_async.py
Comment thread tests/unit/test_rest_streaming_async.py Outdated
Comment thread tests/unit/test_rest_streaming_async.py Outdated
Comment thread tests/unit/test_rest_streaming_async.py
Comment thread tests/unit/test_rest_streaming_async.py
ohmayr mentioned this pull request Jan 12, 2026
Open

ohmayr commented Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

I will move test_rest_streaming_async.py from tests/unit to tests/asyncio which is where we have our async tests once we do not have any other open concerns.

vchudnov-g approved these changes Aug 26, 2024
View reviewed changes

vchudnov-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please address the comments, but no blockers. Thanks for doing this!

Comment thread google/api_core/rest_streaming.py
Comment thread google/api_core/_rest_streaming_base.py
Comment thread 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`.

vchudnov-g Aug 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Where is this exception raised? Line 46 will succeed with a matching method of any class, right?

ohmayr Sep 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it's reasonable to not raise an error for this. We've added the relevant type hints. Cleaned up the docstring.

Comment thread tests/unit/test_rest_streaming_async.py
Comment thread tests/helpers.py Outdated
ohmayr added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 27, 2024
ohmayr commented Sep 2, 2024
View reviewed changes
Comment thread google/api_core/rest_streaming_async.py Outdated
ohmayr removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 18, 2024
ohmayr merged commit 1b7bb6d into main Sep 18, 2024
ohmayr deleted the add-support-for-async-rest-streaming branch September 18, 2024 15:36
release-please Bot mentioned this pull request Sep 18, 2024
Merged
This was referenced May 30, 2025
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
vchudnov-g vchudnov-g approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

vchudnov-g

Labels

size: l Pull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL