Skip to content

Commit

Permalink
add a warning when a near-table is detected (#30)
Browse files Browse the repository at this point in the history 
this PR
- makes it that `nu::value::is_table` returns a `nu::value::Table`
variant which hold more information about why the value is not a table
- adds a warning banner at the bottom to indicate the user why they are
seeing a list of records
- adds a bunch of new tests for `is_table` for the new table variants
- make the colors of the warning banner configurable

> **Important**
> this will only work with #41, this can be achieved by running
> ```bash
> git merge bump-set_foreground
> ```
> on top of this commit
  • Loading branch information
@amtoine
amtoine authored Apr 13, 2024
1 parent bf44d75 commit c50c9b1
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 31 deletions.
4 changes: 4 additions & 0 deletions examples/config/default.nuon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
foreground: white,
},
},
warning: {
foreground: red,
background: yellow,
}
}
keybindings: {
quit: 'q', # quit `explore`
Expand Down
15 changes: 15 additions & 0 deletions src/config/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ pub struct ColorConfig {
pub status_bar: StatusBarColorConfig,
/// the color when editing a cell
pub editor: EditorColorConfig,
/// the color of a warning banner
pub warning: BgFgColorConfig,
}

/// a pair of background / foreground colors
Expand Down Expand Up @@ -182,6 +184,10 @@ impl Default for Config {
foreground: Color::White,
},
},
warning: BgFgColorConfig {
background: Color::Yellow,
foreground: Color::Red,
},
},
keybindings: KeyBindingsMap {
quit: KeyCode::Char('q'),
Expand Down Expand Up @@ -419,6 +425,15 @@ impl Config {
}
}
}
"warning" => {
if let Some(val) = try_fg_bg_colors(
&value,
&["colors", "warning"],
&config.colors.warning,
)? {
config.colors.warning = val
}
}
x => return Err(invalid_field(&["colors", x], cell.span())),
}
}
Expand Down
110 changes: 87 additions & 23 deletions src/nu/value.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ use nu_protocol::{
record, Record, Span, Type, Value,
};

#[derive(Debug, PartialEq)]
pub(crate) enum Table {
Empty,
RowNotARecord(usize, Type),
RowIncompatibleLen(usize, usize, usize),
RowIncompatibleType(usize, String, Type, Type),
RowInvalidKey(usize, String, Vec<String>),
IsValid,
NotAList,
}

pub(crate) fn mutate_value_cell(value: &Value, cell_path: &CellPath, cell: &Value) -> Value {
if cell_path.members.is_empty() {
return cell.clone();
Expand Down Expand Up @@ -68,29 +79,29 @@ pub(crate) fn mutate_value_cell(value: &Value, cell_path: &CellPath, cell: &Valu
}
}

pub(crate) fn is_table(value: &Value) -> bool {
pub(crate) fn is_table(value: &Value) -> Table {
match value {
Value::List { vals, .. } => {
if vals.is_empty() {
return false;
return Table::Empty;
}

// extract the columns of each row as hashmaps for easier access
let mut rows = Vec::new();
for val in vals {
for (i, val) in vals.iter().enumerate() {
match val.get_type() {
Type::Record(fields) => {
rows.push(fields.into_iter().collect::<HashMap<String, Type>>())
}
_ => return false,
t => return Table::RowNotARecord(i, t),
};
}

// check the number of columns for each row
let n = rows[0].keys().len();
for row in rows.iter().skip(1) {
for (i, row) in rows.iter().skip(1).enumerate() {
if row.keys().len() != n {
return false;
return Table::RowIncompatibleLen(i + 1, row.keys().len(), n);
}
}

Expand All @@ -100,7 +111,7 @@ pub(crate) fn is_table(value: &Value) -> bool {
for (key, val) in rows[0].iter() {
let mut ty = val;

for row in rows.iter().skip(1) {
for (i, row) in rows.iter().skip(1).enumerate() {
match row.get(key) {
Some(v) => match ty {
Type::Nothing => ty = v,
Expand All @@ -113,19 +124,28 @@ pub(crate) fn is_table(value: &Value) -> bool {
// tables
| (v != ty)
{
return false;
return Table::RowIncompatibleType(
i + 1,
key.clone(),
v.clone(),
ty.clone(),
);
}
}
}
},
None => return false,
None => {
let mut keys = row.keys().cloned().collect::<Vec<String>>();
keys.sort();
return Table::RowInvalidKey(i + 1, key.clone(), keys);
}
}
}
}

true
Table::IsValid
}
_ => false,
_ => Table::NotAList,
}
}

Expand Down Expand Up @@ -158,7 +178,7 @@ pub(crate) fn is_table(value: &Value) -> bool {
/// }
/// ```
pub(crate) fn transpose(value: &Value) -> Value {
if is_table(value) {
if matches!(is_table(value), Table::IsValid) {
let value_rows = match value {
Value::List { vals, .. } => vals,
_ => return value.clone(),
Expand Down Expand Up @@ -257,9 +277,9 @@ mod tests {
use super::{is_table, mutate_value_cell};
use crate::nu::{
cell_path::{to_path_member_vec, PM},
value::transpose,
value::{transpose, Table},
};
use nu_protocol::{ast::CellPath, record, Config, Value};
use nu_protocol::{ast::CellPath, record, Config, Type, Value};

fn default_value_repr(value: &Value) -> String {
value.to_expanded_string(" ", &Config::default())
Expand Down Expand Up @@ -410,8 +430,9 @@ mod tests {
"b" => Value::test_int(2),
}),
]);
assert!(
assert_eq!(
is_table(&table),
Table::IsValid,
"{} should be a table",
default_value_repr(&table)
);
Expand All @@ -426,8 +447,9 @@ mod tests {
"b" => Value::test_int(2),
}),
]);
assert!(
assert_eq!(
is_table(&table_with_out_of_order_columns),
Table::IsValid,
"{} should be a table",
default_value_repr(&table_with_out_of_order_columns)
);
Expand All @@ -442,8 +464,9 @@ mod tests {
"b" => Value::test_int(2),
}),
]);
assert!(
assert_eq!(
is_table(&table_with_nulls),
Table::IsValid,
"{} should be a table",
default_value_repr(&table_with_nulls)
);
Expand All @@ -458,8 +481,9 @@ mod tests {
"b" => Value::test_float(2.34),
}),
]);
assert!(
assert_eq!(
is_table(&table_with_number_colum),
Table::IsValid,
"{} should be a table",
default_value_repr(&table_with_number_colum)
);
Expand All @@ -473,8 +497,9 @@ mod tests {
"b" => Value::test_int(1),
}),
]);
assert!(
!is_table(&not_a_table_missing_field),
assert_eq!(
is_table(&not_a_table_missing_field),
Table::RowIncompatibleLen(1, 2, 1),
"{} should not be a table",
default_value_repr(&not_a_table_missing_field)
);
Expand All @@ -489,13 +514,52 @@ mod tests {
"b" => Value::test_list(vec![Value::test_int(1)]),
}),
]);
assert!(
!is_table(&not_a_table_incompatible_types),
assert_eq!(
is_table(&not_a_table_incompatible_types),
Table::RowIncompatibleType(
1,
"b".to_string(),
Type::List(Box::new(Type::Int)),
Type::Int
),
"{} should not be a table",
default_value_repr(&not_a_table_incompatible_types)
);

assert!(!is_table(&Value::test_int(0)));
assert_eq!(is_table(&Value::test_int(0)), Table::NotAList);

assert_eq!(is_table(&Value::test_list(vec![])), Table::Empty);

let not_a_table_row_not_record = Value::test_list(vec![
Value::test_record(record! {
"a" => Value::test_string("a"),
"b" => Value::test_int(1),
}),
Value::test_int(0),
]);
assert_eq!(
is_table(&not_a_table_row_not_record),
Table::RowNotARecord(1, Type::Int),
"{} should not be a table",
default_value_repr(&not_a_table_row_not_record)
);

let not_a_table_row_invalid_key = Value::test_list(vec![
Value::test_record(record! {
"a" => Value::test_string("a"),
"b" => Value::test_int(1),
}),
Value::test_record(record! {
"a" => Value::test_string("a"),
"c" => Value::test_int(2),
}),
]);
assert_eq!(
is_table(&not_a_table_row_invalid_key),
Table::RowInvalidKey(1, "b".into(), vec!["a".into(), "c".into()]),
"{} should not be a table",
default_value_repr(&not_a_table_row_invalid_key)
);
}

#[test]
Expand Down
53 changes: 45 additions & 8 deletions src/ui.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,6 @@ fn repr_table(table: &[Record]) -> (Vec<String>, Vec<String>, Vec<Vec<String>>)
/// the data will be rendered on top of the bar, and on top of the cell path in case
/// [`crate::config::Config::show_cell_path`] is set to `true`.
fn render_data(frame: &mut Frame, app: &App, config: &Config) {
let data_frame_height = if config.show_cell_path {
frame.size().height - 2
} else {
frame.size().height - 1
};
let rect_without_bottom_bar = Rect::new(0, 0, frame.size().width, data_frame_height);

let mut data_path = app.position.members.clone();
let current = if !app.is_at_bottom() {
data_path.pop()
Expand All @@ -251,6 +244,50 @@ fn render_data(frame: &mut Frame, app: &App, config: &Config) {
)
});

let table_type = is_table(&value);
let is_a_table = matches!(table_type, crate::nu::value::Table::IsValid);

let mut data_frame_height = if config.show_cell_path {
frame.size().height - 2
} else {
frame.size().height - 1
};
if !is_a_table {
let msg = match table_type {
crate::nu::value::Table::Empty => None,
crate::nu::value::Table::RowNotARecord(i, t) => {
Some(format!("row $.{} is not a record: {}", i, t))
}
crate::nu::value::Table::RowIncompatibleLen(i, l, e) => Some(format!(
"row $.{} has incompatible length with first row: expected {} found {}",
i, e, l
)),
crate::nu::value::Table::RowIncompatibleType(i, k, t, e) => Some(format!(
"cell $.{}.{} has incompatible type with first row: expected {} found {}",
i, k, e, t
)),
crate::nu::value::Table::RowInvalidKey(i, k, ks) => Some(format!(
"row $.{} does not contain key '{}': list of keys {:?}",
i, k, ks
)),
crate::nu::value::Table::NotAList => None,
crate::nu::value::Table::IsValid => unreachable!(),
};

if let Some(msg) = msg {
data_frame_height -= 1;
frame.render_widget(
Paragraph::new(msg).alignment(Alignment::Right).style(
Style::default()
.bg(config.colors.warning.background)
.fg(config.colors.warning.foreground),
),
Rect::new(0, data_frame_height, frame.size().width, 1),
);
}
}
let rect_without_bottom_bar = Rect::new(0, 0, frame.size().width, data_frame_height);

let normal_name_style = Style::default()
.fg(config.colors.normal.name.foreground)
.bg(config.colors.normal.name.background);
Expand All @@ -273,7 +310,7 @@ fn render_data(frame: &mut Frame, app: &App, config: &Config) {
None => 0,
};

if is_table(&value) {
if is_a_table {
let (columns, shapes, cells) = match value {
Value::List { vals, .. } => {
let recs = vals
Expand Down

0 comments on commit c50c9b1

Please sign in to comment.