refactor: general small refactors to simplify code #1382
Conversation
|
These are some things I encountered while adding documentation in #1381. The only one that I'm not sure about is the removal of |
ThomasFrans
force-pushed
the
refactor-small-bits
branch
from
595b895 to
a235602
Compare
- Remove `expect` in favor of `unwrap` when the `Result`'s error variant contains the info in the `expect` anyway (eg. when locking things). The line number/context are given by the backtrace. - Remove over-specification of types (`&T` instead of `&RWReadLockGuard`) - Put reused values into constants - `FromStr` instead of manual function - Change `if let Some(()) = ...` to `if T.is_some()`
ThomasFrans
force-pushed
the
refactor-small-bits
branch
from
a235602 to
c05561e
Compare
|
Before solving the merge conflicts, I quickly changed the |
|
Thanks! Back then I thought the tiny extra context that |
|
There is the stack trace to point out the location and there is the general message of the error that says what went wrong. I think one of the next Rust releases is going to remove a bunch of debug information by default by stripping every binary, whether it's in the profile or not. You could always disable the stripping again if that happens. I quickly checked what another project like Alacritty does for locking and sending to channels and it seems like they also use If you'd like to keep the context of the |
| let user = ASYNC_RUNTIME.get().unwrap().block_on(user_rx).ok(); | ||
| let volume = cfg.state().volume; | ||
| spotify.set_volume(volume); | ||
|
|
||
| spotify.api.set_worker_channel(spotify.channel.clone()); | ||
| spotify.api.update_token(); | ||
|
|
||
| spotify.api.set_user(spotify.user.clone()); | ||
| spotify.api.set_user(user); |
There was a problem hiding this comment.
Probably not a problem, but this will set the user later than before. A quick check didn't give me anything, so I think this is fine.
There was a problem hiding this comment.
I don't fully understand. I thought nothing would change because Spotify::user was only used in the Spotify::new() method?
There was a problem hiding this comment.
Huh, yeah, I think I may have misread that. Sorry for the noise!
There was a problem hiding this comment.
No I understand. I grepped the project so I knew it wasn't used anywhere else. I can imagine you don't know every line off the top of your head 😄
(PS In the next days I'll probably try to solve/continue work on some of the bugs that have shown up in the last couple of months, if that's ok 🙂)
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn parses_album_uri() { | ||
| let uri_type = "spotify:album:29F5MF6Q9VYlryDsYEQz6a".parse(); | ||
| assert!(matches!(uri_type, Ok(UriType::Album))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn parse_invalid_uri() { | ||
| let uri_type: Result<UriType, _> = "kayava".parse(); | ||
| assert!(matches!(uri_type, Err(UriParseError))); | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
| fn parse_playlist_uri() { | ||
| let uri_type = "spotify:playlist:37i9dQZF1DX36Xw4IJIVKA".parse(); | ||
| assert!(matches!(uri_type, Ok(UriType::Playlist))); | ||
| } | ||
| } |
Some small general refactoring to improve code readability and adhere to Rust conventions.
Describe your changes
expectin favor ofunwrapwhen theResult's error variant contains the info in theexpectanyway (eg. when locking things). The line number/context are given by the backtrace.&Tinstead of&RWReadLockGuard)try_frominstead of manual functionif let Some(()) = ...toif T.is_some()Checklist before requesting a review
not performance improvements, etc.)