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

re-export async-attributes #238

Merged
5 commits merged into from
Nov 7, 2019
Merged

re-export async-attributes #238

5 commits merged into from
Nov 7, 2019

Conversation

yoshuawuyts
Copy link
Contributor

Closes #237. The docs don't render properly on this, but I think that's okay. Thanks!

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

yoshuawuyts commented Nov 2, 2019

This PR and async-rs/async-attributes#8 have been updated to now use the attributes feature as suggested in #438.

I think merging this would actually be really nice, as we enable people to write some nicer-looking applications the way they could with Runtime, but we don't lose any performance in the base case. Coupled with the runtime feature I think async-std will be a really well rounded option for almost every use!

Screenshots

Screenshot_2019-11-03 async_std - Rust

@ghost
Copy link

ghost commented Nov 2, 2019

Are these now stable attributes? I assume so and fine with that, but just making sure it's not an oversight. :)

There's not much we can do wrong with these attributes - they're pretty straightforward, right?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approving, but let's wait for @skade to also take a look.

@ghost ghost requested a review from skade November 2, 2019 23:37
@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Nov 2, 2019

There's not much we can do wrong with these attributes - they're pretty straightforward, right?

Not necessarily, and I think we should be cautious. async-rs/async-attributes#11 and async-rs/async-attributes#9 actually should be resolved first.

edit: I don't think we should be exposing that particular API. So another option is we punt #[bench] until we figure it out and only expose #[test] and #[main]. Those two could be insta-stable for sure.

@yoshuawuyts
Copy link
Contributor Author

Updated the PR to be exactly that. Thanks!

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

ghost commented Nov 2, 2019

Agree with everything you said 👍

@ghost
Copy link

ghost commented Nov 6, 2019

ping for @skade

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Nov 7, 2019

@stjepang I would like to make this available in the next release; I'd like to propose we merge this.

@ghost ghost merged commit 84880c4 into master Nov 7, 2019
@ghost ghost deleted the attributes branch November 7, 2019 22:10
This pull request was closed.
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.

Expose async-attributes macros behind feature flag
1 participant