From 1dec32c37ac2f563e164021c9f21052f11fd1d20 Mon Sep 17 00:00:00 2001 From: Michael Gattozzi Date: Mon, 9 Dec 2019 17:42:08 -0500 Subject: Make the pre-commit script pedantic and fix errors This commit really ups the level and quality of the Rust code by setting clippy to pedantic mode. It also fixes an issue where bash continued to run scripts even if something failed with a non-zero exit status. We also deny all warnings so as to actually fail the builds and the commit hooks. This should make sure code quality stays at a high level. --- .cargo/config | 61 +++++++++++++++++++++++++++++++++++++ .dev-suite/hooked/pre-commit | 4 ++- hooked/src/bin/hooked-commit-msg.rs | 2 ++ hooked/src/main.rs | 14 +++++---- hooked/tests/init.rs | 4 +-- shared/src/lib.rs | 5 ++- src/main.rs | 9 ++++-- ticket/src/actions.rs | 17 ++++++----- ticket/src/main.rs | 47 +++++++++++++++++++--------- ticket/src/tui.rs | 16 +++++----- 10 files changed, 137 insertions(+), 42 deletions(-) create mode 100644 .cargo/config diff --git a/.cargo/config b/.cargo/config new file mode 100644 index 0000000..76fe2f0 --- /dev/null +++ b/.cargo/config @@ -0,0 +1,61 @@ +[build] +rustflags = [ + "-D", + "warnings", + "-D", + "missing-debug-implementations", + "-D", + "missing-docs", + "-D", + "trivial-casts", + "-D", + "trivial-numeric-casts", + "-D", + "unused-extern-crates", + "-D", + "unused-import-braces", + "-D", + "unused-qualifications", + "-D", + "unused-results", + "-D", + "bad-style", + "-D", + "const-err", + "-D", + "dead-code", + "-D", + "improper-ctypes", + "-D", + "legacy-directory-ownership", + "-D", + "non-shorthand-field-patterns", + "-D", + "no-mangle-generic-items", + "-D", + "overflowing-literals", + "-D", + "path_statements", + "-D", + "patterns-in-fns-without-body", + "-D", + "plugin-as-library", + "-D", + "private-in-public", + "-D", + "safe-extern-statics", + "-D", + "unconditional-recursion", + "-D", + "unions-with-drop-fields", + "-D", + "unused", + "-D", + "unused-allocation", + "-D", + "unused-comparisons", + "-D", + "unused-parens", + "-D", + "while-true" +] diff --git a/.dev-suite/hooked/pre-commit b/.dev-suite/hooked/pre-commit index 9ae83cf..931d7ac 100755 --- a/.dev-suite/hooked/pre-commit +++ b/.dev-suite/hooked/pre-commit @@ -1,5 +1,7 @@ #! /bin/bash +set -eu -o pipefail + cargo build --all cargo test --all cargo +nightly fmt --all -- --check -cargo clippy --all --all-targets +cargo clippy --all --all-targets -- -W clippy::pedantic diff --git a/hooked/src/bin/hooked-commit-msg.rs b/hooked/src/bin/hooked-commit-msg.rs index 7f2a996..b7de022 100644 --- a/hooked/src/bin/hooked-commit-msg.rs +++ b/hooked/src/bin/hooked-commit-msg.rs @@ -1,3 +1,5 @@ +//! Linting tool for commit messages + use shared::find_root; use std::{ env::args, diff --git a/hooked/src/main.rs b/hooked/src/main.rs index a3d8199..7e33499 100644 --- a/hooked/src/main.rs +++ b/hooked/src/main.rs @@ -1,3 +1,5 @@ +//! git hook manager tool + #[cfg(windows)] use anyhow::bail; use anyhow::Result; @@ -45,9 +47,9 @@ enum Args { #[paw::main] fn main(args: Args) { - env::var("RUST_LOG").map(drop).unwrap_or_else(|_| { - env::set_var("RUST_LOG", "info"); - }); + env::var("RUST_LOG") + .ok() + .map_or_else(|| env::set_var("RUST_LOG", "info"), drop); pretty_env_logger::init(); if let Err(e) = match args { Args::Init => init(), @@ -74,7 +76,9 @@ fn init() -> Result<()> { let git_hook = &git_hooks.join(hook); debug!("git_hook path: {}", git_hook.display()); - if !path.exists() { + if path.exists() { + debug!("git hook {} already exists. Skipping creation.", hook); + } else { debug!("Creating dev-suite hook."); let mut file = fs::File::create(&path)?; trace!("File created."); @@ -86,8 +90,6 @@ fn init() -> Result<()> { file.write_all(b"#! /bin/bash")?; debug!("Writing data to file."); debug!("Created git hook {}.", hook); - } else { - debug!("git hook {} already exists. Skipping creation.", hook); } let path = path.canonicalize()?; diff --git a/hooked/tests/init.rs b/hooked/tests/init.rs index 2596d5b..65c4aab 100644 --- a/hooked/tests/init.rs +++ b/hooked/tests/init.rs @@ -33,10 +33,10 @@ const HOOKS: [&str; 18] = [ #[test] fn init() -> Result<(), Box> { let dir = tempdir()?; - Repository::init(&dir)?; + let _ = Repository::init(&dir)?; let mut cmd = Command::cargo_bin("hooked")?; env::set_current_dir(&dir)?; - cmd.arg("init").assert().success(); + let _ = cmd.arg("init").assert().success(); let git = &dir.path().join(".git").join("hooks"); let dev = &dir.path().join(".dev-suite").join("hooked"); diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 138e8d0..db28822 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -1,3 +1,5 @@ +//! Common functionality needed in many of the tools + use anyhow::{ bail, Result, @@ -7,6 +9,7 @@ use std::{ path::PathBuf, }; +/// Finds the top level folder of the repo and returns it's canonicalized path pub fn find_root() -> Result { let mut location = env::current_dir()?; let mut found_root = false; @@ -14,7 +17,7 @@ pub fn find_root() -> Result { for loc in location.ancestors() { let mut loc = loc.join(".git"); if loc.exists() { - loc.pop(); + let _ = loc.pop(); found_root = true; location = loc.canonicalize()?; break; diff --git a/src/main.rs b/src/main.rs index 7869f4d..a1fac85 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +//! dev-suite cli tool to install and update devsuite and it's tooling use anyhow::{ format_err, Result, @@ -32,9 +33,10 @@ fn main(args: Args) { } } +/// Initialize a git repo with all the tools wanted for it fn init() -> Result<()> { // Make sure we're in a valid git repo - find_root()?; + let _ = find_root()?; let checkboxes = &["hooked - Managed git hooks", "ticket - In repo tickets"]; let defaults = &[true, true]; let selections = Checkboxes::with_theme(&ColorfulTheme::default()) @@ -61,7 +63,7 @@ fn init() -> Result<()> { .map_err(|_| format_err!( "It looks like hooked is not on your $PATH. Did you run 'ds install'?" ))?; - Command::new("hooked").arg("init").spawn()?.wait()?; + let _ = Command::new("hooked").arg("init").spawn()?.wait()?; } Tools::Ticket => { which("ticket") @@ -69,7 +71,7 @@ fn init() -> Result<()> { .map_err(|_| format_err!( "It looks like ticket is not on your $PATH. Did you run 'ds install'?" ))?; - Command::new("ticket").arg("init").spawn()?.wait()?; + let _ = Command::new("ticket").arg("init").spawn()?.wait()?; } } } @@ -77,6 +79,7 @@ fn init() -> Result<()> { Ok(()) } +/// Tools available for installation or usage by dev-suite enum Tools { Hooked, Ticket, diff --git a/ticket/src/actions.rs b/ticket/src/actions.rs index 3e237ff..0d371b3 100644 --- a/ticket/src/actions.rs +++ b/ticket/src/actions.rs @@ -10,18 +10,21 @@ use log::*; use shared::find_root; use std::{ fs, - path::PathBuf, + path::{ + Path, + PathBuf, + }, }; pub fn get_open_tickets() -> Result> { - get_tickets(open_tickets()?) + get_tickets(&open_tickets()?) } pub fn get_closed_tickets() -> Result> { - get_tickets(closed_tickets()?) + get_tickets(&closed_tickets()?) } -fn get_tickets(path: PathBuf) -> Result> { +fn get_tickets(path: &Path) -> Result> { let mut out = Vec::new(); debug!("Looking for ticket."); for entry in fs::read_dir(&path)? { @@ -63,14 +66,14 @@ pub fn get_all_ticketsv0() -> Result> { Ok(tickets) } pub fn get_open_ticketsv0() -> Result> { - get_ticketsv0(open_tickets()?) + get_ticketsv0(&open_tickets()?) } pub fn get_closed_ticketsv0() -> Result> { - get_ticketsv0(closed_tickets()?) + get_ticketsv0(&closed_tickets()?) } -fn get_ticketsv0(path: PathBuf) -> Result> { +fn get_ticketsv0(path: &Path) -> Result> { let mut out = Vec::new(); debug!("Looking for ticket."); for entry in fs::read_dir(&path)? { diff --git a/ticket/src/main.rs b/ticket/src/main.rs index 11e27e8..74a5699 100644 --- a/ticket/src/main.rs +++ b/ticket/src/main.rs @@ -1,3 +1,6 @@ +//! ticket is a cli tool to create, delete, and manage tickets as part of +//! repository, rather than a separate service outside the history of the +//! code. mod actions; mod tui; @@ -18,6 +21,7 @@ use serde::{ Serialize, }; use std::{ + convert::TryInto, env, fs, process, @@ -55,9 +59,9 @@ enum Cmd { #[paw::main] fn main(args: Args) { - env::var("RUST_LOG").map(drop).unwrap_or_else(|_| { - env::set_var("RUST_LOG", "info"); - }); + env::var("RUST_LOG") + .ok() + .map_or_else(|| env::set_var("RUST_LOG", "info"), drop); pretty_env_logger::init(); if let Some(cmd) = args.cmd { @@ -116,8 +120,8 @@ fn new() -> Result<()> { "Create buffer file for the description at {}.", description.display() ); - fs::File::create(&description)?; - Command::new(&env::var("EDITOR").unwrap_or_else(|_| "vi".into())) + let _ = fs::File::create(&description)?; + let _ = Command::new(&env::var("EDITOR").unwrap_or_else(|_| "vi".into())) .arg(&description) .spawn()? .wait()?; @@ -131,7 +135,11 @@ fn new() -> Result<()> { title, status: Status::Open, id: Uuid::new_v1( - Timestamp::from_unix(Context::new(1), Utc::now().timestamp() as u64, 0), + Timestamp::from_unix( + Context::new(1), + Utc::now().timestamp().try_into()?, + 0, + ), &[0, 5, 2, 4, 9, 3], )?, assignees: Vec::new(), @@ -254,15 +262,15 @@ fn migrate() -> Result<()> { let open_tickets_path = open_tickets()?; let closed_tickets_path = closed_tickets()?; - for t in tickets.into_iter() { + for t in tickets { let ticket = Ticket { title: t.title, status: t.status, id: Uuid::new_v1( - Timestamp::from_unix(&ctx, Utc::now().timestamp() as u64, 0), + Timestamp::from_unix(&ctx, Utc::now().timestamp().try_into()?, 0), &[0, 5, 2, 4, 9, 3], )?, - assignees: t.assignee.map(|a| vec![a]).unwrap_or_else(Vec::new), + assignees: t.assignee.map_or_else(Vec::new, |a| vec![a]), description: t.description, comments: Vec::new(), version: Version::V1, @@ -289,7 +297,9 @@ fn migrate() -> Result<()> { Ok(()) } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +/// The fundamental type this tool revolves around. The ticket represents +/// everything about an issue or future plan for the code base. pub struct Ticket { title: String, status: Status, @@ -300,15 +310,21 @@ pub struct Ticket { version: Version, } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +/// Enum representing what version of the ticket it is and the assumptions that +/// can be made about it pub enum Version { + /// The first version V1, } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +/// Newtype to represent a User or maintainer pub struct User(String); -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +/// Original version of the tickets on disk. This exists for historical reasons +/// but is deprecated and likely to be removed. pub struct TicketV0 { title: String, status: Status, @@ -317,8 +333,11 @@ pub struct TicketV0 { description: String, } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +/// What is the current state of a ticket pub enum Status { + /// The ticket has been opened but the issue has not been resolved Open, + /// The ticket has a corresponding fix and has been closed Closed, } diff --git a/ticket/src/tui.rs b/ticket/src/tui.rs index e1aa666..01f00bf 100644 --- a/ticket/src/tui.rs +++ b/ticket/src/tui.rs @@ -130,8 +130,8 @@ pub struct Config { } impl Default for Config { - fn default() -> Config { - Config { + fn default() -> Self { + Self { exit_key: Key::Char('q'), tick_rate: Duration::from_millis(250), } @@ -139,11 +139,11 @@ impl Default for Config { } impl Events { - pub fn new() -> Events { - Events::with_config(Config::default()) + pub fn new() -> Self { + Self::with_config(Config::default()) } - pub fn with_config(config: Config) -> Events { + pub fn with_config(config: Config) -> Self { let (tx, rx) = mpsc::channel(); let input_handle = { let tx = tx.clone(); @@ -171,7 +171,7 @@ impl Events { } }) }; - Events { + Self { rx, input_handle, tick_handle, @@ -198,8 +198,8 @@ pub fn run() -> Result<()> { tabs: TabsState::new(vec!["Open", "Closed"]), tickets: { let mut map = BTreeMap::new(); - map.insert("Open".into(), get_open_tickets()?); - map.insert("Closed".into(), get_closed_tickets()?); + let _ = map.insert("Open".into(), get_open_tickets()?); + let _ = map.insert("Closed".into(), get_closed_tickets()?); TicketState::new(map) }, }; -- cgit v1.2.3