Skip to content

Use Iterable instead Iterator on iterate_in_threadpool - #2362

Merged
Kludex merged 7 commits into
Kludex:masterfrom
jamesbraza:fixing-async-content-stream-typing
Dec 20, 2023
Merged

Use Iterable instead Iterator on iterate_in_threadpool#2362
Kludex merged 7 commits into
Kludex:masterfrom
jamesbraza:fixing-async-content-stream-typing

Conversation

@jamesbraza

@jamesbraza jamesbraza commented Dec 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Having AsyncContentStream match typing of SyncContentStream, they should both be Iterator or Iterable, not separate.

Given iterate_in_threadpool is typed with Iterator, and returns an AsyncIterator, I think then AsyncContentStream should be an Iterator as well

From mypy inferred types in test_streaming_response_custom_iterable, it seems AsyncIterable is required. So, this PR standardizes on Iterable/AsyncIterable

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@jamesbraza jamesbraza changed the title Fixed AsyncContentStream typing to be AsyncIterator Fixed AsyncContentStream typing to be AsyncIterable Dec 7, 2023
@jamesbraza

jamesbraza commented Dec 11, 2023

Copy link
Copy Markdown
Contributor Author

Cc @tomchristie if you got time to take a look at this one, it's another bug I encountered recently

Kludex
Kludex requested changes Dec 13, 2023
View reviewed changes
Comment thread starlette/concurrency.py Outdated
@lovelydinosaur

lovelydinosaur commented Dec 13, 2023

Copy link
Copy Markdown

@jamesbraza you probably want to check in with other encode maintainers for this project. (I'm focused on httpx, httpcore at the moment.)

@jamesbraza
jamesbraza requested a review from Kludex December 13, 2023 18:14
Kludex
Kludex requested changes Dec 13, 2023
View reviewed changes
Comment thread starlette/concurrency.py Outdated
Comment thread starlette/concurrency.py Outdated
@jamesbraza
jamesbraza requested a review from Kludex December 13, 2023 21:24
Kludex
Kludex reviewed Dec 16, 2023
View reviewed changes
Comment thread starlette/concurrency.py Outdated
@jamesbraza
jamesbraza requested a review from Kludex December 18, 2023 06:31
@Kludex Kludex changed the title Fixed AsyncContentStream typing to be AsyncIterable Use Iterable instead Iterator on iterate_in_threadpool Dec 20, 2023
@Kludex
Kludex merged commit 966f0fc into Kludex:master Dec 20, 2023
@jamesbraza
jamesbraza deleted the fixing-async-content-stream-typing branch December 20, 2023 22:16
@coderabbitai coderabbitai Bot mentioned this pull request Aug 23, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kludex Kludex Kludex approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jamesbraza @lovelydinosaur @Kludex