From 5cf17f6015e8fce5a8a0799c70e7a032b7f024ad Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Sun, 21 Jan 2024 05:47:13 -0500 Subject: other: flatten process config struct and clean up help text (#1395) * refactor: flatten process config field * other: clean up some doc formatting using indoc and breaklines * fix broken test * remove default as that breaks things for now * add test * more tests --- Cargo.lock | 13 +- Cargo.toml | 8 +- src/canvas/styling/colour_utils.rs | 32 +++++ src/options.rs | 7 +- src/options/args.rs | 260 +++++++++++++++++----------------- src/options/config/process_columns.rs | 29 ++-- tests/integration/arg_tests.rs | 2 +- 7 files changed, 183 insertions(+), 168 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cdd58724..c92a9d68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -170,6 +170,7 @@ dependencies = [ "hashbrown", "humantime", "indexmap", + "indoc", "itertools 0.12.0", "kstring", "libc", @@ -240,18 +241,18 @@ checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" [[package]] name = "clap" -version = "4.4.16" +version = "4.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "58e54881c004cec7895b0068a0a954cd5d62da01aef83fa35b1e594497bf5445" +checksum = "1e578d6ec4194633722ccf9544794b71b1385c3c027efe0c55db226fc880865c" dependencies = [ "clap_builder", ] [[package]] name = "clap_builder" -version = "4.4.16" +version = "4.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "59cb82d7f531603d2fd1f507441cdd35184fa81beff7bd489570de7f773460bb" +checksum = "4df4df40ec50c46000231c914968278b1eb05098cf8f1b3a518a95030e71d1c7" dependencies = [ "anstream", "anstyle", @@ -262,9 +263,9 @@ dependencies = [ [[package]] name = "clap_complete" -version = "4.4.6" +version = "4.4.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97aeaa95557bd02f23fbb662f981670c3d20c5a26e69f7354b28f57092437fcd" +checksum = "eaf7dcb7c21d8ca1a2482ee0f1d341f437c9a7af6ca6da359dc5e1b164e98215" dependencies = [ "clap", ] diff --git a/Cargo.toml b/Cargo.toml index af2cfd2d..b0007203 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,7 +75,7 @@ default = ["deploy"] anyhow = "1.0.79" backtrace = "0.3.69" cfg-if = "1.0.0" -clap = { version = "4.4.16", features = ["default", "cargo", "wrap_help"] } +clap = { version = "4.4.18", features = ["default", "cargo", "wrap_help"] } concat-string = "1.0.1" crossterm = "0.27.0" ctrlc = { version = "3.4.2", features = ["termination"] } @@ -85,6 +85,7 @@ fern = { version = "0.6.2", optional = true } hashbrown = "0.14.3" humantime = "2.1.0" indexmap = "2.1.0" +indoc = "2.0.4" itertools = "0.12.0" kstring = { version = "2.0.0", features = ["arc"] } log = { version = "0.4.20", optional = true } @@ -139,11 +140,12 @@ predicates = "3.0.4" portable-pty = "0.8.1" [build-dependencies] -clap = { version = "4.4.16", features = ["default", "cargo", "wrap_help"] } -clap_complete = "4.4.6" +clap = { version = "4.4.18", features = ["default", "cargo", "wrap_help"] } +clap_complete = "4.4.8" clap_complete_fig = "4.4.2" clap_complete_nushell = "4.4.2" clap_mangen = "0.2.17" +indoc = "2.0.4" [package.metadata.deb] section = "utility" diff --git a/src/canvas/styling/colour_utils.rs b/src/canvas/styling/colour_utils.rs index 5c39be74..1c1def3e 100644 --- a/src/canvas/styling/colour_utils.rs +++ b/src/canvas/styling/colour_utils.rs @@ -147,6 +147,21 @@ The following are supported strings: mod test { use super::*; + #[test] + fn general_str_to_colour() { + assert_eq!(str_to_colour("red").unwrap(), Color::Red); + assert!(str_to_colour("r ed").is_err()); + + assert_eq!(str_to_colour("#ffffff").unwrap(), Color::Rgb(255, 255, 255)); + assert!(str_to_colour("#fff fff").is_err()); + + assert_eq!( + str_to_colour("255, 255, 255").unwrap(), + Color::Rgb(255, 255, 255) + ); + assert!(str_to_colour("255, 256, 255").is_err()); + } + #[test] fn invalid_colour_names() { // Test invalid spacing in single word. @@ -265,4 +280,21 @@ mod test { assert!(convert_hex_to_color("#हिन्दी").is_err()); } + + #[test] + fn test_rgb_colours() { + assert_eq!( + convert_rgb_to_color("0, 0, 0").unwrap(), + Color::Rgb(0, 0, 0) + ); + assert_eq!( + convert_rgb_to_color("255, 255, 255").unwrap(), + Color::Rgb(255, 255, 255) + ); + assert!(convert_rgb_to_color("255, 256, 255").is_err()); + assert!(convert_rgb_to_color("256, 0, 256").is_err()); + assert!(convert_rgb_to_color("1, -1, 1").is_err()); + assert!(convert_rgb_to_color("1, -100000, 1").is_err()); + assert!(convert_rgb_to_color("1, -100000, 100000").is_err()); + } } diff --git a/src/options.rs b/src/options.rs index 092d982d..902c5c87 100644 --- a/src/options.rs +++ b/src/options.rs @@ -112,10 +112,7 @@ pub fn init_app( let network_use_binary_prefix = is_flag_enabled!(network_use_binary_prefix, matches, config); let proc_columns: Option> = { - let columns = config - .processes - .as_ref() - .and_then(|cfg| cfg.columns.clone()); + let columns = config.processes.as_ref().map(|cfg| cfg.columns.clone()); match columns { Some(columns) => { @@ -410,8 +407,6 @@ pub fn get_widget_layout( // Confirm that we have at least ONE widget left - if not, error out! if iter_id > 0 { ret_bottom_layout.get_movement_mappings(); - // debug!("Bottom layout: {ret_bottom_layout:#?}"); - ret_bottom_layout } else { return Err(BottomError::ConfigError( diff --git a/src/options/args.rs b/src/options/args.rs index a2182577..b410eba4 100644 --- a/src/options/args.rs +++ b/src/options/args.rs @@ -3,80 +3,12 @@ //! Note that you probably want to keep this as a single file so the build script doesn't //! trip all over itself. +// TODO: New sections are misaligned! See if we can get that fixed. + use std::cmp::Ordering; -use clap::{builder::PossibleValuesParser, *}; - -const DEFAULT_WIDGET_TYPE_STR: &str = { - #[cfg(feature = "battery")] - { - "\ -Sets which widget type to use as the default widget. -For the default layout, this defaults to the 'process' widget. -For a custom layout, it defaults to the first widget it sees. - -For example, suppose we have a layout that looks like: -+-------------------+-----------------------+ -| CPU (1) | CPU (2) | -+---------+---------+-------------+---------+ -| Process | CPU (3) | Temperature | CPU (4) | -+---------+---------+-------------+---------+ - -Setting '--default_widget_type Temp' will make the Temperature -widget selected by default. - -Supported widget names: -+--------------------------+ -| cpu | -+--------------------------+ -| mem, memory | -+--------------------------+ -| net, network | -+--------------------------+ -| proc, process, processes | -+--------------------------+ -| temp, temperature | -+--------------------------+ -| disk | -+--------------------------+ -| batt, battery | -+--------------------------+ -" - } - #[cfg(not(feature = "battery"))] - { - "\ -Sets which widget type to use as the default widget. -For the default layout, this defaults to the 'process' widget. -For a custom layout, it defaults to the first widget it sees. - -For example, suppose we have a layout that looks like: -+-------------------+-----------------------+ -| CPU (1) | CPU (2) | -+---------+---------+-------------+---------+ -| Process | CPU (3) | Temperature | CPU (4) | -+---------+---------+-------------+---------+ - -Setting '--default_widget_type Temp' will make the Temperature -widget selected by default. - -Supported widget names: -+--------------------------+ -| cpu | -+--------------------------+ -| mem, memory | -+--------------------------+ -| net, network | -+--------------------------+ -| proc, process, processes | -+--------------------------+ -| temp, temperature | -+--------------------------+ -| disk | -+--------------------------+ -" - } -}; +use clap::*; +use indoc::indoc; pub fn get_matches() -> ArgMatches { build_app().get_matches() @@ -175,16 +107,22 @@ fn general_args(cmd: Command) -> Command { .action(ArgAction::Set) .value_name("CONFIG PATH") .help("Sets the location of the config file.") - .long_help("Sets the location of the config file. Expects a config file in the TOML format. If it doesn't exist, one is created.") + .long_help( + "Sets the location of the config file. Expects a config file in the TOML format.\ + If it doesn't exist, one is created.", + ) .value_hint(ValueHint::AnyPath); let default_time_value = Arg::new("default_time_value") - .short('t') - .long("default_time_value") - .action(ArgAction::Set) - .value_name("TIME") - .help("Default time value for graphs.") - .long_help("Default time value for graphs. Takes a number in milliseconds or a human duration (e.g. 60s). The minimum time is 30s, and the default is 60s."); + .short('t') + .long("default_time_value") + .action(ArgAction::Set) + .value_name("TIME") + .help("Default time value for graphs.") + .long_help( + "Default time value for graphs. Takes a number in milliseconds or a human \ + duration (e.g. 60s). The minimum time is 30s, and the default is 60s.", + ); // TODO: Charts are broken in the manpage let default_widget_count = Arg::new("default_widget_count") @@ -193,39 +131,71 @@ fn general_args(cmd: Command) -> Command { .requires_all(["default_widget_type"]) .value_name("INT") .help("Sets the n'th selected widget type as the default.") - .long_help( - "\ -Sets the n'th selected widget type to use as the default widget. -Requires 'default_widget_type' to also be set, and defaults to 1. - -This reads from left to right, top to bottom. For example, suppose -we have a layout that looks like: -+-------------------+-----------------------+ -| CPU (1) | CPU (2) | -+---------+---------+-------------+---------+ -| Process | CPU (3) | Temperature | CPU (4) | -+---------+---------+-------------+---------+ - -And we set our default widget type to 'CPU'. If we set -'--default_widget_count 1', then it would use the CPU (1) as -the default widget. If we set '--default_widget_count 3', it would -use CPU (3) as the default instead. - ", - ); + .long_help(indoc! { + "Sets the n'th selected widget type to use as the default widget. + Requires 'default_widget_type' to also be set, and defaults to 1. + + This reads from left to right, top to bottom. For example, suppose + we have a layout that looks like: + +-------------------+-----------------------+ + | CPU (1) | CPU (2) | + +---------+---------+-------------+---------+ + | Process | CPU (3) | Temperature | CPU (4) | + +---------+---------+-------------+---------+ + + And we set our default widget type to 'CPU'. If we set + '--default_widget_count 1', then it would use the CPU (1) as + the default widget. If we set '--default_widget_count 3', it would + use CPU (3) as the default instead." + }); let default_widget_type = Arg::new("default_widget_type") .long("default_widget_type") .action(ArgAction::Set) .value_name("WIDGET TYPE") .help("Sets the default widget type, use --help for info.") - .long_help(DEFAULT_WIDGET_TYPE_STR); + .long_help(indoc!{ + "Sets which widget type to use as the default widget. For the default \ + layout, this defaults to the 'process' widget. For a custom layout, it defaults \ + to the first widget it sees. + + For example, suppose we have a layout that looks like: + +-------------------+-----------------------+ + | CPU (1) | CPU (2) | + +---------+---------+-------------+---------+ + | Process | CPU (3) | Temperature | CPU (4) | + +---------+---------+-------------+---------+ + + Setting '--default_widget_type Temp' will make the temperature widget selected by default." + }) + .value_parser([ + "cpu", + "mem", + "net", + "network", + "proc", + "process", + "processes", + "temp", + "temperature", + "disk", + #[cfg(not(feature = "battery"))] + "batt", + #[cfg(not(feature = "battery"))] + "battery", + ]); let expanded_on_startup = Arg::new("expanded_on_startup") .short('e') .long("expanded") .action(ArgAction::SetTrue) .help("Expand the default widget upon starting the app.") - .long_help("Expand the default widget upon starting the app. Same as pressing \"e\" inside the app. Use with \"default_widget_type\" and \"default_widget_count\" to select desired expanded widget. This flag has no effect in basic mode (--basic)."); + .long_help( + "Expand the default widget upon starting the app. \ + Same as pressing \"e\" inside the app. Use with \"default_widget_type\" \ + and \"default_widget_count\" to select the desired expanded widget. This \ + flag has no effect in basic mode (--basic).", + ); let rate = Arg::new("rate") .short('r') @@ -233,7 +203,11 @@ use CPU (3) as the default instead. .action(ArgAction::Set) .value_name("TIME") .help("Sets the data refresh rate.") - .long_help("Sets the data refresh rate. Takes a number in milliseconds or a human duration (e.g. 5s). The minimum is 250ms, and defaults to 1000ms. Smaller values may take more computer resources."); + .long_help( + "Sets the data refresh rate. Takes a number in milliseconds or a human\ + duration (e.g. 5s). The minimum is 250ms, and defaults to 1000ms. Smaller \ + values may take more computer resources.", + ); let time_delta = Arg::new("time_delta") .short('d') @@ -241,14 +215,23 @@ use CPU (3) as the default instead. .action(ArgAction::Set) .value_name("TIME") .help("The amount of time changed upon zooming.") - .long_help("The amount of time changed when zooming in/out. Takes a number in milliseconds or a human duration (e.g. 30s). The minimum is 1s, and defaults to 15s."); + .long_help( + "The amount of time changed when zooming in/out. Takes a number in \ + milliseconds or a human duration (e.g. 30s). The minimum is 1s, and \ + defaults to 15s.", + ); + // TODO: Unify how we do defaults. let retention = Arg::new("retention") .long("retention") .action(ArgAction::Set) .value_name("TIME") .help("The timespan of data stored.") - .long_help("How much data is stored at once in terms of time. Takes a number in milliseconds or a human duration (e.g. 20m), with a minimum of 1 minute. Note higher values will take up more memory. Defaults to 10 minutes."); + .long_help( + "How much data is stored at once in terms of time. Takes a number \ + in milliseconds or a human duration (e.g. 20m), with a minimum of 1 minute. \ + Note that higher values will take up more memory. Defaults to 10 minutes.", + ); cmd.args(args![ autohide_time, @@ -278,35 +261,28 @@ fn style_args(cmd: Command) -> Command { .long("color") .action(ArgAction::Set) .value_name("COLOR SCHEME") - .value_parser(PossibleValuesParser::new([ + .value_parser([ "default", "default-light", "gruvbox", "gruvbox-light", "nord", "nord-light", - ])) + ]) .hide_possible_values(true) - .help("Use a color scheme, use --help for info.") - .long_help( - "\ -Use a pre-defined color scheme. Currently supported values are: -+------------------------------------------------------------+ -| default | -+------------------------------------------------------------+ -| default-light (default but for use with light backgrounds) | -+------------------------------------------------------------+ -| gruvbox (a bright theme with 'retro groove' colors) | -+------------------------------------------------------------+ -| gruvbox-light (gruvbox but for use with light backgrounds) | -+------------------------------------------------------------+ -| nord (an arctic, north-bluish color palette) | -+------------------------------------------------------------+ -| nord-light (nord but for use with light backgrounds) | -+------------------------------------------------------------+ -Defaults to \"default\". - ", - ); + .help( + "Use a color scheme, use --help for info on the colors. \ + [possible values: default, default-light, gruvbox, gruvbox-light, nord, nord-light]", + ) + .long_help(indoc! { + "Use a pre-defined color scheme. Currently supported values are: + - default + - default-light (default but adjusted for lighter backgrounds) + - gruvbox (a bright theme with 'retro groove' colors) + - gruvbox-light (gruvbox but adjusted for lighter backgrounds) + - nord (an arctic, north-bluish color palette) + - nord-light (nord but adjusted for lighter backgrounds)" + }); cmd.arg(color) } @@ -358,14 +334,20 @@ fn process_args(cmd: Command) -> Command { .long("current_usage") .action(ArgAction::SetTrue) .help("Sets process CPU% to be based on current CPU%.") - .long_help("Sets process CPU% usage to be based on the current system CPU% usage rather than total CPU usage."); + .long_help( + "Sets process CPU% usage to be based on the current system CPU% usage rather \ + than total CPU usage.", + ); let unnormalized_cpu = Arg::new("unnormalized_cpu") .short('n') .long("unnormalized_cpu") .action(ArgAction::SetTrue) .help("Show process CPU% usage without normalizing over the number of cores.") - .long_help("Shows all process CPU% usage without averaging over the number of CPU cores in the system."); + .long_help( + "Shows all process CPU% usage without averaging over the number of CPU cores \ + in the system.", + ); let group_processes = Arg::new("group_processes") .short('g') @@ -391,7 +373,10 @@ fn process_args(cmd: Command) -> Command { .long("disable_advanced_kill") .action(ArgAction::SetTrue) .help("Hides advanced process killing.") - .long_help("Hides advanced options to stop a process on Unix-like systems. The only option shown is 15 (TERM)."); + .long_help( + "Hides advanced options to stop a process on Unix-like systems. The only \ + option shown is 15 (TERM).", + ); let whole_word = Arg::new("whole_word") .short('W') @@ -446,7 +431,10 @@ fn mem_args(cmd: Command) -> Command { .long("mem_as_value") .action(ArgAction::SetTrue) .help("Defaults to showing process memory usage by value.") - .long_help("Defaults to showing process memory usage by value. Otherwise, it defaults to showing it by percentage."); + .long_help( + "Defaults to showing process memory usage by value. Otherwise, it defaults \ + to showing it by percentage.", + ); #[cfg(not(target_os = "windows"))] { @@ -466,11 +454,15 @@ fn mem_args(cmd: Command) -> Command { fn network_args(cmd: Command) -> Command { let cmd = cmd.next_help_heading("Network Options"); + // TODO: Change this to be configured as network graph type? let use_old_network_legend = Arg::new("use_old_network_legend") .long("use_old_network_legend") .action(ArgAction::SetTrue) .help("DEPRECATED - uses a separate network legend.") - .long_help("DEPRECATED - uses an older (pre-0.4), separate network widget legend. This display is not tested anymore and may be broken."); + .long_help( + "DEPRECATED - uses an older (pre-0.4), separate network widget legend. This \ + display is not tested anymore and may be broken.", + ); let network_use_bytes = Arg::new("network_use_bytes") .long("network_use_bytes") @@ -488,7 +480,10 @@ fn network_args(cmd: Command) -> Command { .long("network_use_binary_prefix") .action(ArgAction::SetTrue) .help("Displays the network widget with binary prefixes.") - .long_help("Displays the network widget with binary prefixes (i.e. kibibits, mebibits) rather than a decimal prefix (i.e. kilobits, megabits). Defaults to decimal prefixes."); + .long_help( + "Displays the network widget with binary prefixes (i.e. kibibits, mebibits) \ + rather than a decimal prefix (i.e. kilobits, megabits). Defaults to decimal prefixes.", + ); cmd.args(args![ use_old_network_legend, @@ -595,7 +590,8 @@ mod test { assert!( !help_str.to_string().contains("\nOptions:\n"), - "the default 'Options' heading should not exist; if it does then an argument is missing a help heading." + "the default 'Options' heading should not exist; if it does then an argument is \ + missing a help heading." ); } } diff --git a/src/options/config/process_columns.rs b/src/options/config/process_columns.rs index 504faaf3..ea65bf27 100644 --- a/src/options/config/process_columns.rs +++ b/src/options/config/process_columns.rs @@ -5,7 +5,8 @@ use crate::widgets::ProcWidgetColumn; /// Process column settings. #[derive(Clone, Debug, Default, Deserialize)] pub struct ProcessConfig { - pub columns: Option>, + #[serde(default)] + pub columns: Vec, } #[cfg(test)] @@ -17,7 +18,7 @@ mod test { fn empty_column_setting() { let config = ""; let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); - assert!(generated.columns.is_none()); + assert!(generated.columns.is_empty()); } #[test] @@ -29,7 +30,7 @@ mod test { let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); assert_eq!( generated.columns, - Some(vec![ + vec![ ProcWidgetColumn::Cpu, ProcWidgetColumn::PidOrCount, ProcWidgetColumn::User, @@ -41,7 +42,7 @@ mod test { ProcWidgetColumn::Time, ProcWidgetColumn::User, ProcWidgetColumn::State, - ]), + ], ); } @@ -55,30 +56,18 @@ mod test { fn process_column_settings_3() { let config = r#"columns = ["Twrite", "T.Write"]"#; let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); - assert_eq!( - generated.columns, - Some(vec![ProcWidgetColumn::TotalWrite; 2]) - ); + assert_eq!(generated.columns, vec![ProcWidgetColumn::TotalWrite; 2]); let config = r#"columns = ["Tread", "T.read"]"#; let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); - assert_eq!( - generated.columns, - Some(vec![ProcWidgetColumn::TotalRead; 2]) - ); + assert_eq!(generated.columns, vec![ProcWidgetColumn::TotalRead; 2]); let config = r#"columns = ["read", "rps", "r/s"]"#; let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); - assert_eq!( - generated.columns, - Some(vec![ProcWidgetColumn::ReadPerSecond; 3]) - ); + assert_eq!(generated.columns, vec![ProcWidgetColumn::ReadPerSecond; 3]); let config = r#"columns = ["write", "wps", "w/s"]"#; let generated: ProcessConfig = toml_edit::de::from_str(config).unwrap(); - assert_eq!( - generated.columns, - Some(vec![ProcWidgetColumn::WritePerSecond; 3]) - ); + assert_eq!(generated.columns, vec![ProcWidgetColumn::WritePerSecond; 3]); } } diff --git a/tests/integration/arg_tests.rs b/tests/integration/arg_tests.rs index 13ebc5f0..5bb8a6bb 100644 --- a/tests/integration/arg_tests.rs +++ b/tests/integration/arg_tests.rs @@ -109,7 +109,7 @@ fn test_invalid_default_widget_1() { .arg("fake_widget") .assert() .failure() - .stderr(predicate::str::contains("invalid widget name")); + .stderr(predicate::str::contains("invalid value")); } #[test] -- cgit v1.2.3