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

impl Stream and remove the "eager retry" logic #1

Merged
merged 12 commits into from Jan 14, 2023
Merged

Conversation

loyd
Copy link
Contributor

@loyd loyd commented Sep 1, 2022

  • Added a Stream instance.
  • Replaced deprecated lints and methods.
  • Removed repolling on Pending. It's a controversial change, but I don't understand why the lib contains that logic. Firstly, it's a pretty rare situation, but it adds extra CAS on every Pending. Secondly, it doesn't seem to be the library's responsibility (and It breaks our tests (WS pool with tricky logic)).

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

These are good changes, thank you! Left some notes inline.

Also, would you mind rebasing against master to pick up the CI changes I just made?

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@loyd loyd changed the title impl Stream impl Stream and remove the "eager retry" logic Sep 6, 2022
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #1 (6f05015) into main (4bf517f) will increase coverage by 7.8%.
The diff coverage is 94.1%.

Additional details and impacted files
Impacted Files Coverage Δ
src/lib.rs 94.2% <94.1%> (+7.8%) ⬆️

src/lib.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Sep 17, 2022

Looks like two CI failures. One is because the rustdoc lint hadn't yet been renamed in 1.51, which we still support, so its build fails. The other is because, seemingly, something in our dependency graph isn't truthfully representing their minimum required dependency versions. You may have to explicitly list async-stream-impl in our Cargo.toml with a higher version requirement (and perhaps send a PR to tokio-stream to bump its listed dependency version?).

@loyd
Copy link
Contributor Author

loyd commented Jan 4, 2023

@jonhoo, could you run lints? I want to fix them finally =)

@loyd
Copy link
Contributor Author

loyd commented Jan 5, 2023

tokio-rs/tokio#5347

Cargo.toml Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Jan 7, 2023

Happy to merge this once we remove that now-unneeded line from Cargo.toml 🎉
I'll do a release shortly after.

@loyd
Copy link
Contributor Author

loyd commented Jan 8, 2023

So, are you waiting for the new release of tokio-test? The latest release was in May, this is not a quick process, sadly. I think specifying async-stream is good enough to make a release. Maybe the comment should be remove once tokio-test v0.4.3 is released =)

@jonhoo
Copy link
Owner

jonhoo commented Jan 8, 2023

Ah, I thought they did a release after merging the PR. In that case just updating the comment is fine 👍

@jonhoo jonhoo merged commit d8fedad into jonhoo:main Jan 14, 2023
@jonhoo
Copy link
Owner

jonhoo commented Jan 14, 2023

Released in 0.2.3 🎉 Thanks for sticking with it :)

jonhoo added a commit that referenced this pull request Jan 14, 2023
…odecov-action-3

Bump codecov/codecov-action from 2 to 3
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

2 participants