cleanup(gax): rename paginator to stream#1611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1611 +/- ##
=======================================
Coverage 95.72% 95.72%
=======================================
Files 43 43
Lines 1823 1823
=======================================
Hits 1745 1745
Misses 78 78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// | ||
| /// This API is gated by the `unstable-stream` feature. | ||
| pub fn to_stream(self) -> impl futures::Stream<Item = Result<T, E>> + Unpin { | ||
| pub fn into_stream(self) -> impl futures::Stream<Item = Result<T, E>> + Unpin { |
There was a problem hiding this comment.
Does the same argument apply to:
google-cloud-rust/src/lro/src/lib.rs
Line 280 in 457d0ff
??
If not: what happens if we ever change the implementation of Paginator? Do we need to rename this function?
There was a problem hiding this comment.
If not: what happens if we ever change the implementation of
Paginator? Do we need to rename this function?
Good point.
Changing the implementation from being cheap to being expensive (for example) would be an unwanted behavior change. The name into_* enforces this (albeit very softly).
Does the same argument apply to
lro::Poller::to_stream?
Mostly. My plan was to rename that in a separate PR.
It is weirder in that it is a trait, which could have any implementation1. I think the name into_* suggests things we want about the implementation though.
Footnotes
-
I need to consider https://github.com/googleapis/google-cloud-rust/issues/1609#issuecomment-2751674686 to see if we can seal the trait, and thus be confident that it has only one implementation, and that
into_*is appropriate. ↩
There was a problem hiding this comment.
I am worried about the naming changing back and forth. It may be better to give this a non-standard name so people do not have expectations about performance, even if it is sealed.
With that said, I had forgotten you may be waiting for an answer. Approved so you can make progress, but I am going to open a tracking bug.
There was a problem hiding this comment.
I am not blocked on this PR going through. It does not have to go through.
I am going to open a tracking bug.
I think we should make a decision now and probably never look back.
My vote is for into_stream. I think that is the best name at this time.
There was a problem hiding this comment.
My vote is for
into_stream. I think that is the best name at this time.
Since it also applies to Poller, I think I buy that.
2 participants
Noticed in #1541
https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
My read on this is that we should use
into_because this is a cheap operation that consumes the underlying type. It is at a similar level of abstraction, but I think the "cheap" part is whyinto_fits better thanto_.This is not a break, because the renamed APIs (added in #1466) have not been released yet.