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

add default wrapper implementations #1

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

akiradeveloper
Copy link
Contributor

This PR adds default wrapper implementations for Future and Stream.

This crate is typically used to add Sync to these two traits so it is good to have default implementations.

Also, as this is a library crate let's untrack Cargo.lock file.

Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

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

I’m very sorry for having missed this PR!

In general it makes sense to me, would be a good addition. The Cargo.lock file so far was completely inconsequential, but when adding dependencies I agree that it should be removed from git.

Regarding those dependencies: now that SyncWrapper is no_std, this PR would need to add a std feature that forwards to futures-core/std.

src/lib.rs Outdated
@@ -19,6 +19,8 @@
#![doc(html_logo_url = "https://developer.actyx.com/img/logo.svg")]
#![doc(html_favicon_url = "https://developer.actyx.com/img/favicon.ico")]

pub mod ext;
Copy link
Member

Choose a reason for hiding this comment

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

I’d probably put everything into lib.rs — the library is still pretty small.

src/ext.rs Outdated
use std::future::Future;
use std::task::{Context, Poll};
use std::pin::Pin;

Copy link
Member

Choose a reason for hiding this comment

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

Could you add doc comments with examples?

src/ext.rs Outdated
impl <F> SyncFuture<F> {
pub fn new(fut: F) -> Self {
Self { fut: SyncWrapper::new(fut) }
}
Copy link
Member

Choose a reason for hiding this comment

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

perhaps add into_inner as well

Cargo.toml Outdated
futures = { version = "0.3" }

[dependencies]
futures-core = { version = "0.3" }
Copy link
Member

Choose a reason for hiding this comment

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

how about

Suggested change
futures-core = { version = "0.3" }
futures-core = { version = "0.3", default-features = false }

to keep this crate no_std?

src/lib.rs Outdated
Self { inner: SyncWrapper::new(inner) }
}
pub fn into_inner(self) -> SyncWrapper<F> {
self.inner
Copy link
Member

Choose a reason for hiding this comment

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

Since SyncWrapper is an implementation detail of SyncFuture, I’d want to return plain F from this function.

src/lib.rs Outdated
///
/// let fut = async { 1 };
/// let fut = SyncFuture::new(fut);
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

I usually put these docs on the struct itself, which has the added benefit of showing the first line in the summary of the docs.

Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@rkuhn rkuhn merged commit 778cffe into Actyx:master Aug 16, 2021
@akiradeveloper akiradeveloper deleted the default-impl branch August 16, 2021 09:15
@Nugine Nugine mentioned this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants