Skip to content

expose task::ready! #133

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

Merged
merged 4 commits into from
Sep 24, 2019
Merged

expose task::ready! #133

merged 4 commits into from
Sep 24, 2019

Conversation

yoshuawuyts
Copy link
Contributor

@yoshuawuyts yoshuawuyts commented Aug 30, 2019

Export async_std::task::ready!. Thanks!

Screenshot_2019-08-30 async_std task - Rust

@yoshuawuyts yoshuawuyts changed the title expose future::ready! expose task::ready! Aug 30, 2019
Copy link
Contributor

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

I believe you forgot to push the changes.

@ghost
Copy link

ghost commented Sep 2, 2019

Instead of re-exporting the macro from futures, why not reimplement it? :) That way we're less reliant on the futures crate and besides, the implementation is fairly small anyway:

#[macro_export]
macro_rules! ready {
    ($e:expr $(,)?) => {
        match $e {
            ::std::task::Poll::Ready(t) => t,
            ::std::task::Poll::Pending => return ::std::task::Poll::Pending,
        }
    };
}

As for the placement of the macro, I wonder if we should just put it in the root module? That's what the standard library does with its macros - they're all in the root module and re-exported from the prelude.

I believe that's for historical reasons because at Rust 1.0 it was not possible to place macros in arbitrary modules and they had to be in the crate root (they weren't even importable!), but this convention has kind of stuck and now all new macros still get added there.

We already have one such macro placed into root, async_std::task_local, but I believe uses will never import it manually and will instead get it from the prelude.

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Sep 2, 2019

@stjepang that's exactly what I did in the first pass at this, and the only way I got it to work was by re-exporting it from an external module.

I mean: we should probably create async-macros anyway, but I figured for now I'd just go with the easiest path which is to re-export the futures macro.

@ghost
Copy link

ghost commented Sep 3, 2019

If we're going to add the ready macro, I think I'd be most comfortable with placing it into async_std rather than async_std::task and then also re-exporting it into async_std::prelude.

By the way, another benefit of not re-exporting stuff from the futures crate is that we can write better docs for it. The macro currently has very basic docs and we could write a nice doc example there.

@yoshuawuyts
Copy link
Contributor Author

@stjepang we probably shouldn't name it ready then. How about poll_ready?

There's recent precedent for exposing submodule macros in std now too I believe; Alex recently had something on this. I'll ping him about it.

@yoshuawuyts
Copy link
Contributor Author

@stjepang another thought: what are we going to do about {join!,try_join!,select!,try_select!}? I'd argue they shouldn't be top-level constructs either, but probably live under future::*. If that is the case, then having task::ready! would be similar.

The other option for them is to change their naming to be future_try_join or something, but that feels worse than future::try_join. I think if we angle it from there it makes more sense to expose these macros from their respective submodules.

Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
@yoshuawuyts
Copy link
Contributor Author

Now that #187 and #208 are out, this patch has also been migrated to use async-macros (with docs we can manage to our liking).

@stjepang how do you feel about the way macros are laid out now?

Signed-off-by: Yoshua Wuyts <[email protected]>
@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Sep 18, 2019
@yoshuawuyts
Copy link
Contributor Author

We now have stream::join as module-level macro and marked as unstable. I think it should be okay to merge this to let people try this out. Thanks!

@yoshuawuyts yoshuawuyts merged commit 553e480 into async-rs:master Sep 24, 2019
@yoshuawuyts yoshuawuyts deleted the ready_macro branch September 24, 2019 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants