From 88d1dc2b01a3d6b3cbfa85239f9489f232da624a Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 3 Mar 2024 23:21:44 +0100 Subject: [PATCH] Streamline some code --- src/core/collection/album.rs | 32 ++++ src/core/collection/artist.rs | 67 +++++++-- src/core/database/serde/deserialize.rs | 4 +- src/core/musichoard/musichoard.rs | 194 ++++++++++++++++--------- src/core/testmod.rs | 2 +- src/tests.rs | 24 ++- src/tui/testmod.rs | 3 +- tests/testlib.rs | 14 +- 8 files changed, 227 insertions(+), 113 deletions(-) diff --git a/src/core/collection/album.rs b/src/core/collection/album.rs index d0222d4..b750c4c 100644 --- a/src/core/collection/album.rs +++ b/src/core/collection/album.rs @@ -118,6 +118,12 @@ impl Merge for Album { } } +impl> From for AlbumId { + fn from(value: S) -> Self { + AlbumId::new(value) + } +} + impl AsRef for AlbumId { fn as_ref(&self) -> &AlbumId { self @@ -193,6 +199,32 @@ mod tests { assert!(album_1 < album_2); } + #[test] + fn set_clear_seq() { + let mut album = Album { + id: "an album".into(), + date: AlbumDate::default(), + seq: AlbumSeq::default(), + tracks: vec![], + }; + + assert_eq!(album.seq, AlbumSeq(0)); + + // Setting a seq on an album. + album.set_seq(AlbumSeq(6)); + assert_eq!(album.seq, AlbumSeq(6)); + + album.set_seq(AlbumSeq(6)); + assert_eq!(album.seq, AlbumSeq(6)); + + album.set_seq(AlbumSeq(8)); + assert_eq!(album.seq, AlbumSeq(8)); + + // Clearing seq. + album.clear_seq(); + assert_eq!(album.seq, AlbumSeq(0)); + } + #[test] fn merge_album_no_overlap() { let left = FULL_COLLECTION[0].albums[0].to_owned(); diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index ab9d8b0..372dc20 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -2,6 +2,7 @@ use std::{ collections::HashMap, fmt::{self, Debug, Display}, mem, + str::FromStr, }; use url::Url; @@ -139,6 +140,12 @@ impl Merge for Artist { } } +impl> From for ArtistId { + fn from(value: S) -> Self { + ArtistId::new(value) + } +} + impl AsRef for ArtistId { fn as_ref(&self) -> &ArtistId { self @@ -169,9 +176,13 @@ pub struct MusicBrainz(Url); impl MusicBrainz { /// Validate and wrap a MusicBrainz URL. - pub fn new>(url: S) -> Result { + pub fn new_from_str>(url: S) -> Result { let url = Url::parse(url.as_ref())?; + Self::new_from_url(url) + } + /// Validate and wrap a MusicBrainz URL. + pub fn new_from_url(url: Url) -> Result { if !url .domain() .map(|u| u.ends_with("musicbrainz.org")) @@ -199,11 +210,36 @@ impl AsRef for MusicBrainz { } } -impl TryFrom<&str> for MusicBrainz { +impl FromStr for MusicBrainz { + type Err = Error; + + fn from_str(s: &str) -> Result { + MusicBrainz::new_from_str(s) + } +} + +// A blanket TryFrom would be better, but https://stackoverflow.com/a/64407892 +macro_rules! impl_try_from_for_musicbrainz { + ($from:ty) => { + impl TryFrom<$from> for MusicBrainz { + type Error = Error; + + fn try_from(value: $from) -> Result { + MusicBrainz::new_from_str(value) + } + } + }; +} + +impl_try_from_for_musicbrainz!(&str); +impl_try_from_for_musicbrainz!(&String); +impl_try_from_for_musicbrainz!(String); + +impl TryFrom for MusicBrainz { type Error = Error; - fn try_from(value: &str) -> Result { - MusicBrainz::new(value) + fn try_from(value: Url) -> Result { + MusicBrainz::new_from_url(value) } } @@ -230,34 +266,35 @@ mod tests { #[test] fn musicbrainz() { let uuid = "d368baa8-21ca-4759-9731-0b2753071ad8"; - let url = format!("https://musicbrainz.org/artist/{uuid}"); - let mb = MusicBrainz::new(&url).unwrap(); - assert_eq!(url, mb.as_ref()); + let url_str = format!("https://musicbrainz.org/artist/{uuid}"); + let url: Url = url_str.as_str().try_into().unwrap(); + let mb: MusicBrainz = url.try_into().unwrap(); + assert_eq!(url_str, mb.as_ref()); assert_eq!(uuid, mb.mbid()); let url = "not a url at all".to_string(); let expected_error: Error = url::ParseError::RelativeUrlWithoutBase.into(); - let actual_error = MusicBrainz::new(url).unwrap_err(); + let actual_error = MusicBrainz::from_str(&url).unwrap_err(); assert_eq!(actual_error, expected_error); assert_eq!(actual_error.to_string(), expected_error.to_string()); let url = "https://musicbrainz.org/artist/i-am-not-a-uuid".to_string(); let expected_error: Error = Uuid::try_parse("i-am-not-a-uuid").unwrap_err().into(); - let actual_error = MusicBrainz::new(url).unwrap_err(); + let actual_error = MusicBrainz::from_str(&url).unwrap_err(); assert_eq!(actual_error, expected_error); assert_eq!(actual_error.to_string(), expected_error.to_string()); let url = "https://musicbrainz.org/artist".to_string(); let expected_error = Error::UrlError(format!("invalid MusicBrainz URL: {url}")); - let actual_error = MusicBrainz::new(&url).unwrap_err(); + let actual_error = MusicBrainz::from_str(&url).unwrap_err(); assert_eq!(actual_error, expected_error); assert_eq!(actual_error.to_string(), expected_error.to_string()); } #[test] fn urls() { - assert!(MusicBrainz::new(MUSICBRAINZ).is_ok()); - assert!(MusicBrainz::new(MUSICBUTLER).is_err()); + assert!(MusicBrainz::from_str(MUSICBRAINZ).is_ok()); + assert!(MusicBrainz::from_str(MUSICBUTLER).is_err()); } #[test] @@ -266,7 +303,7 @@ mod tests { let sort_id_1 = ArtistId::new("sort id 1"); let sort_id_2 = ArtistId::new("sort id 2"); - let mut artist = Artist::new(artist_id.clone()); + let mut artist = Artist::new(&artist_id.name); assert_eq!(artist.id, artist_id); assert_eq!(artist.sort, None); @@ -317,14 +354,14 @@ mod tests { // Setting a URL on an artist. artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap()); - _ = expected.insert(MusicBrainz::new(MUSICBRAINZ).unwrap()); + _ = expected.insert(MUSICBRAINZ.try_into().unwrap()); assert_eq!(artist.musicbrainz, expected); artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap()); assert_eq!(artist.musicbrainz, expected); artist.set_musicbrainz_url(MUSICBRAINZ_2.try_into().unwrap()); - _ = expected.insert(MusicBrainz::new(MUSICBRAINZ_2).unwrap()); + _ = expected.insert(MUSICBRAINZ_2.try_into().unwrap()); assert_eq!(artist.musicbrainz, expected); // Clearing URLs. diff --git a/src/core/database/serde/deserialize.rs b/src/core/database/serde/deserialize.rs index 8b403ec..cc4a64a 100644 --- a/src/core/database/serde/deserialize.rs +++ b/src/core/database/serde/deserialize.rs @@ -5,7 +5,7 @@ use serde::Deserialize; use crate::core::{ collection::{ album::{Album, AlbumDate, AlbumId, AlbumSeq}, - artist::{Artist, ArtistId, MusicBrainz}, + artist::{Artist, ArtistId}, Collection, }, database::LoadError, @@ -51,7 +51,7 @@ impl TryFrom for Artist { Ok(Artist { id: ArtistId::new(artist.name), sort: artist.sort.map(ArtistId::new), - musicbrainz: artist.musicbrainz.map(MusicBrainz::new).transpose()?, + musicbrainz: artist.musicbrainz.map(TryInto::try_into).transpose()?, properties: artist.properties, albums: artist.albums.into_iter().map(Into::into).collect(), }) diff --git a/src/core/musichoard/musichoard.rs b/src/core/musichoard/musichoard.rs index b82c8ea..cad9ec3 100644 --- a/src/core/musichoard/musichoard.rs +++ b/src/core/musichoard/musichoard.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use crate::core::{ collection::{ album::{Album, AlbumDate, AlbumId, AlbumSeq}, - artist::{Artist, ArtistId}, + artist::{Artist, ArtistId, MusicBrainz}, track::{Track, TrackId, TrackNum, TrackQuality}, Collection, MergeCollections, }, @@ -259,63 +259,61 @@ impl MusicHoard { Ok(()) } - fn update_collection(&mut self, func: FN) -> Result<(), Error> + fn update_collection(&mut self, fn_coll: FnColl) -> Result<(), Error> where - FN: FnOnce(&mut Collection), + FnColl: FnOnce(&mut Collection), { - func(&mut self.pre_commit); + fn_coll(&mut self.pre_commit); self.commit() } - fn update_artist_and, FNARTIST, FNCOLL>( + fn update_artist_and( &mut self, - artist_id: ID, - fn_artist: FNARTIST, - fn_collection: FNCOLL, + artist_id: &ArtistId, + fn_artist: FnArtist, + fn_coll: FnColl, ) -> Result<(), Error> where - FNARTIST: FnOnce(&mut Artist), - FNCOLL: FnOnce(&mut Collection), + FnArtist: FnOnce(&mut Artist), + FnColl: FnOnce(&mut Collection), { - fn_artist(Self::get_artist_mut_or_err( - &mut self.pre_commit, - artist_id.as_ref(), - )?); - self.update_collection(fn_collection) + let artist = Self::get_artist_mut_or_err(&mut self.pre_commit, artist_id)?; + fn_artist(artist); + self.update_collection(fn_coll) } - fn update_artist, FN>( + fn update_artist( &mut self, - artist_id: ID, - func: FN, + artist_id: &ArtistId, + fn_artist: FnArtist, ) -> Result<(), Error> where - FN: FnOnce(&mut Artist), + FnArtist: FnOnce(&mut Artist), { - self.update_artist_and(artist_id, func, |_| {}) + self.update_artist_and(artist_id, fn_artist, |_| {}) } - fn update_album_and, ALBUM: AsRef, FNALBUM, FNARTIST, FNCOLL>( + fn update_album_and( &mut self, - artist_id: ARTIST, - album_id: ALBUM, - fn_album: FNALBUM, - fn_artist: FNARTIST, - fn_collection: FNCOLL, + artist_id: &ArtistId, + album_id: &AlbumId, + fn_album: FnAlbum, + fn_artist: FnArtist, + fn_coll: FnColl, ) -> Result<(), Error> where - FNALBUM: FnOnce(&mut Album), - FNARTIST: FnOnce(&mut Artist), - FNCOLL: FnOnce(&mut Collection), + FnAlbum: FnOnce(&mut Album), + FnArtist: FnOnce(&mut Artist), + FnColl: FnOnce(&mut Collection), { - let artist = Self::get_artist_mut_or_err(&mut self.pre_commit, artist_id.as_ref())?; - let album = Self::get_album_mut_or_err(artist, album_id.as_ref())?; + let artist = Self::get_artist_mut_or_err(&mut self.pre_commit, artist_id)?; + let album = Self::get_album_mut_or_err(artist, album_id)?; fn_album(album); fn_artist(artist); - self.update_collection(fn_collection) + self.update_collection(fn_coll) } - pub fn add_artist>(&mut self, artist_id: ID) -> Result<(), Error> { + pub fn add_artist>(&mut self, artist_id: IntoId) -> Result<(), Error> { let artist_id: ArtistId = artist_id.into(); self.update_collection(|collection| { @@ -326,7 +324,7 @@ impl MusicHoard { }) } - pub fn remove_artist>(&mut self, artist_id: ID) -> Result<(), Error> { + pub fn remove_artist>(&mut self, artist_id: Id) -> Result<(), Error> { self.update_collection(|collection| { let index_opt = collection.iter().position(|a| &a.id == artist_id.as_ref()); if let Some(index) = index_opt { @@ -335,102 +333,109 @@ impl MusicHoard { }) } - pub fn set_artist_sort, SORT: Into>( + pub fn set_artist_sort, IntoId: Into>( &mut self, - artist_id: ID, - artist_sort: SORT, + artist_id: Id, + artist_sort: IntoId, ) -> Result<(), Error> { self.update_artist_and( - artist_id, + artist_id.as_ref(), |artist| artist.set_sort_key(artist_sort), |collection| Self::sort_artists(collection), ) } - pub fn clear_artist_sort>(&mut self, artist_id: ID) -> Result<(), Error> { + pub fn clear_artist_sort>(&mut self, artist_id: Id) -> Result<(), Error> { self.update_artist_and( - artist_id, + artist_id.as_ref(), |artist| artist.clear_sort_key(), |collection| Self::sort_artists(collection), ) } - pub fn set_artist_musicbrainz, S: AsRef>( + pub fn set_artist_musicbrainz, Mb: TryInto, E>( &mut self, - artist_id: ID, - url: S, - ) -> Result<(), Error> { - let url = url.as_ref().try_into()?; - self.update_artist(artist_id, |artist| artist.set_musicbrainz_url(url)) + artist_id: Id, + url: Mb, + ) -> Result<(), Error> + where + Error: From, + { + let mb = url.try_into()?; + self.update_artist(artist_id.as_ref(), |artist| artist.set_musicbrainz_url(mb)) } - pub fn clear_artist_musicbrainz>( + pub fn clear_artist_musicbrainz>( &mut self, - artist_id: ID, + artist_id: Id, ) -> Result<(), Error> { - self.update_artist(artist_id, |artist| artist.clear_musicbrainz_url()) + self.update_artist(artist_id.as_ref(), |artist| artist.clear_musicbrainz_url()) } - pub fn add_to_artist_property, S: AsRef + Into>( + pub fn add_to_artist_property, S: AsRef + Into>( &mut self, - artist_id: ID, + artist_id: Id, property: S, values: Vec, ) -> Result<(), Error> { - self.update_artist(artist_id, |artist| artist.add_to_property(property, values)) + self.update_artist(artist_id.as_ref(), |artist| { + artist.add_to_property(property, values) + }) } - pub fn remove_from_artist_property, S: AsRef>( + pub fn remove_from_artist_property, S: AsRef>( &mut self, - artist_id: ID, + artist_id: Id, property: S, values: Vec, ) -> Result<(), Error> { - self.update_artist(artist_id, |artist| { + self.update_artist(artist_id.as_ref(), |artist| { artist.remove_from_property(property, values) }) } - pub fn set_artist_property, S: AsRef + Into>( + pub fn set_artist_property, S: AsRef + Into>( &mut self, - artist_id: ID, + artist_id: Id, property: S, values: Vec, ) -> Result<(), Error> { - self.update_artist(artist_id, |artist| artist.set_property(property, values)) + self.update_artist(artist_id.as_ref(), |artist| { + artist.set_property(property, values) + }) } - pub fn clear_artist_property, S: AsRef>( + pub fn clear_artist_property, S: AsRef>( &mut self, - artist_id: ID, + artist_id: Id, property: S, ) -> Result<(), Error> { - self.update_artist(artist_id, |artist| artist.clear_property(property)) + self.update_artist(artist_id.as_ref(), |artist| artist.clear_property(property)) } - pub fn set_album_seq, ALBUM: AsRef>( + pub fn set_album_seq, AlbumIdRef: AsRef>( &mut self, - artist_id: ARTIST, - album_id: ALBUM, + artist_id: ArtistIdRef, + album_id: AlbumIdRef, seq: u8, ) -> Result<(), Error> { self.update_album_and( - artist_id, - album_id, + artist_id.as_ref(), + album_id.as_ref(), |album| album.set_seq(AlbumSeq(seq)), |artist| artist.albums.sort_unstable(), |_| {}, ) } - pub fn clear_album_seq, ALBUM: AsRef>( + pub fn clear_album_seq, AlbumIdRef: AsRef>( &mut self, - artist_id: ARTIST, - album_id: ALBUM, + artist_id: ArtistIdRef, + album_id: AlbumIdRef, ) -> Result<(), Error> { self.update_album_and( - artist_id, - album_id, + artist_id.as_ref(), + album_id.as_ref(), |album| album.clear_seq(), |artist| artist.albums.sort_unstable(), |_| {}, @@ -621,7 +626,7 @@ mod tests { assert!(music_hoard .set_artist_musicbrainz(&artist_id, MUSICBRAINZ) .is_ok()); - _ = expected.insert(MusicBrainz::new(MUSICBRAINZ).unwrap()); + _ = expected.insert(MUSICBRAINZ.try_into().unwrap()); assert_eq!(music_hoard.collection[0].musicbrainz, expected); // Clearing URLs on an artist that does not exist is an error. @@ -733,6 +738,51 @@ mod tests { assert!(music_hoard.collection[0].properties.is_empty()); } + #[test] + fn set_clear_album_seq() { + let mut database = MockIDatabase::new(); + + let artist_id = ArtistId::new("an artist"); + let album_id = AlbumId::new("an album"); + let album_id_2 = AlbumId::new("another album"); + + let mut database_result = vec![Artist::new(artist_id.clone())]; + database_result[0].albums.push(Album { + id: album_id.clone(), + date: AlbumDate::default(), + seq: AlbumSeq::default(), + tracks: vec![], + }); + + database + .expect_load() + .times(1) + .return_once(|| Ok(database_result)); + database.expect_save().times(2).returning(|_| Ok(())); + + let mut music_hoard = MusicHoard::database(database).unwrap(); + assert_eq!(music_hoard.collection[0].albums[0].seq, AlbumSeq(0)); + + // Seting seq on an album not belonging to the artist is an error. + assert!(music_hoard + .set_album_seq(&artist_id, &album_id_2, 6) + .is_err()); + assert_eq!(music_hoard.collection[0].albums[0].seq, AlbumSeq(0)); + + // Set seq. + assert!(music_hoard.set_album_seq(&artist_id, &album_id, 6).is_ok()); + assert_eq!(music_hoard.collection[0].albums[0].seq, AlbumSeq(6)); + + // Clearing seq on an album that does not exist is an error. + assert!(music_hoard + .clear_album_seq(&artist_id, &album_id_2) + .is_err()); + + // Clear seq. + assert!(music_hoard.clear_album_seq(&artist_id, &album_id).is_ok()); + assert_eq!(music_hoard.collection[0].albums[0].seq, AlbumSeq(0)); + } + #[test] fn merge_collection_no_overlap() { let half: usize = FULL_COLLECTION.len() / 2; diff --git a/src/core/testmod.rs b/src/core/testmod.rs index e5863ae..a0620fe 100644 --- a/src/core/testmod.rs +++ b/src/core/testmod.rs @@ -1,5 +1,5 @@ use once_cell::sync::Lazy; -use std::collections::HashMap; +use std::{collections::HashMap, str::FromStr}; use crate::core::collection::{ album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq}, diff --git a/src/tests.rs b/src/tests.rs index aa8e5b2..c54bb6e 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -454,11 +454,9 @@ macro_rules! full_collection { let artist_a = iter.next().unwrap(); assert_eq!(artist_a.id.name, "Album_Artist ‘A’"); - artist_a.musicbrainz = Some( - MusicBrainz::new( - "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000", - ).unwrap(), - ); + artist_a.musicbrainz = Some(MusicBrainz::from_str( + "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000", + ).unwrap()); artist_a.properties = HashMap::from([ (String::from("MusicButler"), vec![ @@ -477,11 +475,9 @@ macro_rules! full_collection { let artist_b = iter.next().unwrap(); assert_eq!(artist_b.id.name, "Album_Artist ‘B’"); - artist_b.musicbrainz = Some( - MusicBrainz::new( - "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111", - ).unwrap(), - ); + artist_b.musicbrainz = Some(MusicBrainz::from_str( + "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111", + ).unwrap()); artist_b.properties = HashMap::from([ (String::from("MusicButler"), vec![ @@ -504,11 +500,9 @@ macro_rules! full_collection { let artist_c = iter.next().unwrap(); assert_eq!(artist_c.id.name, "The Album_Artist ‘C’"); - artist_c.musicbrainz = Some( - MusicBrainz::new( - "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111", - ).unwrap(), - ); + artist_c.musicbrainz = Some(MusicBrainz::from_str( + "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111", + ).unwrap()); // Nothing for artist_d diff --git a/src/tui/testmod.rs b/src/tui/testmod.rs index 8f3e8ef..1d7e506 100644 --- a/src/tui/testmod.rs +++ b/src/tui/testmod.rs @@ -1,10 +1,11 @@ +use std::{collections::HashMap, str::FromStr}; + use musichoard::collection::{ album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq}, artist::{Artist, ArtistId, MusicBrainz}, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, }; use once_cell::sync::Lazy; -use std::collections::HashMap; use crate::tests::*; diff --git a/tests/testlib.rs b/tests/testlib.rs index bc9fc2c..d2bc4d3 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -1,5 +1,5 @@ use once_cell::sync::Lazy; -use std::collections::HashMap; +use std::{collections::HashMap, str::FromStr}; use musichoard::collection::{ album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq}, @@ -17,8 +17,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { sort: Some(ArtistId{ name: String::from("Arkona") }), - musicbrainz: Some(MusicBrainz::new( - "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212", + musicbrainz: Some(MusicBrainz::from_str( + "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ @@ -204,7 +204,7 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { name: String::from("Eluveitie"), }, sort: None, - musicbrainz: Some(MusicBrainz::new( + musicbrainz: Some(MusicBrainz::from_str( "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38", ).unwrap()), properties: HashMap::from([ @@ -447,7 +447,7 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { name: String::from("Frontside"), }, sort: None, - musicbrainz: Some(MusicBrainz::new( + musicbrainz: Some(MusicBrainz::from_str( "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490", ).unwrap()), properties: HashMap::from([ @@ -600,7 +600,7 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { sort: Some(ArtistId { name: String::from("Heaven’s Basement"), }), - musicbrainz: Some(MusicBrainz::new( + musicbrainz: Some(MusicBrainz::from_str( "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc", ).unwrap()), properties: HashMap::from([ @@ -730,7 +730,7 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { name: String::from("Metallica"), }, sort: None, - musicbrainz: Some(MusicBrainz::new( + musicbrainz: Some(MusicBrainz::from_str( "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab", ).unwrap()), properties: HashMap::from([