-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Amend RFC 2996 to replace Stream
with AsyncIterator
#3208
Conversation
ef850a8
to
8e0c995
Compare
This seems to reflect the API shift we've been talking about for a while and have been assuming would happen. Thanks for writing this up! I'm going to start an FCP to confirm consensus on this renaming. @rfcbot merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
This does make sense to me; when I hear "stream" I think something different. That said, should this add a section to the Rationale & Alternatives saying why this name was chosen? |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
That's a good point. I think we should. Would it be okay if I file a follow-up PR so we can discuss the exact wording there without holding up the FCP in this PR? |
I think it's perfectly fine to add more "why" details during the FCP; they're not like bors where a new commit requires a new approval. (Changing the design can sometimes need a new FCP, but if it's just recording rationale and alternatives that doesn't invalidate checkboxes IMHO.) So since it'll take 10 days for the FCP to complete anyway, I'd suggest just updating this PR. But later is totally fine too. (And I'm not on libs-api, so nothing I say here is authoritative.) |
Ah, okay - I'll do that then! |
This PR seems like a good change to me. While I like "Streams" better as a name (it's shorter and sounds better to my ears), it also conveys to me very little about what Streams actually are. I think AsyncIterator is much clearer in that it immediately gives some idea what it refers to, so it makes sense to prioritize clarity in my opinion. |
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.
Thanks for putting this up, @yoshuawuyts.
This reflects the general approach we've been talking about for awhile to stick to AsyncFoo
naming for any trait Foo
which has a direct synchronous analogue in the standard library. While Stream
is definitely nicer to type than AsyncIterator
, I agree with @eholk that having clarity is important, and I think internal consistency and predictability are important too.
8e0c995
to
86da76c
Compare
Added a section on naming, as requested. |
1a9e51d
to
15cb79e
Compare
Big fan. |
...Does this mean to say the iterator's elements are iterated over in a non-serial "out of order" fashion, so we are no longer required to iterate in a "eager" or "sync" fashion in order to execute an item, or that the iterator is iterated over serially, but the items returned are Futures? |
@workingjubilee the iterator is iterated over serially, but the items returned are Futures. From the summary section of the RFC:
|
Those actually describe the same thing to my understanding, |
You must consume the items of an To restate what @Nemo157 said (partly for my own understanding): It's true that in both I feel like there should be a table for all this :) |
Yes, I think the "other interpretation" I had in mind was either
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Move `{core,std}::stream::Stream` to `{core,std}::async_iter::AsyncIterator` Following amendments in rust-lang/rfcs#3208. cc rust-lang#79024 cc `@yoshuawuyts` `@joshtriplett`
Move `{core,std}::stream::Stream` to `{core,std}::async_iter::AsyncIterator` Following amendments in rust-lang/rfcs#3208. cc rust-lang#79024 cc ``@yoshuawuyts`` ``@joshtriplett``
RENDERED
Tracking Issue
This PR updates
RFC 2996: Async Stream
to use "async iterator" terminology instead. This was brought up on theasync_stream
tracking issue by @joshtriplett, capturing a conversation that happened on Zulip. The proposal has since seen quite a bit of support both from the ecosystem and async foundations working group.Additionally there is also strong precedence in other languages for using an "iterator"/"async iterator" naming scheme:
Symbol.Iterator
,Symbol.AsyncIterator
IEnumerable
,IAsyncEnumerable
__iter__
,__aiter__
Sequence
,AsyncSequence
Hence I'd like to propose this amendment to
RFC 2996: Async Stream
to use "async iterator" terminology instead. Thanks!cc/ @rust-lang/libs-api, @rust-lang/wg-async-foundations @nellshamrell