-
Notifications
You must be signed in to change notification settings - Fork 119
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
refactor: Split consumer.rs file #243
Conversation
So much easier to read! <3. |
@CleverAkanoa is this PR ready for review now? |
I think so :) |
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.
Thanks for your effort! Minor comments inline.
Co-authored-by: tison <wander4096@gmail.com>
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.
Thanks for your contribution @CleverAkanoa!
Merging...
Thanks @tisonkun <3 |
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.
Hello! I just noticed some errors when we bumped from 5.0.0 to 5.0.1 and tracked them back to this PR. Both due to using async_std
when we don't have that feature active.
task::{Context, Poll}, | ||
}; | ||
|
||
use async_std::prelude::Stream; |
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 causes a compile error when the async_std
feature is not active.
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.
We can use futures::Stream
which is runtime agnostic. I filed a patch: #250.
time::{Duration, Instant}, | ||
}; | ||
|
||
use async_std::prelude::Stream; |
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.
Same here.
This PR aims to ease the code navigability of the consumer part.
consumer.rs involves more than 2000 lines of codes, it's quite hard to know where you are.
Even all structure are related to consumer, they can be split in more than one files.
No code has been changed, only moved and visibility opened when it was mandatory.