Skip to content

Migrate FutureExt examples to use await! #1088

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jul 18, 2018

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 14, 2018

No description provided.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 14, 2018

Blocked on rust-lang/rust#52357 to be able to actually use async blocks in doc-tests.

EDIT: I didn't think it was worth it to migrate to a more complicated boilerplate using top-level async fn then update it to use async blocks again once rustdoc is fixed, hopefully the fix should be pretty small and this will work in a not-too-distant nightly.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 17, 2018

Fix should be in the next nightly, will test and update tomorrow once that's out.

@aturon aturon requested a review from MajorBreakfast July 17, 2018 17:26
@MajorBreakfast
Copy link
Contributor

I'll sign off as soon as soon as I see it building with the new nightly. Looks solid.

One notable change I see is that you removed the prelude imports and used direct imports instead. I think that that's a great idea because it gives the reader a better understanding about what types and traits are involved. We should apply this style to all examples (not in this PR).

/// #![feature(async_await, await_macro, futures_api)]
/// # futures::executor::block_on(async {
/// use futures::future::{self, FutureExt};
/// use futures::executor::{spawn_with_handle, ThreadPool};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not alphabetically ordered

/// // Note, unlike most examples this is written in the context of a
/// // synchronous function to better illustrate the cross-thread aspect of
/// // the `shared` combinator.
///
/// use std::thread;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want separate std imports? During my refactorings for 0.3 I made it so that all imports are simply always bundled together alphabetically ordered and std is treated like any other crate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I don't really mind either way.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 18, 2018

Updated. There's now just one example that's failing to compile, I've ignored it for now and will take a look at finding/opening a rust-lang/rust ticket to link to this evening.

@@ -214,18 +212,16 @@ pub trait FutureExt: Future {
/// # Example
Copy link
Contributor

@MajorBreakfast MajorBreakfast Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Singular: Usually we have plural there. The docs for the standard library also always use the plural here: "Examples"

Edit: There are some more below.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 18, 2018

Issue is probably rust-lang/rust#49537, something to do with calling a function taking a closure inside a generator.

@MajorBreakfast MajorBreakfast merged commit 3beee84 into rust-lang:0.3 Jul 18, 2018
@Nemo157 Nemo157 deleted the async-examples branch February 16, 2019 16:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants