From 2aa5c2c41f816cc828de2545b669ffa158549d64 Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Sun, 26 Feb 2023 12:23:30 -0800 Subject: [PATCH] Simplify `str trim` command (#8205) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? This change removes 3 flags (`--all`, `--both`, and `--format`) from `str trim`. This is a net reduction of ~450 LoC and `str trim` no longer depends on `fancy_regex`. ### Why? I found these flags to be quite confusing when reviewing `str trim` earlier today: 1. `--all` removes characters even if they're in the centre of the the string. - This is arguably not "trimming"! In all programming languages I'm familiar with, trimming only affects the start and end of a string. - If someone needs to do this, `str replace` is more natural IMO 2. `--both` trims from the left and right - Confusing and unnecessary given that this is also the default behaviour 3. `--format` replaces multiple spaces with a single space, even in the centre of the string - Again, I don't think this falls under the scope of "trimming". IMO `str replace` is a more natural fit I believe that `str trim` is simpler and easier to understand after this change. ### Before ``` 〉help str trim Trim whitespace or specific character Search terms: whitespace, strip, lstrip, rstrip Usage: > str trim {flags} ...(rest) Flags: -h, --help - Display the help message for this command -c, --char - character to trim (default: whitespace) -l, --left - trims characters only from the beginning of the string -r, --right - trims characters only from the end of the string -a, --all - trims all characters from both sides of the string *and* in the middle -b, --both - trims all characters from left and right side of the string -f, --format - trims spaces replacing multiple characters with singles in the middle ``` ### After ``` 〉help str trim Trim whitespace or specific character Search terms: whitespace, strip, lstrip, rstrip Usage: > str trim {flags} ...(rest) Flags: -h, --help - Display the help message for this command -c, --char - character to trim (default: whitespace) -l, --left - trims characters only from the beginning of the string -r, --right - trims characters only from the end of the string ``` --- .../nu-command/src/strings/str_/trim/trim_.rs | 540 ++---------------- 1 file changed, 42 insertions(+), 498 deletions(-) diff --git a/crates/nu-command/src/strings/str_/trim/trim_.rs b/crates/nu-command/src/strings/str_/trim/trim_.rs index ea36f29d5..627056fb3 100644 --- a/crates/nu-command/src/strings/str_/trim/trim_.rs +++ b/crates/nu-command/src/strings/str_/trim/trim_.rs @@ -1,5 +1,4 @@ use crate::input_handler::{operate, CmdArgument}; -use fancy_regex::Regex; use nu_engine::CallExt; use nu_protocol::{ ast::{Call, CellPath}, @@ -12,7 +11,7 @@ pub struct SubCommand; struct Arguments { to_trim: Option, - closure_flags: ClosureFlags, + trim_side: TrimSide, cell_paths: Option>, mode: ActionMode, } @@ -23,13 +22,10 @@ impl CmdArgument for Arguments { } } -#[derive(Default, Debug, Copy, Clone)] -pub struct ClosureFlags { - all_flag: bool, - left_trim: bool, - right_trim: bool, - format_flag: bool, - both_flag: bool, +pub enum TrimSide { + Left, + Right, + Both, } impl Command for SubCommand { @@ -62,21 +58,6 @@ impl Command for SubCommand { "trims characters only from the end of the string", Some('r'), ) - .switch( - "all", - "trims all characters from both sides of the string *and* in the middle", - Some('a'), - ) - .switch( - "both", - "trims all characters from left and right side of the string", - Some('b'), - ) - .switch( - "format", - "trims spaces replacing multiple characters with singles in the middle", - Some('f'), - ) } fn usage(&self) -> &str { "Trim whitespace or specific character" @@ -115,19 +96,19 @@ impl Command for SubCommand { None => ActionMode::Global, Some(_) => ActionMode::Local, }; + + let left = call.has_flag("left"); + let right = call.has_flag("right"); + let trim_side = match (left, right) { + (true, true) => TrimSide::Both, + (true, false) => TrimSide::Left, + (false, true) => TrimSide::Right, + (false, false) => TrimSide::Both, + }; + let args = Arguments { to_trim, - closure_flags: ClosureFlags { - all_flag: call.has_flag("all"), - left_trim: call.has_flag("left"), - right_trim: call.has_flag("right"), - format_flag: call.has_flag("format"), - both_flag: call.has_flag("both") - || (!call.has_flag("all") - && !call.has_flag("left") - && !call.has_flag("right") - && !call.has_flag("format")), // this is the case if no flags are provided - }, + trim_side, cell_paths, mode, }; @@ -146,11 +127,6 @@ impl Command for SubCommand { example: "'=== Nu shell ===' | str trim -c '=' | str trim", result: Some(Value::test_string("Nu shell")), }, - Example { - description: "Trim all characters", - example: "' Nu shell ' | str trim -a", - result: Some(Value::test_string("Nushell")), - }, Example { description: "Trim whitespace from the beginning of string", example: "' Nu shell ' | str trim -l", @@ -183,11 +159,11 @@ pub enum ActionMode { fn action(input: &Value, arg: &Arguments, head: Span) -> Value { let char_ = arg.to_trim; - let closure_flags = &arg.closure_flags; + let trim_side = &arg.trim_side; let mode = &arg.mode; match input { Value::String { val: s, .. } => Value::String { - val: trim(s, char_, closure_flags), + val: trim(s, char_, trim_side), span: head, }, // Propagate errors by explicitly matching them before the final case. @@ -228,14 +204,7 @@ fn action(input: &Value, arg: &Arguments, head: Span) -> Value { } } -fn trim(s: &str, char_: Option, closure_flags: &ClosureFlags) -> String { - let ClosureFlags { - left_trim, - right_trim, - all_flag, - both_flag, - format_flag, - } = closure_flags; +fn trim(s: &str, char_: Option, trim_side: &TrimSide) -> String { let delimiters = match char_ { Some(c) => vec![c], // Trying to make this trim work like rust default trim() @@ -250,47 +219,10 @@ fn trim(s: &str, char_: Option, closure_flags: &ClosureFlags) -> String { ], //whitespace }; - if *left_trim { - s.trim_start_matches(&delimiters[..]).to_string() - } else if *right_trim { - s.trim_end_matches(&delimiters[..]).to_string() - } else if *all_flag { - s.split(&delimiters[..]) - .filter(|s| !s.is_empty()) - .collect::() - } else if *both_flag { - s.trim_matches(&delimiters[..]).to_string() - } else if *format_flag { - // The idea here is to use regex to go through these delimiters and - // where there are multiple, replace them with singles - - // create our return string which is a copy of the original string - let mut return_string = String::from(s); - // Iterate through the delimiters replacing them with regex friendly names - for r in &delimiters { - let reg = match r { - ' ' => r"\s".to_string(), - '\x09' => r"\t".to_string(), - '\x0A' => r"\n".to_string(), - '\x0B' => r"\v".to_string(), - '\x0C' => r"\f".to_string(), - '\x0D' => r"\r".to_string(), - _ => format!(r"\{r}"), - }; - // create a regex string that looks for 2 or more of each of these characters - let re_str = format!("{reg}{{2,}}"); - // create the regex - let re = Regex::new(&re_str).expect("Error creating regular expression"); - // replace all multiple occurrences with single occurrences represented by r - let new_str = re.replace_all(&return_string, r.to_string()); - // update the return string so the next loop has the latest changes - return_string = new_str.to_string(); - } - // for good measure, trim_matches, which gets the start and end - // theoretically we shouldn't have to do this but from my testing, we do. - return_string.trim_matches(&delimiters[..]).to_string() - } else { - s.trim().to_string() + match trim_side { + TrimSide::Left => s.trim_start_matches(&delimiters[..]).to_string(), + TrimSide::Right => s.trim_end_matches(&delimiters[..]).to_string(), + TrimSide::Both => s.trim_matches(&delimiters[..]).to_string(), } } @@ -331,14 +263,10 @@ mod tests { fn trims() { let word = Value::test_string("andres "); let expected = Value::test_string("andres"); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Local, }; @@ -350,13 +278,9 @@ mod tests { fn trims_global() { let word = Value::test_string(" global "); let expected = Value::test_string("global"); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Global, }; @@ -368,13 +292,9 @@ mod tests { fn global_trim_ignores_numbers() { let number = Value::test_int(2020); let expected = Value::test_int(2020); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Global, }; @@ -388,14 +308,10 @@ mod tests { let row = make_record(vec!["a", "b"], vec![" c ", " d "]); // ["a".to_string() => string(" c "), " b ".to_string() => string(" d ")]; let expected = make_record(vec!["a", "b"], vec!["c", "d"]); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Global, }; @@ -407,14 +323,10 @@ mod tests { fn global_trim_table() { let row = make_list(vec![" a ", "d"]); let expected = make_list(vec!["a", "d"]); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Global, }; @@ -426,179 +338,25 @@ mod tests { fn trims_custom_character_both_ends() { let word = Value::test_string("!#andres#!"); let expected = Value::test_string("#andres#"); - let closure_flags = ClosureFlags { - both_flag: true, - ..Default::default() - }; let args = Arguments { to_trim: Some('!'), - closure_flags, + trim_side: TrimSide::Both, cell_paths: None, mode: ActionMode::Local, }; let actual = action(&word, &args, Span::test_data()); assert_eq!(actual, expected); } - #[test] - fn trims_all_white_space() { - let word = Value::test_string(" Value1 a lot of spaces "); - let expected = Value::test_string("Value1alotofspaces"); - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some(' '), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trims_row_all_white_space() { - let row = make_record( - vec!["a", "b"], - vec![" nu shell ", " b c d e "], - ); - let expected = make_record(vec!["a", "b"], vec!["nushell", "bcde"]); - - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - let args = Arguments { - to_trim: None, - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trims_table_all_white_space() { - let row = Value::List { - vals: vec![ - Value::test_string(" nu shell "), - Value::test_int(65), - Value::test_string(" d"), - ], - span: Span::test_data(), - }; - let expected = Value::List { - vals: vec![ - Value::test_string("nushell"), - Value::test_int(65), - Value::test_string("d"), - ], - span: Span::test_data(), - }; - - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: None, - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn trims_all_custom_character() { - let word = Value::test_string(".Value1.a.lot..of...dots."); - let expected = Value::test_string("Value1alotofdots"); - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some('.'), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trims_row_all_custom_character() { - let row = make_record(vec!["a", "b"], vec!["!!!!nu!!shell!!!", "!!b!c!!d!e!!"]); - let expected = make_record(vec!["a", "b"], vec!["nushell", "bcde"]); - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some('!'), - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trims_table_all_custom_character() { - let row = Value::List { - vals: vec![ - Value::test_string("##nu####shell##"), - Value::test_int(65), - Value::test_string("#d"), - ], - span: Span::test_data(), - }; - let expected = Value::List { - vals: vec![ - Value::test_string("nushell"), - Value::test_int(65), - Value::test_string("d"), - ], - span: Span::test_data(), - }; - - let closure_flags = ClosureFlags { - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some('#'), - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } #[test] fn trims_whitespace_from_left() { let word = Value::test_string(" andres "); let expected = Value::test_string("andres "); - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Local, }; @@ -610,14 +368,10 @@ mod tests { fn global_trim_left_ignores_numbers() { let number = Value::test_int(2020); let expected = Value::test_int(2020); - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Global, }; @@ -629,14 +383,10 @@ mod tests { fn trims_left_global() { let word = Value::test_string(" global "); let expected = Value::test_string("global "); - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Global, }; @@ -648,14 +398,10 @@ mod tests { fn global_trim_left_row() { let row = make_record(vec!["a", "b"], vec![" c ", " d "]); let expected = make_record(vec!["a", "b"], vec!["c ", "d "]); - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Global, }; @@ -682,13 +428,9 @@ mod tests { span: Span::test_data(), }; - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Global, }; @@ -700,14 +442,10 @@ mod tests { fn trims_custom_chars_from_left() { let word = Value::test_string("!!! andres !!!"); let expected = Value::test_string(" andres !!!"); - let closure_flags = ClosureFlags { - left_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: Some('!'), - closure_flags, + trim_side: TrimSide::Left, cell_paths: None, mode: ActionMode::Local, }; @@ -718,13 +456,10 @@ mod tests { fn trims_whitespace_from_right() { let word = Value::test_string(" andres "); let expected = Value::test_string(" andres"); - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; + let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Local, }; @@ -736,13 +471,9 @@ mod tests { fn trims_right_global() { let word = Value::test_string(" global "); let expected = Value::test_string(" global"); - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Global, }; @@ -754,14 +485,9 @@ mod tests { fn global_trim_right_ignores_numbers() { let number = Value::test_int(2020); let expected = Value::test_int(2020); - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; - let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Global, }; @@ -773,14 +499,9 @@ mod tests { fn global_trim_right_row() { let row = make_record(vec!["a", "b"], vec![" c ", " d "]); let expected = make_record(vec!["a", "b"], vec![" c", " d"]); - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; - let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Global, }; @@ -806,15 +527,9 @@ mod tests { ], span: Span::test_data(), }; - - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; - let args = Arguments { to_trim: None, - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Global, }; @@ -826,185 +541,14 @@ mod tests { fn trims_custom_chars_from_right() { let word = Value::test_string("#@! andres !@#"); let expected = Value::test_string("#@! andres !@"); - let closure_flags = ClosureFlags { - right_trim: true, - ..Default::default() - }; let args = Arguments { to_trim: Some('#'), - closure_flags, + trim_side: TrimSide::Right, cell_paths: None, mode: ActionMode::Local, }; let actual = action(&word, &args, Span::test_data()); assert_eq!(actual, expected); } - - #[test] - fn trims_whitespace_format_flag() { - let word = Value::test_string(" nushell is great "); - let expected = Value::test_string("nushell is great"); - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some(' '), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn trims_format_flag_global() { - let word = Value::test_string("global "); - let expected = Value::test_string("global"); - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - let args = Arguments { - to_trim: Some(' '), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - #[test] - fn global_trim_format_flag_ignores_numbers() { - let number = Value::test_int(2020); - let expected = Value::test_int(2020); - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: None, - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&number, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trim_format_flag_row() { - let row = make_record(vec!["a", "b"], vec![" c ", " b c d e "]); - let expected = make_record(vec!["a", "b"], vec!["c", "b c d e"]); - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: None, - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn global_trim_format_flag_table() { - let row = Value::List { - vals: vec![ - Value::test_string(" a b c d "), - Value::test_int(65), - Value::test_string(" b c d e f"), - ], - span: Span::test_data(), - }; - let expected = Value::List { - vals: vec![ - Value::test_string("a b c d"), - Value::test_int(65), - Value::test_string("b c d e f"), - ], - span: Span::test_data(), - }; - - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: None, - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&row, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn trims_custom_chars_format_flag() { - let word = Value::test_string(".Value1.a..lot...of....dots."); - let expected = Value::test_string("Value1.a.lot.of.dots"); - let closure_flags = ClosureFlags { - format_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some('.'), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn trims_all_format_flag_whitespace() { - let word = Value::test_string(" nushell is great "); - let expected = Value::test_string("nushellisgreat"); - let closure_flags = ClosureFlags { - format_flag: true, - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some(' '), - closure_flags, - cell_paths: None, - mode: ActionMode::Local, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } - - #[test] - fn trims_all_format_flag_global() { - let word = Value::test_string(" nushell is great "); - let expected = Value::test_string("nushellisgreat"); - let closure_flags = ClosureFlags { - format_flag: true, - all_flag: true, - ..Default::default() - }; - - let args = Arguments { - to_trim: Some(' '), - closure_flags, - cell_paths: None, - mode: ActionMode::Global, - }; - let actual = action(&word, &args, Span::test_data()); - assert_eq!(actual, expected); - } }