-
Notifications
You must be signed in to change notification settings - Fork 171
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
RUST-43 Add change streams examples for documentation #572
RUST-43 Add change streams examples for documentation #572
Conversation
{ | ||
// Start Changestream Example 1 | ||
let mut stream = inventory.watch(None, None).await?; | ||
let next = stream.next().await.transpose()?; |
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.
Rather than using next()
and transpose
, we can also import TryStreamExt
and then just try_next()
. I think that's what we have in our existing examples.
On a related note, I think we might want to include the (Try)StreamExt
import in the examples, since users will probably just copy/paste them and figuring out they need to import that trait could be difficult. That said, they'll also need to figure out they need a futures
dependency, so maybe it doesn't really matter.
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.
Updated to use TryStreamExt
and include that in the examples. I don't think the futures
dependency needs to be worried about; anyone more than passingly familiar with Rust will know that use foo::bar;
means you need to pull in crate foo
.
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.
Looks like the commit(s) for this still need to be pushed 🙂
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.
Derp, done.
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.
Not necessarily something we need to address completely in this PR, but the documentation_examples.rs
file is getting pretty long. When we introduced the transactions examples we did so in the top-level tests
directory rather than src/test
and I think it would be nice to continue doing that with documentation examples going forward (incl. here). This helps to ensure that we're not accidentally using anything in the doc examples that isn't part of the public API.
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.
Yeah that's a good point and seems like something we should do. Maybe that would make a good quick-wins ticket? I think it would probably be best to move them all at once so the docs team only needs to look for them in one place.
{ | ||
// Start Changestream Example 1 | ||
let mut stream = inventory.watch(None, None).await?; | ||
let next = stream.next().await.transpose()?; |
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.
Looks like the commit(s) for this still need to be pushed 🙂
Filed RUST-1180 |
3f25d1a
to
79257c7
Compare
No description provided.