From e389e51b2bbc5d2c549a8faefbf190e204c97393 Mon Sep 17 00:00:00 2001 From: Artemiy Date: Wed, 22 Feb 2023 19:18:33 +0300 Subject: [PATCH] Display empty records and lists (#7925) # Description Fix some issues related to #7444 1. Empty lists and records are now displayed as a small notice in a box: ![image](https://user-images.githubusercontent.com/17511668/215832023-3f8d743a-2899-416f-9109-7876ad2bbedf.png) ![image](https://user-images.githubusercontent.com/17511668/215832273-c737b8a4-af33-4c16-8dd3-bd4f0fd19b5a.png) 2. Empty records are now correctly displayed if inside of another record list or table: ![image](https://user-images.githubusercontent.com/17511668/215832597-00f0cebc-a3b6-4ce8-8373-a9340d4c7020.png) ![image](https://user-images.githubusercontent.com/17511668/215832540-ab0e2a14-b8f6-4f47-976c-42003b622ef6.png) 3. Fixed inconsistent coloring of empty list placeholder inside of lists/tables: ![image](https://user-images.githubusercontent.com/17511668/215832924-813ffe17-e04e-4301-97c3-1bdbccf1825c.png) ![image](https://user-images.githubusercontent.com/17511668/215832963-4765c4cf-3036-4bcc-81e1-ced941fa47cb.png) # User-Facing Changes `table` command now displays empty records and lists like a table with text and correctly displays empty records inside tables and lists. New behavior of displaying empty lists and records can be disabled using `table.show_empty` config option. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-color-config/src/style_computer.rs | 2 +- crates/nu-command/src/viewers/table.rs | 310 ++++++++++++------ crates/nu-command/tests/commands/find.rs | 4 +- .../nu-command/tests/commands/path/split.rs | 4 +- .../nu-command/tests/commands/run_external.rs | 2 +- crates/nu-command/tests/commands/table.rs | 12 +- crates/nu-protocol/src/config.rs | 5 + crates/nu-protocol/src/ty.rs | 29 ++ .../src/sample_config/default_config.nu | 1 + tests/shell/pipeline/commands/internal.rs | 4 +- 10 files changed, 256 insertions(+), 117 deletions(-) diff --git a/crates/nu-color-config/src/style_computer.rs b/crates/nu-color-config/src/style_computer.rs index c78b72631..a40fac789 100644 --- a/crates/nu-color-config/src/style_computer.rs +++ b/crates/nu-color-config/src/style_computer.rs @@ -118,7 +118,7 @@ impl<'a> StyleComputer<'a> { // Used only by the `table` command. pub fn style_primitive(&self, value: &Value) -> TextStyle { - let s = self.compute(&value.get_type().to_string(), value); + let s = self.compute(&value.get_type().get_non_specified_string(), value); match *value { Value::Bool { .. } => TextStyle::with_style(AlignmentHorizontal::Left, s), diff --git a/crates/nu-command/src/viewers/table.rs b/crates/nu-command/src/viewers/table.rs index b8ee4adbf..67c1cd712 100644 --- a/crates/nu-command/src/viewers/table.rs +++ b/crates/nu-command/src/viewers/table.rs @@ -55,7 +55,7 @@ impl Command for Table { vec!["display", "render"] } - fn signature(&self) -> nu_protocol::Signature { + fn signature(&self) -> Signature { Signature::build("table") .input_output_types(vec![(Type::Any, Type::Any)]) // TODO: make this more precise: what turns into string and what into raw stream @@ -276,61 +276,18 @@ fn handle_table_command( ctrlc, metadata, ), - PipelineData::Value(Value::Record { cols, vals, span }, ..) => { - // Create a StyleComputer to compute styles for each value in the table. - let style_computer = &StyleComputer::from_config(engine_state, stack); - let result = match table_view { - TableView::General => build_general_table2( - style_computer, - cols, - vals, - ctrlc.clone(), - config, - term_width, - ), - TableView::Expanded { - limit, - flatten, - flatten_separator, - } => { - let sep = flatten_separator.as_deref().unwrap_or(" "); - build_expanded_table( - cols, - vals, - span, - ctrlc.clone(), - config, - style_computer, - term_width, - limit, - flatten, - sep, - ) - } - TableView::Collapsed => { - build_collapsed_table(style_computer, cols, vals, config, term_width) - } - }?; - - let result = strip_output_color(result, config); - - let result = result.unwrap_or_else(|| { - if nu_utils::ctrl_c::was_pressed(&ctrlc) { - "".into() - } else { - // assume this failed because the table was too wide - // TODO: more robust error classification - format!("Couldn't fit table into {term_width} columns!") - } - }); - - let val = Value::String { - val: result, - span: call.head, - }; - - Ok(val.into_pipeline_data()) - } + PipelineData::Value(Value::Record { cols, vals, span }, ..) => handle_record( + cols, + vals, + span, + engine_state, + stack, + call, + table_view, + term_width, + ctrlc, + config, + ), PipelineData::Value(Value::LazyRecord { val, .. }, ..) => { let collected = val.collect()?.into_pipeline_data(); handle_table_command( @@ -527,32 +484,39 @@ fn build_expanded_table( } } Value::Record { cols, vals, span } => { - let result = build_expanded_table( - cols.clone(), - vals.clone(), - span, - ctrlc.clone(), - config, - style_computer, - value_width, - deep, - flatten, - flatten_sep, - )?; + if cols.is_empty() { + // Like list case return styled string instead of empty value + let value = Value::Record { cols, vals, span }; + let text = value_to_styled_string(&value, config, style_computer).0; + wrap_text(&text, value_width, config) + } else { + let result = build_expanded_table( + cols.clone(), + vals.clone(), + span, + ctrlc.clone(), + config, + style_computer, + value_width, + deep, + flatten, + flatten_sep, + )?; - match result { - Some(result) => { - is_expanded = true; - result - } - None => { - let failed_value = value_to_styled_string( - &Value::Record { cols, vals, span }, - config, - style_computer, - ); + match result { + Some(result) => { + is_expanded = true; + result + } + None => { + let failed_value = value_to_styled_string( + &Value::Record { cols, vals, span }, + config, + style_computer, + ); - wrap_text(&failed_value.0, value_width, config) + wrap_text(&failed_value.0, value_width, config) + } } } } @@ -607,6 +571,79 @@ fn build_expanded_table( Ok(table) } +#[allow(clippy::too_many_arguments)] +fn handle_record( + cols: Vec, + vals: Vec, + span: Span, + engine_state: &EngineState, + stack: &mut Stack, + call: &Call, + table_view: TableView, + term_width: usize, + ctrlc: Option>, + config: &Config, +) -> Result { + // Create a StyleComputer to compute styles for each value in the table. + let style_computer = &StyleComputer::from_config(engine_state, stack); + + let result = if cols.is_empty() { + create_empty_placeholder("record", term_width, engine_state, stack) + } else { + let result = match table_view { + TableView::General => build_general_table2( + style_computer, + cols, + vals, + ctrlc.clone(), + config, + term_width, + ), + TableView::Expanded { + limit, + flatten, + flatten_separator, + } => { + let sep = flatten_separator.as_deref().unwrap_or(" "); + build_expanded_table( + cols, + vals, + span, + ctrlc.clone(), + config, + style_computer, + term_width, + limit, + flatten, + sep, + ) + } + TableView::Collapsed => { + build_collapsed_table(style_computer, cols, vals, config, term_width) + } + }?; + + let result = strip_output_color(result, config); + + result.unwrap_or_else(|| { + if nu_utils::ctrl_c::was_pressed(&ctrlc) { + "".into() + } else { + // assume this failed because the table was too wide + // TODO: more robust error classification + format!("Couldn't fit table into {term_width} columns!") + } + }) + }; + + let val = Value::String { + val: result, + span: call.head, + }; + + Ok(val.into_pipeline_data()) +} + fn handle_row_stream( engine_state: &EngineState, stack: &mut Stack, @@ -721,18 +758,18 @@ fn handle_row_stream( Ok(PipelineData::ExternalStream { stdout: Some(RawStream::new( - Box::new(PagingTableCreator { - row_offset, - // These are passed in as a way to have PagingTable create StyleComputers - // for the values it outputs. Because engine_state is passed in, config doesn't need to. - engine_state: engine_state.clone(), - stack: stack.clone(), - ctrlc: ctrlc.clone(), + Box::new(PagingTableCreator::new( head, stream, + // These are passed in as a way to have PagingTable create StyleComputers + // for the values it outputs. Because engine_state is passed in, config doesn't need to. + engine_state.clone(), + stack.clone(), + ctrlc.clone(), + row_offset, width_param, - view: table_view, - }), + table_view, + )), ctrlc, head, None, @@ -1522,9 +1559,36 @@ struct PagingTableCreator { row_offset: usize, width_param: Option, view: TableView, + elements_displayed: usize, + reached_end: bool, } impl PagingTableCreator { + #[allow(clippy::too_many_arguments)] + fn new( + head: Span, + stream: ListStream, + engine_state: EngineState, + stack: Stack, + ctrlc: Option>, + row_offset: usize, + width_param: Option, + view: TableView, + ) -> Self { + PagingTableCreator { + head, + stream, + engine_state, + stack, + ctrlc, + row_offset, + width_param, + view, + elements_displayed: 0, + reached_end: false, + } + } + fn build_extended( &mut self, batch: &[Value], @@ -1665,6 +1729,7 @@ impl Iterator for PagingTableCreator { let start_time = Instant::now(); let mut idx = 0; + let mut reached_end = true; // Pull from stream until time runs out or we have enough items for item in self.stream.by_ref() { @@ -1673,10 +1738,12 @@ impl Iterator for PagingTableCreator { // If we've been buffering over a second, go ahead and send out what we have so far if (Instant::now() - start_time).as_secs() >= 1 { + reached_end = false; break; } if idx == STREAM_PAGE_SIZE { + reached_end = false; break; } @@ -1685,8 +1752,24 @@ impl Iterator for PagingTableCreator { } } + // Count how much elements were displayed and if end of stream was reached + self.elements_displayed += idx; + self.reached_end = self.reached_end || reached_end; + if batch.is_empty() { - return None; + // If this iterator has not displayed a single entry and reached its end (no more elements + // or interrupted by ctrl+c) display as "empty list" + return if self.elements_displayed == 0 && self.reached_end { + // Increase elements_displayed by one so on next iteration next branch of this + // if else triggers and terminates stream + self.elements_displayed = 1; + let term_width = get_width_param(self.width_param); + let result = + create_empty_placeholder("list", term_width, &self.engine_state, &self.stack); + Some(Ok(result.into_bytes())) + } else { + None + }; } let table = match &self.view { @@ -1729,17 +1812,17 @@ impl Iterator for PagingTableCreator { fn load_theme_from_config(config: &Config) -> TableTheme { match config.table_mode.as_str() { - "basic" => nu_table::TableTheme::basic(), - "thin" => nu_table::TableTheme::thin(), - "light" => nu_table::TableTheme::light(), - "compact" => nu_table::TableTheme::compact(), - "with_love" => nu_table::TableTheme::with_love(), - "compact_double" => nu_table::TableTheme::compact_double(), - "rounded" => nu_table::TableTheme::rounded(), - "reinforced" => nu_table::TableTheme::reinforced(), - "heavy" => nu_table::TableTheme::heavy(), - "none" => nu_table::TableTheme::none(), - _ => nu_table::TableTheme::rounded(), + "basic" => TableTheme::basic(), + "thin" => TableTheme::thin(), + "light" => TableTheme::light(), + "compact" => TableTheme::compact(), + "with_love" => TableTheme::with_love(), + "compact_double" => TableTheme::compact_double(), + "rounded" => TableTheme::rounded(), + "reinforced" => TableTheme::reinforced(), + "heavy" => TableTheme::heavy(), + "none" => TableTheme::none(), + _ => TableTheme::rounded(), } } @@ -1849,6 +1932,31 @@ fn need_footer(config: &Config, count_records: u64) -> bool { || matches!(config.footer_mode, FooterMode::Always) } +fn create_empty_placeholder( + value_type_name: &str, + termwidth: usize, + engine_state: &EngineState, + stack: &Stack, +) -> String { + let config = engine_state.get_config(); + + if !config.table_show_empty { + return "".into(); + } + + let empty_info_string = format!("empty {}", value_type_name); + let cell = NuTable::create_cell(empty_info_string, TextStyle::default().dimmed()); + let data = vec![vec![cell]]; + let table = NuTable::new(data, (1, 1)); + + let style_computer = &StyleComputer::from_config(engine_state, stack); + let config = create_table_config(config, style_computer, 1, false, false, false); + + table + .draw(config, termwidth) + .expect("Could not create empty table placeholder") +} + fn colorize_value(value: &mut Value, config: &Config, style_computer: &StyleComputer) { match value { Value::Record { cols, vals, .. } => { diff --git a/crates/nu-command/tests/commands/find.rs b/crates/nu-command/tests/commands/find.rs index e73baccc9..ae9a56d93 100644 --- a/crates/nu-command/tests/commands/find.rs +++ b/crates/nu-command/tests/commands/find.rs @@ -55,11 +55,11 @@ fn find_with_string_search_with_string_not_found() { let actual = nu!( cwd: ".", pipeline( r#" - [moe larry curly] | find shemp + [moe larry curly] | find shemp | is-empty "# )); - assert_eq!(actual.out, ""); + assert_eq!(actual.out, "true"); } #[test] diff --git a/crates/nu-command/tests/commands/path/split.rs b/crates/nu-command/tests/commands/path/split.rs index ffd51cd43..67adf6fa6 100644 --- a/crates/nu-command/tests/commands/path/split.rs +++ b/crates/nu-command/tests/commands/path/split.rs @@ -5,11 +5,11 @@ fn splits_empty_path() { let actual = nu!( cwd: "tests", pipeline( r#" - echo '' | path split + echo '' | path split | is-empty "# )); - assert_eq!(actual.out, ""); + assert_eq!(actual.out, "true"); } #[test] diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 0898273be..85bd78e6c 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -8,7 +8,7 @@ fn better_empty_redirection() { let actual = nu!( cwd: "tests/fixtures/formats", pipeline( r#" - ls | each { |it| nu --testbin cococo $it.name } + ls | each { |it| nu --testbin cococo $it.name } | ignore "# )); diff --git a/crates/nu-command/tests/commands/table.rs b/crates/nu-command/tests/commands/table.rs index f7b1cd1a9..8f3b3e530 100644 --- a/crates/nu-command/tests/commands/table.rs +++ b/crates/nu-command/tests/commands/table.rs @@ -1380,10 +1380,8 @@ fn table_expande_with_no_header_internally_0() { "│ │ │ │ │ highlight │ │ bg │ yellow │ │ │ │", "│ │ │ │ │ │ │ fg │ black │ │ │ │", "│ │ │ │ │ │ ╰────┴────────╯ │ │ │", - "│ │ │ │ │ │ │ │ │", - "│ │ │ │ │ status │ │ │ │", - "│ │ │ │ │ │ │ │ │", - "│ │ │ │ │ try │ │ │ │", + "│ │ │ │ │ status │ {record 0 fields} │ │ │", + "│ │ │ │ │ try │ {record 0 fields} │ │ │", "│ │ │ │ │ │ ╭──────────────────┬─────────╮ │ │ │", "│ │ │ │ │ table │ │ split_line │ #404040 │ │ │ │", "│ │ │ │ │ │ │ cursor │ true │ │ │ │", @@ -1624,10 +1622,8 @@ fn table_expande_with_no_header_internally_1() { "│ │ │ │ │ highlight │ │ bg │ yellow │ │ │ │", "│ │ │ │ │ │ │ fg │ black │ │ │ │", "│ │ │ │ │ │ ╰────┴────────╯ │ │ │", - "│ │ │ │ │ │ │ │ │", - "│ │ │ │ │ status │ │ │ │", - "│ │ │ │ │ │ │ │ │", - "│ │ │ │ │ try │ │ │ │", + "│ │ │ │ │ status │ {record 0 fields} │ │ │", + "│ │ │ │ │ try │ {record 0 fields} │ │ │", "│ │ │ │ │ │ ╭──────────────────┬─────────╮ │ │ │", "│ │ │ │ │ table │ │ split_line │ #404040 │ │ │ │", "│ │ │ │ │ │ │ cursor │ true │ │ │ │", diff --git a/crates/nu-protocol/src/config.rs b/crates/nu-protocol/src/config.rs index d361a33e3..7115ec7cf 100644 --- a/crates/nu-protocol/src/config.rs +++ b/crates/nu-protocol/src/config.rs @@ -65,6 +65,7 @@ pub struct Config { pub external_completer: Option, pub filesize_metric: bool, pub table_mode: String, + pub table_show_empty: bool, pub use_ls_colors: bool, pub color_config: HashMap, pub use_grid_icons: bool, @@ -106,6 +107,7 @@ impl Default for Config { Config { filesize_metric: false, table_mode: "rounded".into(), + table_show_empty: true, external_completer: None, use_ls_colors: true, color_config: HashMap::new(), @@ -885,6 +887,9 @@ impl Value { } } } + "show_empty" => { + try_bool!(cols, vals, index, span, table_show_empty) + } x => { invalid_key!( cols, diff --git a/crates/nu-protocol/src/ty.rs b/crates/nu-protocol/src/ty.rs index e84326369..4c73f3124 100644 --- a/crates/nu-protocol/src/ty.rs +++ b/crates/nu-protocol/src/ty.rs @@ -96,6 +96,35 @@ impl Type { Type::Signature => SyntaxShape::Signature, } } + + /// Get a string representation, without inner type specification of lists, + /// tables and records (get `list` instead of `list` + pub fn get_non_specified_string(&self) -> String { + match self { + Type::Block => String::from("block"), + Type::Closure => String::from("closure"), + Type::Bool => String::from("bool"), + Type::CellPath => String::from("cell path"), + Type::Date => String::from("date"), + Type::Duration => String::from("duration"), + Type::Filesize => String::from("filesize"), + Type::Float => String::from("float"), + Type::Int => String::from("int"), + Type::Range => String::from("range"), + Type::Record(_) => String::from("record"), + Type::Table(_) => String::from("table"), + Type::List(_) => String::from("list"), + Type::Nothing => String::from("nothing"), + Type::Number => String::from("number"), + Type::String => String::from("string"), + Type::ListStream => String::from("list stream"), + Type::Any => String::from("any"), + Type::Error => String::from("error"), + Type::Binary => String::from("binary"), + Type::Custom(_) => String::from("custom"), + Type::Signature => String::from("signature"), + } + } } impl Display for Type { diff --git a/crates/nu-utils/src/sample_config/default_config.nu b/crates/nu-utils/src/sample_config/default_config.nu index 60b8f43f9..f53711f87 100644 --- a/crates/nu-utils/src/sample_config/default_config.nu +++ b/crates/nu-utils/src/sample_config/default_config.nu @@ -187,6 +187,7 @@ let-env config = { table: { mode: rounded # basic, compact, compact_double, light, thin, with_love, rounded, reinforced, heavy, none, other index_mode: always # "always" show indexes, "never" show indexes, "auto" = show indexes when a table has "index" column + show_empty: true # show 'empty list' and 'empty record' placeholders for command output trim: { methodology: wrapping # wrapping or truncating wrapping_try_keep_words: true # A strategy used by the 'wrapping' methodology diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index b2e7f4d19..82fe1bb02 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -299,11 +299,11 @@ fn run_custom_command_with_empty_rest() { let actual = nu!( cwd: ".", r#" - def rest-me-with-empty-rest [...rest: string] { echo $rest }; rest-me-with-empty-rest + def rest-me-with-empty-rest [...rest: string] { $rest }; rest-me-with-empty-rest | is-empty "# ); - assert_eq!(actual.out, r#""#); + assert_eq!(actual.out, r#"true"#); assert_eq!(actual.err, r#""#); }