Fix replacement closures for update, insert, and upsert (#11258)

# Description
This PR addresses #11204 which points out that using a closure for the
replacement value with `update`, `insert`, or `upsert` does not work for
lists.

# User-Facing Changes
- Replacement closures should now work for lists in `upsert`, `insert`,
and `update`. E.g., `[0] | update 0 {|i| $i + 1 }` now gives `[1]`
instead of an unhelpful error.
- `[1 2] | insert 4 20` no longer works. Before, this would give `[1, 2,
null, null, 20]`, but now it gives an error. This was done to match the
intended behavior in `Value::insert_data_at_cell_path`, whereas the
behavior before was probably unintentional. Following
`Value::insert_data_at_cell_path`, inserting at the end of a list is
also fine, so the valid indices for `upsert` and `insert` are
`0..=length` just like `Vec::insert` or list inserts in other languages.

# Tests + Formatting
Added tests for `upsert`, `insert`, and `update`:
- Replacement closures for lists, list streams, records, and tables
- Other list stream tests
This commit is contained in:
Ian Manske
2023-12-09 21:22:45 +00:00
committed by GitHub
parent 94b27267fd
commit fa5d7babb9
7 changed files with 1116 additions and 383 deletions

View File

@ -1143,6 +1143,7 @@ impl Value {
cell_path: &[PathMember],
new_val: Value,
) -> Result<(), ShellError> {
let v_span = self.span();
if let Some((member, path)) = cell_path.split_first() {
match member {
PathMember::String {
@ -1215,22 +1216,26 @@ impl Value {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.upsert_data_at_cell_path(path, new_val)?;
} else if vals.len() == *row_num && path.is_empty() {
// If the upsert is at 1 + the end of the list, it's OK.
// Otherwise, it's prohibited.
vals.push(new_val);
} else {
} else if vals.len() != *row_num {
return Err(ShellError::InsertAfterNextFreeIndex {
available_idx: vals.len(),
span: *span,
});
} else if !path.is_empty() {
return Err(ShellError::AccessBeyondEnd {
max_idx: vals.len() - 1,
span: *span,
});
} else {
// If the upsert is at 1 + the end of the list, it's OK.
vals.push(new_val);
}
}
Value::Error { error, .. } => return Err(*error.clone()),
v => {
_ => {
return Err(ShellError::NotAList {
dst_span: *span,
src_span: v.span(),
src_span: v_span,
});
}
},
@ -1632,22 +1637,30 @@ impl Value {
} => match self {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.insert_data_at_cell_path(path, new_val, head_span)?;
} else if vals.len() == *row_num && path.is_empty() {
// If the insert is at 1 + the end of the list, it's OK.
// Otherwise, it's prohibited.
vals.push(new_val);
} else {
if path.is_empty() {
vals.insert(*row_num, new_val);
} else {
v.insert_data_at_cell_path(path, new_val, head_span)?;
}
} else if vals.len() != *row_num {
return Err(ShellError::InsertAfterNextFreeIndex {
available_idx: vals.len(),
span: *span,
});
} else if !path.is_empty() {
return Err(ShellError::AccessBeyondEnd {
max_idx: vals.len() - 1,
span: *span,
});
} else {
// If the insert is at 1 + the end of the list, it's OK.
vals.push(new_val);
}
}
v => {
_ => {
return Err(ShellError::NotAList {
dst_span: *span,
src_span: v.span(),
src_span: v_span,
});
}
},