-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(tokio_util): Stabilise JoinMap #7075
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
feat(tokio_util): Stabilise JoinMap #7075
Conversation
eb9f646
to
5155239
Compare
5155239
to
91695b2
Compare
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.
Overall LGTM.
Looks like just merge conflicts at this point? |
Seems I forgot about this. Will check today |
136eb15
to
765fffd
Compare
Would the change in #7252 preclude a non breaking change in this API? Can we merge this without 7252 then merge in 7252 as an optimization later? |
Yes, that's my understanding. The only public API change noticeable is removing the |
I'd like to get #7257 fixed before we stabilize it. |
Awesome, thanks. I'm going to make a release #7283 containing just the bugfix, and then we can merge this and #7252 and make another release. I'm doing this so that people who want the fix can upgrade to 0.7.15 without being affected by the breaking changes contained in this PR (mainly the addition of a feature flag). |
Anything else blocking this merge? |
I was waiting for conrad to reply on #7252. |
f645277
to
0182f8c
Compare
There seems to be a clippy failure. |
CI is passing now |
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.
LGTM, but an additional comment on the feature requirements.
/// [contains]: fn@Self::contains_key | ||
#[cfg_attr(docsrs, doc(cfg(all(feature = "rt", tokio_unstable))))] | ||
pub struct JoinMap<K, V, S = RandomState> { |
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.
Can we add back the doc attribute? Right now it is rendered like this:
Available on crate feature join-map and (crate features rt or join-map) only.
I believe we can improve this with an explicit attribute.
#[cfg_attr(docsrs, doc(cfg(feature = "join-map")))]
tokio-util/Cargo.toml
Outdated
rt = ["tokio/rt", "tokio/sync", "futures-util"] | ||
join-map = ["tokio/rt", "hashbrown"] |
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 we should just have join-map
also enable the rt
feature. After all, the join map does require a runtime.
rt = ["tokio/rt", "tokio/sync", "futures-util"] | |
join-map = ["tokio/rt", "hashbrown"] | |
rt = ["tokio/rt", "tokio/sync", "futures-util"] | |
join-map = ["rt", "hashbrown"] |
This way, we can simplify the task
module to only exist when rt
is enabled instead of the rt or join-map
requirement it has today.
Wohoo! Thanks folks! 🥇 |
Motivation
The other day I saw someone asking for JoinMap to be stabalised. I happened to notice just now that the task ID system is now stable in tokio.
Solution
Removes the unstable config requirement for
tokio_util::JoinMap
.