From 2086374dac24eda01cdf80cbd48817f02005914b Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Mon, 3 Jun 2024 20:53:15 +0100 Subject: [PATCH 1/9] touchups --- crates/node-core/src/args/database.rs | 37 +++++++++-- crates/storage/errors/src/db.rs | 93 ++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index 77a458cb3492..29ed9af163b2 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -2,14 +2,14 @@ use crate::version::default_client_version; use clap::Args; -use reth_storage_errors::db::LogLevel; +use reth_storage_errors::db::{LogLevel, LogLevelValueParser}; /// Parameters for database configuration #[derive(Debug, Args, PartialEq, Eq, Default, Clone, Copy)] #[command(next_help_heading = "Database")] pub struct DatabaseArgs { /// Database logging level. Levels higher than "notice" require a debug build. - #[arg(long = "db.log-level", value_enum)] + #[arg(long = "db.log-level", value_parser = LogLevelValueParser::default())] pub log_level: Option, /// Open environment in exclusive/monopolistic mode. Makes it possible to open a database on an /// NFS volume. @@ -25,7 +25,6 @@ impl DatabaseArgs { .with_exclusive(self.exclusive) } } - #[cfg(test)] mod tests { use super::*; @@ -39,9 +38,39 @@ mod tests { } #[test] - fn test_parse_database_args() { + fn test_default_database_args() { let default_args = DatabaseArgs::default(); let args = CommandParser::::parse_from(["reth"]).args; assert_eq!(args, default_args); } + + #[test] + fn test_possible_values() { + let possible_values: Vec<_> = + LogLevel::value_variants().iter().map(|v| format!("{:?}", v)).collect(); + let expected_values = + vec!["Fatal", "Error", "Warn", "Notice", "Verbose", "Debug", "Trace", "Extra"]; + assert_eq!(possible_values, expected_values); + } + + #[test] + fn test_command_parser_with_valid_log_level() { + let cmd = + CommandParser::::try_parse_from(["reth", "--db.log-level", "Debug"]) + .unwrap(); + assert_eq!(cmd.args.log_level, Some(LogLevel::Debug)); + } + + #[test] + fn test_command_parser_with_invalid_log_level() { + let result = + CommandParser::::try_parse_from(["reth", "--db.log-level", "invalid"]); + assert!(result.is_err()); + } + + #[test] + fn test_command_parser_without_log_level() { + let cmd = CommandParser::::try_parse_from(&["reth"]).unwrap(); + assert_eq!(cmd.args.log_level, None); + } } diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index 33f8b13c0b41..89baa7950292 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -1,4 +1,9 @@ -use std::fmt::Display; +use clap::{ + builder::{PossibleValue, TypedValueParser}, + error::ErrorKind, + Arg, Command, Error, +}; +use std::{fmt::Display, str::FromStr}; use thiserror::Error; /// Database error type. @@ -103,7 +108,6 @@ pub enum DatabaseWriteOperation { /// Database log level. #[derive(Debug, PartialEq, Eq, Clone, Copy)] -#[cfg_attr(feature = "clap", derive(clap::ValueEnum))] pub enum LogLevel { /// Enables logging for critical conditions, i.e. assertion failures. Fatal, @@ -122,3 +126,88 @@ pub enum LogLevel { /// Enables logging for extra debug-level messages. Extra, } + +impl LogLevel { + /// All possible variants of the `LogLevel` enum + pub const fn value_variants() -> &'static [Self] { + &[ + Self::Fatal, + Self::Error, + Self::Warn, + Self::Notice, + Self::Verbose, + Self::Debug, + Self::Trace, + Self::Extra, + ] + } + + /// Returns all variants descriptions + pub const fn help_message(&self) -> &'static str { + match self { + Self::Fatal => "Enables logging for critical conditions, i.e. assertion failures", + Self::Error => "Enables logging for error conditions", + Self::Warn => "Enables logging for warning conditions", + Self::Notice => "Enables logging for normal but significant conditions", + Self::Verbose => "Enables logging for verbose informational", + Self::Debug => "Enables logging for debug-level messages", + Self::Trace => "Enables logging for trace debug-level messages", + Self::Extra => "Enables logging for extra debug-level messages", + } + } +} + +impl FromStr for LogLevel { + type Err = String; + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "fatal" => Ok(Self::Fatal), + "error" => Ok(Self::Error), + "warn" => Ok(Self::Warn), + "notice" => Ok(Self::Notice), + "verbose" => Ok(Self::Verbose), + "debug" => Ok(Self::Debug), + "trace" => Ok(Self::Trace), + "extra" => Ok(Self::Extra), + _ => Err(format!("Invalid log level: {}", s)), + } + } +} + +/// clap value parser for [`LogLevel`]. +#[derive(Clone, Debug, Default)] +pub struct LogLevelValueParser; + +impl TypedValueParser for LogLevelValueParser { + type Value = LogLevel; + + fn parse_ref( + &self, + _cmd: &Command, + arg: Option<&Arg>, + value: &std::ffi::OsStr, + ) -> Result { + let val = + value.to_str().ok_or_else(|| Error::raw(ErrorKind::InvalidUtf8, "Invalid UTF-8"))?; + + val.parse::().map_err(|err| { + let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); + let possible_values = LogLevel::value_variants() + .iter() + .map(|v| format!("{:?}", v)) + .collect::>() + .join(", "); + let msg = format!( + "Invalid value '{val}' for {arg}: {err}.\n [possible values: {possible_values}]" + ); + clap::Error::raw(clap::error::ErrorKind::InvalidValue, msg) + }) + } + + fn possible_values(&self) -> Option + '_>> { + let values = + LogLevel::value_variants().iter().map(|v| PossibleValue::new(v.help_message())); + Some(Box::new(values)) + } +} From a1b7097b397487f59eb5cea41ab85dc57de6f74e Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Mon, 3 Jun 2024 23:10:51 +0100 Subject: [PATCH 2/9] move parser --- Cargo.lock | 1 - crates/node-core/Cargo.toml | 2 +- crates/node-core/src/args/database.rs | 47 +++++++++++++++++++++++++-- crates/storage/errors/Cargo.toml | 4 +-- crates/storage/errors/src/db.rs | 42 ------------------------ 5 files changed, 46 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7c36f044a64b..ab62dfdc154f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7825,7 +7825,6 @@ dependencies = [ name = "reth-storage-errors" version = "0.2.0-beta.8" dependencies = [ - "clap", "reth-fs-util", "reth-primitives", "thiserror", diff --git a/crates/node-core/Cargo.toml b/crates/node-core/Cargo.toml index a520f662f184..238451109fd2 100644 --- a/crates/node-core/Cargo.toml +++ b/crates/node-core/Cargo.toml @@ -15,7 +15,7 @@ workspace = true reth-primitives.workspace = true reth-fs-util.workspace = true reth-db = { workspace = true, features = ["mdbx"] } -reth-storage-errors = { workspace = true, features = ["clap"] } +reth-storage-errors.workspace = true reth-provider.workspace = true reth-network = { workspace = true, features = ["serde"] } reth-network-p2p.workspace = true diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index 29ed9af163b2..bf1fb066f30c 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -1,8 +1,12 @@ //! clap [Args](clap::Args) for database configuration use crate::version::default_client_version; -use clap::Args; -use reth_storage_errors::db::{LogLevel, LogLevelValueParser}; +use clap::{ + builder::{PossibleValue, TypedValueParser}, + error::ErrorKind, + Arg, Args, Command, Error, +}; +use reth_storage_errors::db::LogLevel; /// Parameters for database configuration #[derive(Debug, Args, PartialEq, Eq, Default, Clone, Copy)] @@ -25,6 +29,43 @@ impl DatabaseArgs { .with_exclusive(self.exclusive) } } + +/// clap value parser for [`LogLevel`]. +#[derive(Clone, Debug, Default)] +struct LogLevelValueParser; + +impl TypedValueParser for LogLevelValueParser { + type Value = LogLevel; + + fn parse_ref( + &self, + _cmd: &Command, + arg: Option<&Arg>, + value: &std::ffi::OsStr, + ) -> Result { + let val = + value.to_str().ok_or_else(|| Error::raw(ErrorKind::InvalidUtf8, "Invalid UTF-8"))?; + + val.parse::().map_err(|err| { + let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); + let possible_values = LogLevel::value_variants() + .iter() + .map(|v| format!("{:?}", v)) + .collect::>() + .join(", "); + let msg = format!( + "Invalid value '{val}' for {arg}: {err}.\n [possible values: {possible_values}]" + ); + clap::Error::raw(clap::error::ErrorKind::InvalidValue, msg) + }) + } + + fn possible_values(&self) -> Option + '_>> { + let values = + LogLevel::value_variants().iter().map(|v| PossibleValue::new(v.help_message())); + Some(Box::new(values)) + } +} #[cfg(test)] mod tests { use super::*; @@ -70,7 +111,7 @@ mod tests { #[test] fn test_command_parser_without_log_level() { - let cmd = CommandParser::::try_parse_from(&["reth"]).unwrap(); + let cmd = CommandParser::::try_parse_from(["reth"]).unwrap(); assert_eq!(cmd.args.log_level, None); } } diff --git a/crates/storage/errors/Cargo.toml b/crates/storage/errors/Cargo.toml index c1ce595ea927..5fa806345269 100644 --- a/crates/storage/errors/Cargo.toml +++ b/crates/storage/errors/Cargo.toml @@ -15,7 +15,5 @@ reth-primitives.workspace = true reth-fs-util.workspace = true thiserror.workspace = true -clap = { workspace = true, features = ["derive"], optional = true } -[features] -clap = ["dep:clap"] \ No newline at end of file + diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index 89baa7950292..f0d5d9b8d405 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -1,8 +1,3 @@ -use clap::{ - builder::{PossibleValue, TypedValueParser}, - error::ErrorKind, - Arg, Command, Error, -}; use std::{fmt::Display, str::FromStr}; use thiserror::Error; @@ -174,40 +169,3 @@ impl FromStr for LogLevel { } } } - -/// clap value parser for [`LogLevel`]. -#[derive(Clone, Debug, Default)] -pub struct LogLevelValueParser; - -impl TypedValueParser for LogLevelValueParser { - type Value = LogLevel; - - fn parse_ref( - &self, - _cmd: &Command, - arg: Option<&Arg>, - value: &std::ffi::OsStr, - ) -> Result { - let val = - value.to_str().ok_or_else(|| Error::raw(ErrorKind::InvalidUtf8, "Invalid UTF-8"))?; - - val.parse::().map_err(|err| { - let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); - let possible_values = LogLevel::value_variants() - .iter() - .map(|v| format!("{:?}", v)) - .collect::>() - .join(", "); - let msg = format!( - "Invalid value '{val}' for {arg}: {err}.\n [possible values: {possible_values}]" - ); - clap::Error::raw(clap::error::ErrorKind::InvalidValue, msg) - }) - } - - fn possible_values(&self) -> Option + '_>> { - let values = - LogLevel::value_variants().iter().map(|v| PossibleValue::new(v.help_message())); - Some(Box::new(values)) - } -} From 7ee0618b40ca5de91621069ea0c02c4f70532431 Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Mon, 3 Jun 2024 23:43:30 +0100 Subject: [PATCH 3/9] fix lint --- crates/node-core/src/args/database.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index bf1fb066f30c..d2249482e4bc 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -50,7 +50,7 @@ impl TypedValueParser for LogLevelValueParser { let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); let possible_values = LogLevel::value_variants() .iter() - .map(|v| format!("{:?}", v)) + .map(|v| format!("\"{}\"", v.help_message())) .collect::>() .join(", "); let msg = format!( From 1bdf24bea65d68d2404a8ae07ee031e07b35a5c8 Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Mon, 3 Jun 2024 23:47:04 +0100 Subject: [PATCH 4/9] fix lint2@ --- crates/node-core/src/args/database.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index d2249482e4bc..71c96c50b87d 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -50,11 +50,11 @@ impl TypedValueParser for LogLevelValueParser { let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); let possible_values = LogLevel::value_variants() .iter() - .map(|v| format!("\"{}\"", v.help_message())) + .map(|v| format!("- {}: {}", v, v.help_message())) .collect::>() - .join(", "); + .join("\n"); let msg = format!( - "Invalid value '{val}' for {arg}: {err}.\n [possible values: {possible_values}]" + "Invalid value '{val}' for {arg}: {err}.\n Possible values:\n{possible_values}" ); clap::Error::raw(clap::error::ErrorKind::InvalidValue, msg) }) From 7db42b4410d61c33c2111dd23d5a1a0497d98a07 Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Tue, 4 Jun 2024 00:01:32 +0100 Subject: [PATCH 5/9] bump --- crates/node-core/src/args/database.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index 71c96c50b87d..8c6c87253f87 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -50,7 +50,7 @@ impl TypedValueParser for LogLevelValueParser { let arg = arg.map(|a| a.to_string()).unwrap_or_else(|| "...".to_owned()); let possible_values = LogLevel::value_variants() .iter() - .map(|v| format!("- {}: {}", v, v.help_message())) + .map(|v| format!("- {:?}: {}", v, v.help_message())) .collect::>() .join("\n"); let msg = format!( From 7990c91945557c039695f5b0de81dc79417c1dbc Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Tue, 4 Jun 2024 21:14:18 +0100 Subject: [PATCH 6/9] nits --- crates/node-core/src/args/database.rs | 6 ++++-- crates/storage/errors/src/db.rs | 20 +++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index 8c6c87253f87..29c22d26af3c 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -32,6 +32,7 @@ impl DatabaseArgs { /// clap value parser for [`LogLevel`]. #[derive(Clone, Debug, Default)] +#[non_exhaustive] struct LogLevelValueParser; impl TypedValueParser for LogLevelValueParser { @@ -61,8 +62,9 @@ impl TypedValueParser for LogLevelValueParser { } fn possible_values(&self) -> Option + '_>> { - let values = - LogLevel::value_variants().iter().map(|v| PossibleValue::new(v.help_message())); + let values = LogLevel::value_variants() + .iter() + .map(|v| PossibleValue::new(v.variant_name()).help(v.help_message())); Some(Box::new(values)) } } diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index f0d5d9b8d405..54025de43fa0 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -1,4 +1,8 @@ -use std::{fmt::Display, str::FromStr}; +use reth_primitives::Log; +use std::{ + fmt::{self, Display, Formatter}, + str::FromStr, +}; use thiserror::Error; /// Database error type. @@ -137,6 +141,20 @@ impl LogLevel { ] } + /// Static str reference to `LogLevel` enum, required for `Clap::Builder::PossibleValue::new()` + pub const fn variant_name(&self) -> &'static str { + match self { + Self::Fatal => "fatal", + Self::Error => "error", + Self::Warn => "warn", + Self::Notice => "notice", + Self::Verbose => "verbose", + Self::Debug => "debug", + Self::Trace => "trace", + Self::Extra => "extra", + } + } + /// Returns all variants descriptions pub const fn help_message(&self) -> &'static str { match self { From 661b2e8b2c85b53c949eadfba05a348029f1fa10 Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Tue, 4 Jun 2024 21:20:13 +0100 Subject: [PATCH 7/9] nits --- crates/storage/errors/src/db.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index 54025de43fa0..61025fdf4dd2 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -1,8 +1,4 @@ -use reth_primitives::Log; -use std::{ - fmt::{self, Display, Formatter}, - str::FromStr, -}; +use std::str::FromStr; use thiserror::Error; /// Database error type. From 17b92e55c5b62905b81d093f516ac38975048f03 Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Tue, 4 Jun 2024 21:26:36 +0100 Subject: [PATCH 8/9] build before... --- crates/storage/errors/src/db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index 61025fdf4dd2..73d3451c3629 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -1,4 +1,4 @@ -use std::str::FromStr; +use std::{fmt::Display, str::FromStr}; use thiserror::Error; /// Database error type. From 637ba826e490e1c73bda60072c25c140e889427b Mon Sep 17 00:00:00 2001 From: "hawkinsjgh@gmail.com" Date: Tue, 4 Jun 2024 21:51:11 +0100 Subject: [PATCH 9/9] book test --- crates/node-core/src/args/database.rs | 31 ++++++++++++++++++++++----- crates/storage/errors/src/db.rs | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/crates/node-core/src/args/database.rs b/crates/node-core/src/args/database.rs index 29c22d26af3c..09e9de82f52d 100644 --- a/crates/node-core/src/args/database.rs +++ b/crates/node-core/src/args/database.rs @@ -89,11 +89,32 @@ mod tests { #[test] fn test_possible_values() { - let possible_values: Vec<_> = - LogLevel::value_variants().iter().map(|v| format!("{:?}", v)).collect(); - let expected_values = - vec!["Fatal", "Error", "Warn", "Notice", "Verbose", "Debug", "Trace", "Extra"]; - assert_eq!(possible_values, expected_values); + // Initialize the LogLevelValueParser + let parser = LogLevelValueParser; + + // Call the possible_values method + let possible_values: Vec = parser.possible_values().unwrap().collect(); + + // Expected possible values + let expected_values = vec![ + PossibleValue::new("fatal") + .help("Enables logging for critical conditions, i.e. assertion failures"), + PossibleValue::new("error").help("Enables logging for error conditions"), + PossibleValue::new("warn").help("Enables logging for warning conditions"), + PossibleValue::new("notice") + .help("Enables logging for normal but significant condition"), + PossibleValue::new("verbose").help("Enables logging for verbose informational"), + PossibleValue::new("debug").help("Enables logging for debug-level messages"), + PossibleValue::new("trace").help("Enables logging for trace debug-level messages"), + PossibleValue::new("extra").help("Enables logging for extra debug-level messages"), + ]; + + // Check that the possible values match the expected values + assert_eq!(possible_values.len(), expected_values.len()); + for (actual, expected) in possible_values.iter().zip(expected_values.iter()) { + assert_eq!(actual.get_name(), expected.get_name()); + assert_eq!(actual.get_help(), expected.get_help()); + } } #[test] diff --git a/crates/storage/errors/src/db.rs b/crates/storage/errors/src/db.rs index 73d3451c3629..b731e4d240dc 100644 --- a/crates/storage/errors/src/db.rs +++ b/crates/storage/errors/src/db.rs @@ -157,7 +157,7 @@ impl LogLevel { Self::Fatal => "Enables logging for critical conditions, i.e. assertion failures", Self::Error => "Enables logging for error conditions", Self::Warn => "Enables logging for warning conditions", - Self::Notice => "Enables logging for normal but significant conditions", + Self::Notice => "Enables logging for normal but significant condition", Self::Verbose => "Enables logging for verbose informational", Self::Debug => "Enables logging for debug-level messages", Self::Trace => "Enables logging for trace debug-level messages",