Change artist new/delete to add/remove (#92)

Closes #89

Reviewed-on: #92

src/bin/musichoard-edit.rs

@@ -42,9 +42,9 @@ impl Category {
 #[derive(StructOpt, Debug)]
 enum ArtistCommand {
     #[structopt(about = "Add a new artist to the collection")]
-    New(ArtistValue),
-    #[structopt(about = "Delete an artist from the collection")]
-    Delete(ArtistValue),
+    Add(ArtistValue),
+    #[structopt(about = "Remove an artist from the collection")]
+    Remove(ArtistValue),
     #[structopt(name = "musicbrainz", about = "Edit the MusicBrainz URL of an artist")]
     MusicBrainz(UrlCommand<SingleUrlValue>),
     #[structopt(
@@ -132,15 +132,11 @@ macro_rules! multi_url_command_dispatch {
 impl ArtistCommand {
     fn handle(self, music_hoard: &mut MH) {
         match self {
-            ArtistCommand::New(artist_value) => {
-                music_hoard
-                    .new_artist(ArtistId::new(artist_value.artist))
-                    .expect("failed to add new artist");
+            ArtistCommand::Add(artist_value) => {
+                music_hoard.add_artist(ArtistId::new(artist_value.artist));
             }
-            ArtistCommand::Delete(artist_value) => {
-                music_hoard
-                    .delete_artist(ArtistId::new(artist_value.artist))
-                    .expect("failed to delete artist");
+            ArtistCommand::Remove(artist_value) => {
+                music_hoard.remove_artist(ArtistId::new(artist_value.artist));
             }
             ArtistCommand::MusicBrainz(url_command) => {
                 single_url_command_dispatch!(url_command, music_hoard, musicbrainz)

src/lib.rs

@@ -829,38 +829,25 @@ impl<LIB: ILibrary, DB: IDatabase> MusicHoard<LIB, DB> {
         }
     }
 
-    pub fn new_artist<ID: Into<ArtistId>>(&mut self, artist_id: ID) -> Result<(), Error> {
+    pub fn add_artist<ID: Into<ArtistId>>(&mut self, artist_id: ID) {
         let artist_id: ArtistId = artist_id.into();
-        if let Ok(artist) = self.get_artist_or_err(&artist_id) {
-            return Err(Error::CollectionError(format!(
-                "artist '{}' is already in the collection",
-                artist.id
-            )));
+
+        if self.get_artist(&artist_id).is_none() {
+            let new_artist = vec![Artist::new(artist_id)];
+
+            let collection = mem::take(&mut self.collection);
+            self.collection = Self::merge(collection, new_artist);
         }
-
-        let new_artist = vec![Artist::new(artist_id)];
-
-        let collection = mem::take(&mut self.collection);
-        self.collection = Self::merge(collection, new_artist);
-
-        Ok(())
     }
 
-    pub fn delete_artist<ID: AsRef<ArtistId>>(&mut self, artist_id: ID) -> Result<(), Error> {
+    pub fn remove_artist<ID: AsRef<ArtistId>>(&mut self, artist_id: ID) {
         let index_opt = self
             .collection
             .iter()
             .position(|a| &a.id == artist_id.as_ref());
 
-        match index_opt {
-            Some(index) => {
-                self.collection.remove(index);
-                Ok(())
-            }
-            None => Err(Error::CollectionError(format!(
-                "artist '{}' is not in the collection",
-                artist_id.as_ref()
-            ))),
+        if let Some(index) = index_opt {
+            self.collection.remove(index);
         }
     }
 
@@ -953,13 +940,14 @@ impl<LIB: ILibrary, DB: IDatabase> MusicHoard<LIB, DB> {
         artists
     }
 
+    fn get_artist(&mut self, artist_id: &ArtistId) -> Option<&mut Artist> {
+        self.collection.iter_mut().find(|a| &a.id == artist_id)
+    }
+
     fn get_artist_or_err(&mut self, artist_id: &ArtistId) -> Result<&mut Artist, Error> {
-        self.collection
-            .iter_mut()
-            .find(|a| &a.id == artist_id)
-            .ok_or_else(|| {
-                Error::CollectionError(format!("artist '{}' is not in the collection", artist_id))
-            })
+        self.get_artist(artist_id).ok_or_else(|| {
+            Error::CollectionError(format!("artist '{}' is not in the collection", artist_id))
+        })
     }
 }
 
@@ -1088,23 +1076,35 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        let mut expected: Vec<Artist> = vec![];
 
-        let actual_err = music_hoard.new_artist(artist_id.clone()).unwrap_err();
-        let expected_err = Error::CollectionError(String::from(
-            "artist 'an artist' is already in the collection",
-        ));
+        music_hoard.add_artist(artist_id.clone());
+        expected.push(Artist::new(artist_id.clone()));
+        assert_eq!(music_hoard.collection, expected);
+
+        music_hoard.add_artist(artist_id.clone());
+        assert_eq!(music_hoard.collection, expected);
+
+        music_hoard.remove_artist(&artist_id_2);
+        assert_eq!(music_hoard.collection, expected);
+
+        music_hoard.remove_artist(&artist_id);
+        _ = expected.pop();
+        assert_eq!(music_hoard.collection, expected);
+    }
+
+    #[test]
+    fn collection_error() {
+        let artist_id = ArtistId::new("an artist");
+        let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
+
+        let actual_err = music_hoard
+            .add_musicbrainz_url(&artist_id, QOBUZ)
+            .unwrap_err();
+        let expected_err =
+            Error::CollectionError(String::from("artist 'an artist' is not in the collection"));
         assert_eq!(actual_err, expected_err);
         assert_eq!(actual_err.to_string(), expected_err.to_string());
-
-        let actual_err = music_hoard.delete_artist(&artist_id_2).unwrap_err();
-        let expected_err = Error::CollectionError(String::from(
-            "artist 'another artist' is not in the collection",
-        ));
-        assert_eq!(actual_err, expected_err);
-        assert_eq!(actual_err.to_string(), expected_err.to_string());
-
-        assert!(music_hoard.delete_artist(&artist_id).is_ok());
     }
 
     #[test]
@@ -1113,7 +1113,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Option<MusicBrainz> = None;
         assert_eq!(music_hoard.collection[0].properties.musicbrainz, expected);
@@ -1184,7 +1184,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Option<MusicBrainz> = None;
         assert_eq!(music_hoard.collection[0].properties.musicbrainz, expected);
@@ -1239,7 +1239,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Vec<MusicButler> = vec![];
         assert_eq!(music_hoard.collection[0].properties.musicbutler, expected);
@@ -1369,7 +1369,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Vec<MusicButler> = vec![];
         assert_eq!(music_hoard.collection[0].properties.musicbutler, expected);
@@ -1432,7 +1432,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Vec<Bandcamp> = vec![];
         assert_eq!(music_hoard.collection[0].properties.bandcamp, expected);
@@ -1562,7 +1562,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Vec<Bandcamp> = vec![];
         assert_eq!(music_hoard.collection[0].properties.bandcamp, expected);
@@ -1625,7 +1625,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Option<Qobuz> = None;
         assert_eq!(music_hoard.collection[0].properties.qobuz, expected);
@@ -1676,7 +1676,7 @@ mod tests {
         let artist_id_2 = ArtistId::new("another artist");
         let mut music_hoard = MusicHoard::<NoLibrary, NoDatabase>::new(None, None);
 
-        assert!(music_hoard.new_artist(artist_id.clone()).is_ok());
+        music_hoard.add_artist(artist_id.clone());
 
         let mut expected: Option<Qobuz> = None;
         assert_eq!(music_hoard.collection[0].properties.qobuz, expected);