-
Notifications
You must be signed in to change notification settings - Fork 636
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
clean up imports & clippy lints #652
Conversation
I'm sorry, but you might have overlooked #649. But wrt error handling I totally agree. |
Yeah I didn't look at the open PRs, my bad. Well, I think there's actual not too much overlap here so we should be able to merge one then the other and fix conflicts. |
Especially the import sections will be hard to merge, obviously we're fundamentally disagreeing about the best style here :) Nightly rustfmt has options to automatically (un)merge and regroup them, so we could also discuss this later, it will be easy to change. |
As for me, you could go ahead and resolve the conflicts or just reapply your changes, @leshow. Except for rewriting |
Great, thank you! I'll try to get to that later on today. But before I do I wonder if I can quickly try to convince you to merge the imports according to some sort of style.
use crate::diffie_hellman::DHLocalKeys;
use crate::protocol;
use crate::protocol::keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext};
use crate::util; becomes: use crate::{
diffie_hellman::DHLocalKeys,
protocol::{self, keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext}},
util,
};
// any mods would go here
use super::codec::APCodec;
use crate::{
diffie_hellman::DHLocalKeys,
protocol::{self, keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext}},
util,
};
// third party
use byteorder::{BigEndian, ByteOrder, WriteBytesExt};
use hmac::{Hmac, Mac, NewMac};
use protobuf::{self, Message};
use rand::thread_rng;
use sha1::Sha1;
use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
use tokio_util::codec::{Decoder, Framed};
// std
use std::io; Of course this is all very minor things to nitpick on, I just wanted to say my piece and hear your feedback before I re-apply my changes. If you don't want to merge the imports, I would like to still split the std-imports from the third party imports just to make that spot obvious. |
I prefer a "shallow" style where only items of the same module are merged.
The imports should definitely be grouped by crate/third-party/std, but I would use another order, as below. There are two related unstable rustfmt options: What do the maintainers think? |
Until
I personally prefer this style.. but end of the day it's just a code style, so as long as |
Was |
You are right - I had forgotten that I had aliased
We can always pick this up later -- consistency (so with |
Okay, no problem, thanks for your thoughts. |
Okay, I've re-applied some of my changes on top of the tokio_migration branch. This has nothing to do with my current changes but just something I noticed: The way that the cargo features are set up here is not great. Just opening the workspace in VSCode will give you the error "Cannot use two decoders at the same time". If you try and compile like In general I just don't think the way these features are set up is a tenable situation. features are meant to be additive, not exclusive, and on top of that, we should be looking to not have a compile error when used with rust-analyzer. Unless I'm the only one getting this error? Reading some of these crates, it feels like there is a lot of accidental complexity that could be done away with. I think we could make some quick good improvements to this codebase by adding a CI that does clippy lint, rustfmt, etc. |
I've also pushed a change to use |
I'm not satisfied with the features for the decoders as well: Previously it was solved a different way where compilation could not fail (they had sone kind of precedence tremor > vorbis > lewton), but it had the drawback that lewton was always in the dependency tree, so I changed it. I don't get errors in rust-analyzer, maybe you've activated a setting to check with Would it be an option to drop |
Yep, I forgot I have Did lewton really add much to the compile time? Without any of the optional features, it doesn't look too big. Although it looks like it could use an update as well. In any case, why are there are different ogg vorbis backends to begin with? Does one work better than the other? As you said it seems like we should just pick one. However, if do still want all three, then I wouldn't put them behind cargo features like this, it's just not designed for the sort of mutually exclusive selection we're doing. Also, our version of
looking through |
Last time I used The same question should be asked regarding |
If they don't actually work then I don't know why we would keep them. Perhaps we can get some input from the project maintainers on this. |
I think there's a place for I don't think looking at a library's issue list paints a complete enough to picture. Yes, As for |
I haven't taken a look at |
@leshow I think you're confusing DNS and mDNS, so I'm really keen to hear the opinion of the maintainers who probably remember why these alternatives were introduced. |
I'm pretty sure trust-dns has an option to resolve multicast also. Check out: https://github.com/bluejekyll/trust-dns/blob/main/crates/proto/src/multicast/mdns_client_stream.rs#L24 It's got a higher-level wrapper in their client library also behind the |
@willstott101 Are you maybe able to tell us if |
We've sidetracked a bit from what this PR actually does, but I'm glad these convos are happening! Once these things are decided perhaps we can make some issues to split the work up, don't want to get into a situation where we duplicate effort again. |
Many of the optional features require a non-rust library which adds non-cargo dependencies onto the build. If they're not behind features then there'd have to be some other way to choose between them. It's my strong preference for librespot to be pure-rust by default due to the ease of Windows support if that's the case. One of the main issues with maintaining libmdns has been platform support, which is not always trivial to add CI checking for. trust-dns still claims mDNS support is experimental, but if it works and supports all the same (and maybe even more) platforms than libmdns then it's no skin off my teeth. It looks like no one is maintaining the dns-sd bindings, and it doesn't have incredible cross-platform support. But I have no evidence to suggest it doesn't work, nor even any evidence to suggest it doesn't work better. |
I based this off the
tokio_migration
branch since that seems to be the most up to date. When I familiarize myself with a codebase I often go through and clean up little things. I cleaned up:StreamLoaderController
in audio had methods which didn't need to be&mut self
anymoreThere should be no real code changes here, it's all just cleanups.
There's more I'd like to do but this is already big enough for a PR. I'd like to start removing the
unwrap
andexpect
s in the code. The common practice these days is to use a boxed error type for binary, andError
derive (likethiserror
) on library code. Given that, I think we should consideranyhow
for the binary, we can replaceunwrap
/expect
with.context
and use?
.Now that the code is written in async/await format, there looks to be a lot more we could potentially simplify. Anyway, let me know what you think for now! Thanks!