From d5ad0ab0a165b4926d189b2a7cbef676fb39e2a1 Mon Sep 17 00:00:00 2001 From: "Dr. Matthias Ratajczak" Date: Mon, 13 Feb 2023 20:39:58 +0100 Subject: [PATCH] adjust lint groups, start fixing lints --- README.md | 1 + crates/core/src/args.rs | 15 ++++++---- crates/core/src/backend/backend_trait.rs | 10 +++++-- crates/core/src/cmd.rs | 6 +++- crates/core/src/config.rs | 2 +- crates/core/src/core.rs | 22 ++++++++++---- crates/core/src/grouping/group.rs | 37 ++++++++++++++++-------- crates/core/src/grouping/package.rs | 3 +- crates/core/src/grouping/section.rs | 12 ++++++-- crates/core/src/lib.rs | 9 ++++++ crates/core/src/review/strategy.rs | 2 +- crates/core/src/ui.rs | 6 ++-- 12 files changed, 91 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 88f973c..49971d5 100644 --- a/README.md +++ b/README.md @@ -210,4 +210,5 @@ topgrade * add zsh completion * add license +* in outermost Cargo.toml, add categories, keywords, readme diff --git a/crates/core/src/args.rs b/crates/core/src/args.rs index ea7e052..6db19f6 100644 --- a/crates/core/src/args.rs +++ b/crates/core/src/args.rs @@ -1,5 +1,6 @@ use std::path::PathBuf; +use anyhow::{Context, Result}; use clap::{Arg, ArgMatches, Command}; use path_absolutize::Absolutize; @@ -71,12 +72,16 @@ pub fn get() -> clap::ArgMatches { get_arg_parser().get_matches() } -pub(crate) fn get_absolutized_file_paths(arg_match: &ArgMatches) -> Vec { - arg_match +pub(crate) fn get_absolutized_file_paths(arg_match: &ArgMatches) -> Result> { + Ok(arg_match .get_many::("files") - .unwrap() + .context("getting files from args")? .cloned() .map(PathBuf::from) - .map(|path| path.absolutize().unwrap().into_owned()) - .collect() + .map(|path| { + path.absolutize() + .expect("absolute path should exist") + .into_owned() + }) + .collect()) } diff --git a/crates/core/src/backend/backend_trait.rs b/crates/core/src/backend/backend_trait.rs index 97c0821..3dfe7fb 100644 --- a/crates/core/src/backend/backend_trait.rs +++ b/crates/core/src/backend/backend_trait.rs @@ -30,13 +30,15 @@ pub(crate) trait Backend: Debug { /// Get all packages that were installed in the system explicitly. fn get_explicitly_installed_packages(&self) -> Result>; - fn assign_group(&self, to_assign: Vec<(Package, Rc)>) { + fn assign_group(&self, to_assign: Vec<(Package, Rc)>) -> Result<()> { let group_package_map = get_group_packages_map(to_assign); let section_header = format!("[{}]", self.get_section()); for (group, packages) in group_package_map { - group.save_packages(§ion_header, &packages); + group.save_packages(§ion_header, &packages)?; } + + Ok(()) } /// Install the specified packages. @@ -125,7 +127,9 @@ fn get_group_packages_map( group_package_map.insert(group.clone(), vec![]); } - let inner = group_package_map.get_mut(&group).unwrap(); + let inner = group_package_map + .get_mut(&group) + .expect("either it was already there or we created it"); inner.push(p); } diff --git a/crates/core/src/cmd.rs b/crates/core/src/cmd.rs index a829bba..0725cc1 100644 --- a/crates/core/src/cmd.rs +++ b/crates/core/src/cmd.rs @@ -7,7 +7,11 @@ use crate::env::get_editor; pub fn run_edit_command(files: &[PathBuf]) -> Result { let mut cmd = Command::new(get_editor().context("getting suitable editor")?); - cmd.current_dir(files[0].parent().unwrap()); + cmd.current_dir( + files[0] + .parent() + .context("getting parent dir of first file argument")?, + ); for f in files { cmd.arg(f.to_string_lossy().to_string()); } diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 01474a3..0200a7f 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -30,7 +30,7 @@ impl Config { }; } - let content = from_file.unwrap(); + let content = from_file.expect("we already handled that error"); serde_yaml::from_str(&content).context("parsing yaml config") } diff --git a/crates/core/src/core.rs b/crates/core/src/core.rs index 756c156..ea61830 100644 --- a/crates/core/src/core.rs +++ b/crates/core/src/core.rs @@ -26,7 +26,7 @@ pub struct Pacdef { impl Pacdef { #[must_use] - pub fn new(args: ArgMatches, config: Config, groups: HashSet) -> Self { + pub const fn new(args: ArgMatches, config: Config, groups: HashSet) -> Self { Self { args, config, @@ -106,6 +106,7 @@ impl Pacdef { to_install.install_missing_packages() } + #[allow(clippy::unused_self)] fn edit_group_files(&self, groups: &ArgMatches) -> Result<()> { let group_dir = crate::path::get_pacdef_group_dir()?; @@ -135,6 +136,7 @@ impl Pacdef { Ok(()) } + #[allow(clippy::unused_self)] fn show_version(self) { println!("{}", get_version_string()); } @@ -188,7 +190,10 @@ impl Pacdef { } fn show_group_content(&self, groups: &ArgMatches) -> Result<()> { - let mut iter = groups.get_many::("group").unwrap().peekable(); + let mut iter = groups + .get_many::("group") + .context("getting groups from args")? + .peekable(); let show_more_than_one_group = iter.size_hint().0 > 1; @@ -217,12 +222,17 @@ impl Pacdef { Ok(()) } + #[allow(clippy::unused_self)] fn import_groups(&self, args: &ArgMatches) -> Result<()> { - let files = args::get_absolutized_file_paths(args); + let files = args::get_absolutized_file_paths(args)?; let groups_dir = get_pacdef_group_dir()?; for target in files { - let target_name = target.file_name().unwrap().to_str().unwrap(); + let target_name = target + .file_name() + .context("path should not end in '..'")? + .to_str() + .context("filename is not valid UTF-8")?; if !target.exists() { println!("file {target_name} does not exist, skipping"); @@ -242,6 +252,7 @@ impl Pacdef { Ok(()) } + #[allow(clippy::unused_self)] fn remove_groups(&self, arg_match: &ArgMatches) -> Result<()> { let paths = get_assumed_group_file_names(arg_match)?; @@ -256,6 +267,7 @@ impl Pacdef { Ok(()) } + #[allow(clippy::unused_self)] fn new_groups(&self, arg: &ArgMatches) -> Result<()> { let paths = get_assumed_group_file_names(arg)?; @@ -284,7 +296,7 @@ fn get_assumed_group_file_names(arg_match: &ArgMatches) -> Result> let paths: Vec<_> = arg_match .get_many::("groups") - .unwrap() + .context("getting groups from args")? .map(|s| { let mut possible_group_file = groups_dir.clone(); possible_group_file.push(s); diff --git a/crates/core/src/grouping/group.rs b/crates/core/src/grouping/group.rs index cfbae6f..8d65ce2 100644 --- a/crates/core/src/grouping/group.rs +++ b/crates/core/src/grouping/group.rs @@ -31,7 +31,7 @@ impl Group { } let group = - Group::try_from(&path).with_context(|| format!("reading group file {path:?}"))?; + Self::try_from(&path).with_context(|| format!("reading group file {path:?}"))?; result.insert(group); } @@ -111,17 +111,21 @@ impl Group { }) } - pub(crate) fn save_packages(&self, section_header: &str, packages: &[Package]) { - let mut content = read_to_string(&self.path).unwrap(); + pub(crate) fn save_packages(&self, section_header: &str, packages: &[Package]) -> Result<()> { + let mut content = read_to_string(&self.path) + .with_context(|| format!("reading existing file contents from {:?}", &self.path))?; if content.contains(section_header) { - write_packages_to_existing_section(&mut content, section_header, packages); + write_packages_to_existing_section(&mut content, section_header, packages) + .context("existing section")?; } else { add_new_section_with_packages(&mut content, section_header, packages); } - let mut file = File::create(&self.path).unwrap(); - write!(file, "{content}").unwrap(); + let mut file = File::create(&self.path) + .with_context(|| format!("creating descriptor to output file {:?}", &self.path))?; + + write!(file, "{content}").with_context(|| format!("writing file {:?}", &self.path)) } } @@ -146,9 +150,9 @@ fn write_packages_to_existing_section( group_file_content: &mut String, section_header: &str, packages: &[Package], -) { +) -> Result<()> { let idx_of_first_package_line_in_section = - find_first_package_line_in_section(group_file_content, section_header); + find_first_package_line_in_section(group_file_content, section_header)?; let after = group_file_content.split_off(idx_of_first_package_line_in_section); @@ -157,13 +161,22 @@ fn write_packages_to_existing_section( } group_file_content.push_str(&after); + Ok(()) } -fn find_first_package_line_in_section(group_file_content: &str, section_header: &str) -> usize { - let section_start = group_file_content.find(section_header).unwrap(); - let distance_to_next_newline = group_file_content[section_start..].find('\n').unwrap(); +fn find_first_package_line_in_section( + group_file_content: &str, + section_header: &str, +) -> Result { + let section_start = group_file_content + .find(section_header) + .context("finding first package after section header")?; - section_start + distance_to_next_newline + 1 // + 1 to be after the newline + let distance_to_next_newline = group_file_content[section_start..] + .find('\n') + .context("getting next newline")?; + + Ok(section_start + distance_to_next_newline + 1) // + 1 to be after the newline } fn add_new_section_with_packages( diff --git a/crates/core/src/grouping/package.rs b/crates/core/src/grouping/package.rs index f3288f2..40ecfd1 100644 --- a/crates/core/src/grouping/package.rs +++ b/crates/core/src/grouping/package.rs @@ -27,7 +27,7 @@ impl From for Package { impl Package { fn split_into_name_and_repo(s: &str) -> (String, Option) { let mut iter = s.split('/').rev(); - let name = iter.next().unwrap().to_string(); + let name = iter.next().expect("we checked that earlier").to_string(); let repo = iter.next().map(|s| s.to_string()); (name, repo) } @@ -95,6 +95,7 @@ mod tests { assert_eq!(repo, None); } + #[allow(clippy::unwrap_used)] #[test] fn from() { let x = "myrepo/somepackage # ".to_string(); diff --git a/crates/core/src/grouping/section.rs b/crates/core/src/grouping/section.rs index b74401f..1341d85 100644 --- a/crates/core/src/grouping/section.rs +++ b/crates/core/src/grouping/section.rs @@ -32,9 +32,15 @@ impl Section { .to_string(); let mut packages = HashSet::new(); - // `while let` is unstable, unfortunately - while iter.peek().is_some() && !iter.peek().unwrap().starts_with('[') { - if let Some(package) = Package::try_from(iter.next().unwrap()) { + // `while let` chains are unstable, unfortunately + while iter.peek().is_some() + && !iter + .peek() + .expect("we checked this is some") + .starts_with('[') + { + if let Some(package) = Package::try_from(iter.next().expect("we checked this is some")) + { packages.insert(package); } } diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index e68c391..7d1d407 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,3 +1,12 @@ +#![warn( + clippy::as_conversions, + clippy::use_debug, + clippy::unwrap_used, + clippy::wildcard_dependencies, + clippy::use_self, + clippy::unused_self +)] + mod action; pub mod args; mod backend; diff --git a/crates/core/src/review/strategy.rs b/crates/core/src/review/strategy.rs index ca50ead..12b0e42 100644 --- a/crates/core/src/review/strategy.rs +++ b/crates/core/src/review/strategy.rs @@ -40,7 +40,7 @@ impl Strategy { } if !self.assign_group.is_empty() { - self.backend.assign_group(self.assign_group); + self.backend.assign_group(self.assign_group)?; } Ok(()) diff --git a/crates/core/src/ui.rs b/crates/core/src/ui.rs index 14dda1b..fc26703 100644 --- a/crates/core/src/ui.rs +++ b/crates/core/src/ui.rs @@ -5,7 +5,7 @@ use termios::*; pub(crate) fn get_user_confirmation() -> Result { print!("Continue? [Y/n] "); - std::io::stdout().flush().unwrap(); + std::io::stdout().flush().context("flushing stdout")?; let mut reply = String::new(); std::io::stdin() @@ -29,7 +29,9 @@ pub(crate) fn read_single_char_from_terminal() -> Result { io::stdin() .read_exact(&mut input[..]) .context("reading one byte from stdin")?; - let result = input[0] as char; + let result: char = input[0] + .try_into() + .context("reading a single byte from stdin")?; // stdin is not echoed automatically in this terminal mode println!("{result}");