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

Chore: remove excess dependencies #4

Merged
merged 4 commits into from
Aug 24, 2021
Merged

Chore: remove excess dependencies #4

merged 4 commits into from
Aug 24, 2021

Conversation

BastiDood
Copy link
Contributor

This PR aims to reduce the number of dependencies for both development and production environments.

Production Dependencies

Before, on my machine, building the library requires pulling in 49 crates. This is not so bad, but when I looked into the project manifest, I realized that a huge bulk of the bloat comes from the futures crate.

Hence, the first two commits of this PR addresses this issue by replacing the futures crate for the much more lightweight futures-core crate in production environments. The futures-core crate provides the most minimal interface for the Stream trait.

Overall, this reduces the number of built crates from 49 to 22. That's a significant improvement! This was mostly due to the fact that the futures crate packages an executor by default (among other unused imports), whereas the futures-core crate only exposes core traits. Since bmrng only used the Stream interface, there is no need for the executor (and other imports).

Development Dependencies

Meanwhile, following the removal of the futures crate in all environments, the futures-util crate (which depends on futures_core) is used to regain access to the StreamExt utilities for tests.

Finally, the development dependency on tokio-test has also been removed. The tests only used tokio-test for the assert_ok and assert_err macros, which may easily be emulated by the built-in assert macro with the Result::is_{ok|err} methods.

While I was there in the tests, I also refactored multiple instances of redundant Boolean assertions. That is:

#[test]
fn it_works() {
    // ...
    // This is too verbose.
    assert_eq!(tx.is_closed(), false);

    // This does the same thing,
    // but is definitely more concise
    // and less redundant.
    assert!(!tx.is_closed());
}

If there are any issues with this PR, please do let me know. I will gladly resolve them! Thanks!

Copy link
Owner

@oguzbilgener oguzbilgener left a comment

Choose a reason for hiding this comment

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

Hi there, thank you for the PR! This is certainly a significant improvement in build times!
I have no problem with removing tokio-test either.

Comment on lines -54 to -60
// Cannot test this due to private field in the tokio error implementation
#[cfg(not(tarpaulin_include))]
impl<T> From<MpscRecvError> for RequestError<T> {
fn from(_err: MpscRecvError) -> RequestError<T> {
RequestError::RecvError
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this was unused 👍

@oguzbilgener oguzbilgener merged commit c413778 into oguzbilgener:master Aug 24, 2021
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