refactor: made code more idiomatic

This was done with the help of:

`cargo clippy -- -W clippy::pedantic -W clippy::nursery -W clippy::unwrap_used`

Also fixed some warns that might have turned into errors in the future.

Signed-off-by: Christina Sørensen <christina@cafkafk.com>

Cargo.lock

@@ -179,7 +179,7 @@ dependencies = [
 
 [[package]]
 name = "gg"
-version = "0.0.6"
+version = "0.0.7"
 dependencies = [
  "clap",
  "clap_mangen",

src/git.rs

@@ -53,7 +53,7 @@ pub enum RepoFlags {
 /// For diagrams of the underlying architecture, consult ARCHITECHTURE.md
 ///
 ///
-#[derive(PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub struct Config {
     /// map of all categories
     ///
@@ -66,7 +66,7 @@ pub struct Config {
 /// Represents a category of repositories
 ///
 /// This allows you to organize your repositories into categories
-#[derive(PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub struct Category {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub flags: Option<Vec<RepoFlags>>, // FIXME: not implemented
@@ -78,7 +78,7 @@ pub struct Category {
 }
 
 /// Contain fields for a single link.
-#[derive(PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub struct Links {
     /// The name of the link
     pub name: String,
@@ -87,7 +87,7 @@ pub struct Links {
 }
 
 /// Holds a single git repository and related fields.
-#[derive(PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub struct GitRepo {
     pub name: String,
     pub path: String,
@@ -101,7 +101,7 @@ pub struct GitRepo {
 ////////////////////////////////////
 
 /// Represents a single operation on a repository
-struct SeriesItem<'series> {
+pub struct SeriesItem<'series> {
     /// The string to be displayed to the user
     operation: &'series str,
     /// The closure representing the actual operation
@@ -114,7 +114,10 @@ struct SeriesItem<'series> {
 
 fn handle_file_exists(selff: &Links, tx_path: &Path, rx_path: &Path) {
     match rx_path.read_link() {
-        Ok(file) if file.canonicalize().unwrap() == tx_path.canonicalize().unwrap() => {
+        Ok(file)
+            if file.canonicalize().expect("failed to canonicalize file")
+                == tx_path.canonicalize().expect("failed to canonicalize path") =>
+        {
             debug!(
                 "Linking {} -> {} failed: file already linked",
                 &selff.tx, &selff.rx
@@ -291,7 +294,7 @@ impl GitRepo {
     /// Removes a repository (not implemented)
     ///
     /// Kept here as a reminder that we probably shouldn't do this
-    fn remove(&self) -> Result<(), std::io::Error> {
+    fn remove() -> Result<(), std::io::Error> {
         // https://doc.rust-lang.org/std/fs/fn.remove_dir_all.html
         unimplemented!("This seems to easy to missuse/exploit");
         // fs::remove_dir_all(format!("{}{}", &self.path, &self.name))
@@ -327,7 +330,7 @@ impl Config {
     where
         F: Fn(&GitRepo),
     {
-        for (_, category) in self.categories.iter() {
+        for category in self.categories.values() {
             for (_, repo) in category.repos.as_ref().expect("failed to get repos").iter() {
                 f(repo);
             }
@@ -342,14 +345,13 @@ impl Config {
     where
         F: Fn(&GitRepo) -> bool,
     {
-        for (_, category) in self.categories.iter() {
+        for category in self.categories.values() {
             for (_, repo) in category.repos.as_ref().expect("failed to get repos").iter() {
-                let mut sp =
+                let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op));
-                    Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into());
                 if f(repo) {
-                    sp.stop_and_persist("✔", format!("{}: {}", repo.name, op).into());
+                    sp.stop_and_persist("✔", format!("{}: {}", repo.name, op));
                 } else {
-                    sp.stop_and_persist("❎", format!("{}: {}", repo.name, op).into());
+                    sp.stop_and_persist("❎", format!("{}: {}", repo.name, op));
                 }
             }
         }
@@ -396,17 +398,16 @@ impl Config {
     /// self.series_on_all(series);
     /// ```
     pub fn series_on_all(&self, closures: Vec<SeriesItem>) {
-        for (_, category) in self.categories.iter() {
+        for category in self.categories.values() {
             for (_, repo) in category.repos.as_ref().expect("failed to get repos").iter() {
-                for instruction in closures.iter() {
+                for instruction in &closures {
                     let f = &instruction.closure;
                     let op = instruction.operation;
-                    let mut sp =
+                    let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op));
-                        Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into());
                     if f(repo) {
-                        sp.stop_and_persist("✔", format!("{}: {}", repo.name, op).into());
+                        sp.stop_and_persist("✔", format!("{}: {}", repo.name, op));
                     } else {
-                        sp.stop_and_persist("❎", format!("{}: {}", repo.name, op).into());
+                        sp.stop_and_persist("❎", format!("{}: {}", repo.name, op));
                         break;
                     }
                 }
@@ -443,17 +444,16 @@ impl Config {
     /// self.all_on_all(series);
     /// ```
     pub fn all_on_all(&self, closures: Vec<SeriesItem>) {
-        for (_, category) in self.categories.iter() {
+        for category in self.categories.values() {
             for (_, repo) in category.repos.as_ref().expect("failed to get repos").iter() {
-                for instruction in closures.iter() {
+                for instruction in &closures {
                     let f = &instruction.closure;
                     let op = instruction.operation;
-                    let mut sp =
+                    let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op));
-                        Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into());
                     if f(repo) {
-                        sp.stop_and_persist("✔", format!("{}: {}", repo.name, op).into());
+                        sp.stop_and_persist("✔", format!("{}: {}", repo.name, op));
                     } else {
-                        sp.stop_and_persist("❎", format!("{}: {}", repo.name, op).into());
+                        sp.stop_and_persist("❎", format!("{}: {}", repo.name, op));
                     }
                 }
             }
@@ -465,22 +465,22 @@ impl Config {
     /// Tries to pull all repositories, skips if fail.
     pub fn pull_all(&self) {
         debug!("exectuting pull_all");
-        self.on_all_spinner("pull", |repo| repo.pull());
+        self.on_all_spinner("pull", GitRepo::pull);
     }
     /// Tries to clone all repositories, skips if fail.
     pub fn clone_all(&self) {
         debug!("exectuting clone_all");
-        self.on_all_spinner("clone", |repo| repo.clone());
+        self.on_all_spinner("clone", GitRepo::clone);
     }
     /// Tries to add all work in all repositories, skips if fail.
     pub fn add_all(&self) {
         debug!("exectuting clone_all");
-        self.on_all_spinner("add", |repo| repo.add_all());
+        self.on_all_spinner("add", GitRepo::add_all);
     }
     /// Tries to commit all repositories one at a time, skips if fail.
     pub fn commit_all(&self) {
         debug!("exectuting clone_all");
-        self.on_all_spinner("commit", |repo| repo.commit());
+        self.on_all_spinner("commit", GitRepo::commit);
     }
     /// Tries to commit all repositories with msg, skips if fail.
     pub fn commit_all_msg(&self, msg: &str) {
@@ -494,11 +494,11 @@ impl Config {
         let series: Vec<SeriesItem> = vec![
             SeriesItem {
                 operation: "pull",
-                closure: Box::new(move |repo: &GitRepo| repo.pull()),
+                closure: Box::new(GitRepo::pull),
             },
             SeriesItem {
                 operation: "add",
-                closure: Box::new(move |repo: &GitRepo| repo.add_all()),
+                closure: Box::new(GitRepo::add_all),
             },
             SeriesItem {
                 operation: "commit",
@@ -506,7 +506,7 @@ impl Config {
             },
             SeriesItem {
                 operation: "push",
-                closure: Box::new(move |repo: &GitRepo| repo.push()),
+                closure: Box::new(GitRepo::push),
             },
         ];
         self.all_on_all(series);
@@ -518,19 +518,19 @@ impl Config {
         let series: Vec<SeriesItem> = vec![
             SeriesItem {
                 operation: "pull",
-                closure: Box::new(move |repo: &GitRepo| repo.pull()),
+                closure: Box::new(GitRepo::pull),
             },
             SeriesItem {
                 operation: "add",
-                closure: Box::new(move |repo: &GitRepo| repo.add_all()),
+                closure: Box::new(GitRepo::add_all),
             },
             SeriesItem {
                 operation: "commit",
-                closure: Box::new(move |repo: &GitRepo| repo.commit()),
+                closure: Box::new(move |repo: &GitRepo| repo.commit_with_msg(msg)),
             },
             SeriesItem {
                 operation: "push",
-                closure: Box::new(move |repo: &GitRepo| repo.push()),
+                closure: Box::new(GitRepo::push),
             },
         ];
         self.series_on_all(series);
@@ -541,7 +541,7 @@ impl Config {
     /// Tries to link all repositories, skips if fail.
     pub fn link_all(&self) {
         debug!("exectuting link_all");
-        for link in self.links.iter() {
+        for link in &self.links {
             link.link();
         }
     }

src/main.rs

@@ -99,7 +99,7 @@ fn main() {
             config.commit_all();
         }
         Some(Commands::CommitMsg { msg }) => {
-            config.commit_all_msg(msg.as_ref().unwrap());
+            config.commit_all_msg(msg.as_ref().expect("failed to get message from input"));
         }
         None => (),
     }
@@ -154,8 +154,6 @@ mod config {
                     },
                 );
         }
-        // let yaml = serde_yaml::to_string(&config).unwrap();
-        // println!("{}", yaml);
     }
     #[test]
     fn read_config_populate() {
@@ -163,13 +161,13 @@ mod config {
     }
     #[test]
     fn write_config() {
-        let root = current_dir().unwrap();
+        let root = current_dir().expect("failed to get current dir");
         let config = Config::new(
             &RelativePath::new("./src/test/config.yaml")
                 .to_logical_path(&root)
                 .into_os_string()
                 .into_string()
-                .unwrap(),
+                .expect("failed to turn config into string"),
         );
 
         let mut test_file = File::create(
@@ -177,11 +175,13 @@ mod config {
                 .to_logical_path(&root)
                 .into_os_string()
                 .into_string()
-                .unwrap(),
+                .expect("failed to turn config into string"),
         )
         .expect("failed to create test file");
-        let contents = serde_yaml::to_string(&config).unwrap();
+        let contents = serde_yaml::to_string(&config).expect("failed to turn config into string");
-        test_file.write_all(contents.as_bytes()).unwrap();
+        test_file
+            .write_all(contents.as_bytes())
+            .expect("failed to write contents of config into file");
 
         let test_config = Config::new(&RelativePath::new("./src/test/test.yaml").to_string());
         assert_eq!(config, test_config);
@@ -205,13 +205,13 @@ mod config {
     }
     #[test]
     fn is_config_readable() {
-        let root = current_dir().unwrap();
+        let root = current_dir().expect("failed to get current dir");
         let config = Config::new(
             &RelativePath::new("./src/test/config.yaml")
                 .to_logical_path(root)
                 .into_os_string()
                 .into_string()
-                .unwrap(),
+                .expect("failed to turnn config into string"),
         );
 
         let flags = vec![Clone, Push];

src/test/test.yaml

@@ -1,20 +1,4 @@
 categories:
-  utils:
-    repos:
-      li:
-        name: li
-        path: /home/ces/org/src/git/
-        url: git@github.com:cafkafk/li.git
-        flags:
-        - Clone
-        - Push
-      gg:
-        name: gg
-        path: /home/ces/.dots/
-        url: git@github.com:cafkafk/gg.git
-        flags:
-        - Clone
-        - Push
   stuff:
     flags: []
     repos:
@@ -26,16 +10,25 @@ categories:
         name: gg
         path: /home/ces/.dots/
         url: git@github.com:cafkafk/gg.git
-  config:
+  utils:
-    flags: []
     repos:
-      qmk_firmware:
+      gg:
-        name: qmk_firmware
+        name: gg
-        path: /home/ces/org/src/git/
+        path: /home/ces/.dots/
-        url: git@github.com:cafkafk/qmk_firmware.git
+        url: git@github.com:cafkafk/gg.git
         flags:
         - Clone
         - Push
+      li:
+        name: li
+        path: /home/ces/org/src/git/
+        url: git@github.com:cafkafk/li.git
+        flags:
+        - Clone
+        - Push
+  config:
+    flags: []
+    repos:
       starship:
         name: starship
         path: /home/ces/org/src/git/
@@ -43,6 +36,13 @@ categories:
         flags:
         - Clone
         - Push
+      qmk_firmware:
+        name: qmk_firmware
+        path: /home/ces/org/src/git/
+        url: git@github.com:cafkafk/qmk_firmware.git
+        flags:
+        - Clone
+        - Push
   empty: {}
 links:
 - name: gg

src/utils/dir.rs

@@ -50,7 +50,7 @@ pub fn home_dir() -> String {
 ///
 /// WARNING: NOT THREAD SAFE
 fn change_dir_repo(path: &str, name: &str) {
-    let mut full_path: String = "".to_owned();
+    let mut full_path: String = String::new();
     full_path.push_str(path);
     full_path.push_str(name);
     let root = Path::new(&full_path);