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

update to tokio 1.0 #9

Merged
merged 3 commits into from
Jan 6, 2021
Merged

update to tokio 1.0 #9

merged 3 commits into from
Jan 6, 2021

Conversation

astraw
Copy link
Contributor

@astraw astraw commented Dec 29, 2020

This updates the internal use of tokio to 1.0. As I note in the module-level docstring, the examples (and tests) are kept using tokio 0.3 because tokio itself has removed the Stream trait until it is added in std.

I don't think this is a breaking change because tokio is not part of the API. Thus, I believe this could be released as non-breaking semver version bump.

@codecov-io
Copy link

codecov-io commented Dec 29, 2020

Codecov Report

Merging #9 (63decfb) into master (8292fac) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #9   +/-   ##
=======================================
  Coverage   81.21%   81.21%           
=======================================
  Files           3        3           
  Lines         165      165           
=======================================
  Hits          134      134           
  Misses         31       31           
Impacted Files Coverage Δ
src/combinator.rs 75.00% <ø> (ø)
src/lib.rs 81.81% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8292fac...63decfb. Read the comment docs.

@jonhoo
Copy link
Owner

jonhoo commented Dec 30, 2020

Hmm, I suppose since we're just using sync types, we probably work with any version of tokio, you're right. That's nice! Note that that wouldn't have been the case if we used tokio I/O types or spawning for example, even though no tokio types show up in the external API.

As for the tests, I'd prefer to keep them on the same version nonetheless. Can we leverage tokio-stream here?

@astraw
Copy link
Contributor Author

astraw commented Dec 31, 2020

Good point that although tokio does not show up in the external API, compatibility with older tokio versions is not necessarily guaranteed. I hadn't considered that and thus perhaps this should be investigated more closely before declaring that this does not require a semver bump. (My back-compat testing was limited to the tests in the crate.)

(For reference, my motivation for this PR was simply eliminating tokio 0.3 from my dependency tree and replacing it exclusively with tokio 1.0.)

Regarding keeping the tests using the same version of tokio as the implementation and the Stream trait issue: I agree it is a nice goal and I originally tried to do that. However, I realized that it will substantially complicate the tests to port them to tokio 1.0 while tokio 1.0 is missing streams. As far as I remember (I forget the details already), using tokio-stream will still require a lot of boilerplate for what should be a simple example. In the end I see two possibilities that seem OK: The first is to keep the examples using tokio 0.3 until the Stream definition is in std at which point tokio will also implement this. Unfortunately this will take some months. The second possibility is to demonstrate and test stream-cancel not on tokio streams but e.g. simple self-defined ones.

@jonhoo
Copy link
Owner

jonhoo commented Jan 1, 2021

@astraw I think that should be helped by tokio-rs/tokio#3343, which adds back all the various Stream implementations for the tokio types into tokio-stream?

@astraw
Copy link
Contributor Author

astraw commented Jan 1, 2021

Yes, those are the boilerplate implementations that I thought didn't belong in the stream-cancel examples. I think the stream-cancel examples/tests could be updated once those tokio-stream changes are published.

Until then, in this pull request, I would be happy to rename tokio 0.3 to tokio03 and tokio 1.0 to just tokio. Then tokio03, which would exist only in the examples, would be completely replaced with tokio-stream when that was possible. (The current naming was chosen to change as little code as possible.) Since replacing tokio03 to tokio-stream would touch only the examples, I'm not sure it would merit a new release. Still, such a later change would be useful to get the updated examples into the docstrings.

How do you like that idea?

(Once Stream lands in std, a new version of stream-cancel with a major version bump can be released that depends on the std Stream definition. Presumably the examples could then once again use types directly from tokio, not tokio-stream.)

@astraw
Copy link
Contributor Author

astraw commented Jan 1, 2021

Actually my proposal resulted in fewer changes than I anticipated, so I went ahead and updated this pull request.

@jonhoo
Copy link
Owner

jonhoo commented Jan 5, 2021

I think the tokio-stream changes have been published now :)

@astraw
Copy link
Contributor Author

astraw commented Jan 5, 2021

Yes, now I updated the tests and examples to use the tokio-stream wrappers introduced in 0.1.1.

@jonhoo jonhoo merged commit 9676c82 into jonhoo:master Jan 6, 2021
@jonhoo
Copy link
Owner

jonhoo commented Jan 6, 2021

Beautiful, thank you! Will release shortly.

@jonhoo
Copy link
Owner

jonhoo commented Jan 6, 2021

Released as 0.8.0 🎉

jonhoo added a commit that referenced this pull request Sep 9, 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.

None yet

3 participants