-
Notifications
You must be signed in to change notification settings - Fork 8
Support stable async-std #7
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
Conversation
Hey @flub, thanks for opening this! -- I think we can safely drop the pre-1.9 API and only add support for the new stable API. Once we have that we can release Could you update this PR to reflect that? -- thanks! |
Since async-std 1.8 the API has moved and is considered stable. Since async-std 1.9 the old API has been removed. This adapts stop-token to this new API and drops support for async-std <1.8.
Seems very reasonable, I updated the PR and squashed it back into just a single commit since the history didn't add much value. |
Now that the async-std dependency is newer it makes sense to match the pin-project-lite version of it, so that a duplicate copy is does not need to be compiled in.
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.
This looks great; thanks heaps!
thanks for pushing a new release! |
@@ -1,36 +1,40 @@ | |||
use std::time::Duration; | |||
|
|||
use async_std::{prelude::*, task, sync::channel}; | |||
use async_std::prelude::*; |
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.
@flub I was wondering why the change in use
syntax here? Is it not better to use {}
syntax to make all these imports on 1 line?
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.
mixing multiple levels of of imports quickly becomes confusing to me, that is i avoid having to use ::
inside { }
and nesting braces for imports. fewer lines isn't always more readable. but this doesn't matter much does it?
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.
No, just a style question and I was simply curious.
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.
the downside to this however is that it makes the diff harder to read
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.
if you stick to that style all following diffs are more readable. If you don't every single non-trivial diff becomes harder to read. this is all bikeshedding of course and depends on personal taste :) i wish cargo-fmt had much stricter formatting rules around imports instead of allowing so many variations, it would avoid this bikeshedding.
[features] | ||
unstable = ["async-std/unstable"] | ||
pin-project-lite = "0.2.0" | ||
async-std = "1.8" |
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.
I'm curious about this change. If I understand cargo dependency resolution, async-std = "1.0"
is short for async-std="^1.0"
, which in this case would resolve to async-std="1.9"
since: ^1 := >=1.0.0, <2.0.0
. So I think this version bump is superfluous so semver will evaluate to the latest 1.x
release, namely 1.9
.
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.
1.8 is the earliest version this can support. setting it like this is equivalent to ^1.8
or >=1.8.0 <2.0.0
. As this is a library that means users can themselves say for example async-std = "^1.18"
in their own cargo.toml and this library will not force another duplicate version of async-std to be compiled in as that version is still compatible here.
If we would set this to ^1
then someone depending on this could say = 1.3.0
and we'd get that version here and it would break.
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.
I think i'm missing something about dependency resolution here. When I set async-std="1.0"
cargo downloads 1.9
. I guess I don't understand how to tell cargo to use less than 1.9
.
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.
your binary says async-std >= 1.0.0, < 2.0.0
and you also depend on stop-token which says async-std >= 1.8.0, < 2.0.0
. As that's the only input to cargo's dependency resolver it decides it also prefers the most recent version and it resolves this to async-std 1.9.0
which satisfies all those constraints.
If you want cargo to use something older than 1.9 you need to tell it to do that in your Cargo.toml. Set async-std: <1.9
probably with some other constraints added. If you are asking for async-std < 1.8
but do ask for stop-token >= 0.2
then cargo will build 2 versions of async-std into your binary.
Update async-std and stop-token dependencies, migrate to stable channels
Since async-std 1.8 the API has moved and is considered stable. Since
async-std 1.9 the old API has been removed. This allows both versions
to still work, depending on whether this crate is built with the
"unstable" feature or not.
Unfortunately I'm not sure how to run the test for both cases. There might be some cargo gymnastics that I'm not aware off but to test the unstable feature you need to limit async-std to < 1.9 somehow, but not do that otherwise.