From 69e4790b00b445ad4c39e67de7ce9021ee3bf28c Mon Sep 17 00:00:00 2001 From: alex-tdrn Date: Sun, 30 Jun 2024 14:38:41 +0200 Subject: [PATCH] Skip decoration lines for `detect columns --guess` (#13274) # Description I introduced a regression in #13272 that resulted in `detect columns --guess` to panic whenever it had to handle empty, whitespace-only, or non-whitespace-only lines that go all the way to the last column (and as such, cannot be considered to be lines that only have entries for the first colum). I fix this by detecting these cases and skipping them, since these are usually decoration lines. An example is the second line output by `winget list`: ![image](https://github.com/nushell/nushell/assets/20356389/06c873fb-0a26-45dd-b020-3bcc737d027f) What we don't want to skip, however, is lines that contain no whitespace, and fit into the detected first column, since these lines represent cases where data is only available for the first column, and are not just decoration lines. For example (made up example, there are no such entries in `winget lits`'s output), in this output we would not want to skip the `Docker Desktop` line : ``` Name Id Version Available Source ------------------------------------------------------------------------------------------------------------------------------------- AMD Software ARPMachineX64AMD Catalyst Install Manager 24.4.1 AMD Ryzen Master ARPMachineX64AMD Ryzen Master 2.13.0.2908 Docker Desktop Mozilla Firefox (x64 en-US) Mozilla.Firefox 127.0.2 winget ``` ![image](https://github.com/nushell/nushell/assets/20356389/12e31995-a7c1-4759-8c62-fb4fb199fd2e) NOTE: `winget list | detect columns --guess` does not panic, but sadly still does not work as expected. I believe this is not a nushell issue anymore, but a `winget` one. When being piped, `winget` seems to add extra whitespace and random `\r` symbols at the beginning of the text. This messes with the column detection, of course. ![image](https://github.com/nushell/nushell/assets/20356389/7d1b7e5f-17d0-41c8-8d2f-7896e0d73d66) ![image](https://github.com/nushell/nushell/assets/20356389/56917954-1231-43e7-bacf-e5760e263054) ![image](https://github.com/nushell/nushell/assets/20356389/630bcfc9-eb78-4a45-9c8f-97efc0c224f4) # User-Facing Changes `detect columns --guess` should not panic when receiving output from `winget list` at all anymore. A breaking change is the skipping of decoration lines, especially since scripts probably were doing something like `winget list | lines | reject 1 | str join "\n" | detect columns --guess`. This will now cause them to reject a line with valid data. # Tests + Formatting Added tests that exercise these edge cases, as well as a single-column test to make sure that trivial cases keep working. # After Submitting --- crates/nu-command/src/strings/guess_width.rs | 133 ++++++++++++++++++- 1 file changed, 132 insertions(+), 1 deletion(-) diff --git a/crates/nu-command/src/strings/guess_width.rs b/crates/nu-command/src/strings/guess_width.rs index b214b21bcc..8c758516bd 100644 --- a/crates/nu-command/src/strings/guess_width.rs +++ b/crates/nu-command/src/strings/guess_width.rs @@ -72,7 +72,9 @@ impl GuessWidth { let mut rows = Vec::new(); while let Ok(columns) = self.read() { - rows.push(columns); + if !columns.is_empty() { + rows.push(columns); + } } rows } @@ -180,6 +182,19 @@ fn split(line: &str, pos: &[usize], trim_space: bool) -> Vec { let (line_char_boundaries, line_chars): (Vec, Vec) = line.char_indices().unzip(); let mut w = 0; + if line_chars.is_empty() || line_chars.iter().all(|&c| c.is_whitespace()) { + // current line is completely empty, or only filled with whitespace + return Vec::new(); + } else if !pos.is_empty() + && line_chars.iter().all(|&c| !c.is_whitespace()) + && pos[0] < UnicodeWidthStr::width(line) + { + // we have more than 1 column in the input, but the current line has no whitespace, + // and it is longer than the first detected column separation position + // this indicates some kind of decoration line. let's skip it + return Vec::new(); + } + for p in 0..line_char_boundaries.len() { if pos.is_empty() || n > pos.len() - 1 { start_char = p; @@ -463,6 +478,122 @@ Ștefan Țincu "; assert_eq!(got, want); } + #[test] + fn test_guess_width_single_column() { + let input = "A + +B + +C"; + + let r = Box::new(std::io::BufReader::new(input.as_bytes())) as Box; + let reader = std::io::BufReader::new(r); + + let mut guess_width = GuessWidth { + reader, + pos: Vec::new(), + pre_lines: Vec::new(), + pre_count: 0, + limit_split: 0, + }; + + let want = vec![vec!["A"], vec!["B"], vec!["C"]]; + let got = guess_width.read_all(); + assert_eq!(got, want); + } + + #[test] + fn test_guess_width_row_without_whitespace() { + let input = "A B C D +------- +E F G H"; + + let r = Box::new(std::io::BufReader::new(input.as_bytes())) as Box; + let reader = std::io::BufReader::new(r); + + let mut guess_width = GuessWidth { + reader, + pos: Vec::new(), + pre_lines: Vec::new(), + pre_count: 0, + limit_split: 0, + }; + + let want = vec![vec!["A", "B", "C", "D"], vec!["E", "F", "G", "H"]]; + let got = guess_width.read_all(); + assert_eq!(got, want); + } + + #[test] + fn test_guess_width_row_with_single_column() { + let input = "A B C D +E +F G H I"; + + let r = Box::new(std::io::BufReader::new(input.as_bytes())) as Box; + let reader = std::io::BufReader::new(r); + + let mut guess_width = GuessWidth { + reader, + pos: Vec::new(), + pre_lines: Vec::new(), + pre_count: 0, + limit_split: 0, + }; + + let want = vec![ + vec!["A", "B", "C", "D"], + vec!["E"], + vec!["F", "G", "H", "I"], + ]; + let got = guess_width.read_all(); + assert_eq!(got, want); + } + + #[test] + fn test_guess_width_empty_row() { + let input = "A B C D + +E F G H"; + + let r = Box::new(std::io::BufReader::new(input.as_bytes())) as Box; + let reader = std::io::BufReader::new(r); + + let mut guess_width = GuessWidth { + reader, + pos: Vec::new(), + pre_lines: Vec::new(), + pre_count: 0, + limit_split: 0, + }; + + let want = vec![vec!["A", "B", "C", "D"], vec!["E", "F", "G", "H"]]; + let got = guess_width.read_all(); + assert_eq!(got, want); + } + + #[test] + fn test_guess_width_row_with_only_whitespace() { + let input = "A B C D + +E F G H"; + + let r = Box::new(std::io::BufReader::new(input.as_bytes())) as Box; + let reader = std::io::BufReader::new(r); + + let mut guess_width = GuessWidth { + reader, + pos: Vec::new(), + pre_lines: Vec::new(), + pre_count: 0, + limit_split: 0, + }; + + let want = vec![vec!["A", "B", "C", "D"], vec!["E", "F", "G", "H"]]; + let got = guess_width.read_all(); + assert_eq!(got, want); + } + #[test] fn test_to_table() { let lines = vec![