Skip to content
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

Rename try_stream to drain #96

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Rename try_stream to drain #96

merged 1 commit into from
Mar 16, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Mar 10, 2024

At the suggestion of @i509VCB

At the suggestion of @i509VCB

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull requested a review from taiki-e March 10, 2024 03:23
@taiki-e
Copy link
Collaborator

taiki-e commented Mar 10, 2024

Thanks. I think this is much clearer than the previous one, but I would like to hear @smol-rs/admins's opinions.

@zeenix
Copy link
Member

zeenix commented Mar 10, 2024

Thanks. I think this is much clearer than the previous one

Although I agree but seems the current name was chosen for consistency with std:

/// This is intended to be used as a way of polling a stream without waiting, similar to the
/// [try_iter] function on [std::sync::mpsc::Receiver]. For instance, running this stream
/// on an [async_channel::Receiver] will return all messages that are currently in the
/// channel, but will not wait for new messages.

@zeenix
Copy link
Member

zeenix commented Mar 10, 2024

If we do this, we should probably do it in a backwards-compatible way using type aliases.

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 10, 2024

If we do this, we should probably do it in a backwards-compatible way using type aliases.

That is not necessary because the old name has not been released. (See #94 for the context of this PR.)

@notgull
Copy link
Member Author

notgull commented Mar 11, 2024

/// This is intended to be used as a way of polling a stream without waiting, similar to the
/// [try_iter] function on [std::sync::mpsc::Receiver]. For instance, running this stream
/// on an [async_channel::Receiver] will return all messages that are currently in the
/// channel, but will not wait for new messages.

Unfortunately it's not really an Iterator so we can't name it that.

@zeenix
Copy link
Member

zeenix commented Mar 11, 2024

Unfortunately it's not really an Iterator so we can't name it that.

I wasn't suggesting we name it an iterator. That was quoting the existing docs, which justify the try_stream name for consistency with std.

@notgull
Copy link
Member Author

notgull commented Mar 11, 2024

I wasn't suggesting we name it an iterator. That was quoting the existing docs, which justify the try_stream name for consistency with std.

The issue with that is that TryStream is already a taken name in futures, and I think naming the combinator this would cause confusion. Hence the change to Drain.

@fogti
Copy link
Member

fogti commented Mar 12, 2024

how does this compare to futures' TryStream then?

@notgull
Copy link
Member Author

notgull commented Mar 13, 2024

how does this compare to futures' TryStream then?

Mostly unrelated. TryStream is a Stream that returns a Result, while Drain is a Stream combinator that returns as many elements that can be returned as possible without waiting.

@fogti
Copy link
Member

fogti commented Mar 13, 2024

ok, then I side with you that try_stream is unfortunate naming.

@notgull notgull merged commit 16333c5 into master Mar 16, 2024
7 checks passed
@notgull notgull deleted the notgull/name branch March 16, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants