From 414334009aa816ea2c2ab647ea4130a33828a917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christina=20S=C3=B8rensen?= Date: Sun, 2 Jul 2023 16:42:12 +0200 Subject: [PATCH] refactor: made code more idiomatic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Cargo.lock | 2 +- src/git.rs | 74 +++++++++++++++++++++++----------------------- src/main.rs | 20 ++++++------- src/test/test.yaml | 44 +++++++++++++-------------- src/utils/dir.rs | 2 +- 5 files changed, 71 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab04531..fd51770 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,7 +179,7 @@ dependencies = [ [[package]] name = "gg" -version = "0.0.6" +version = "0.0.7" dependencies = [ "clap", "clap_mangen", diff --git a/src/git.rs b/src/git.rs index 53e093c..0ef26e7 100644 --- a/src/git.rs +++ b/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>, // 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 = - Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into()); + let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op)); 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) { - 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 = - Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into()); + let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op)); 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) { - 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 = - Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op).into()); + let mut sp = Spinner::new(Spinners::Dots10, format!("{}: {}", repo.name, op)); 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 = 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 = 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(); } } diff --git a/src/main.rs b/src/main.rs index 6012201..bcc2635 100644 --- a/src/main.rs +++ b/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(); - test_file.write_all(contents.as_bytes()).unwrap(); + let contents = serde_yaml::to_string(&config).expect("failed to turn config into string"); + 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]; diff --git a/src/test/test.yaml b/src/test/test.yaml index b5b3327..1184f45 100644 --- a/src/test/test.yaml +++ b/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: - flags: [] + utils: repos: - qmk_firmware: - name: qmk_firmware - path: /home/ces/org/src/git/ - url: git@github.com:cafkafk/qmk_firmware.git + gg: + name: gg + path: /home/ces/.dots/ + 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 diff --git a/src/utils/dir.rs b/src/utils/dir.rs index 1530bab..9e90f71 100644 --- a/src/utils/dir.rs +++ b/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);