holm/nushell
1
1
Fork 0
forked from extern/nushell
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Make get hole errors and cell path hole errors identical (improvement on #7002) (#7647)

Browse Source 
# Description

This closes #7498, as well as fixes an issue reported in
https://github.com/nushell/nushell/pull/7002#issuecomment-1368340773

BEFORE:
```
〉[{foo: 'bar'} {}] | get foo
Error: nu:🐚:column_not_found (link)

  × Cannot find column
   ╭─[entry #5:1:1]
 1 │ [{foo: 'bar'} {}] | get foo
   · ────────┬────────   ─┬─
   ·         │            ╰── value originates here
   ·         ╰── cannot find column 'Empty cell'
   ╰────

〉[{foo: 'bar'} {}].foo
╭───┬─────╮
│ 0 │ bar │
│ 1 │     │
╰───┴─────╯
```
AFTER:
```
〉[{foo: 'bar'} {}] | get foo
Error: nu:🐚:column_not_found (link)

  × Cannot find column
   ╭─[entry #1:1:1]
 1 │ [{foo: 'bar'} {}] | get foo
   ·               ─┬        ─┬─
   ·                │         ╰── cannot find column 'foo'
   ·                ╰── value originates here
   ╰────

〉[{foo: 'bar'} {}].foo
Error: nu:🐚:column_not_found (link)

  × Cannot find column
   ╭─[entry #3:1:1]
 1 │ [{foo: 'bar'} {}].foo
   ·               ─┬  ─┬─
   ·                │   ╰── cannot find column 'foo'
   ·                ╰── value originates here       
   ╰────
```

EDIT: This also changes the semantics of `get`/`select` `-i` somewhat.
I've decided to leave it like this because it works more intuitively
with `default` and `compact`.
BEFORE:
```
〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
null
```
AFTER:
```
〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
[[foo]; [null], [null], [null]]
```

# User-Facing Changes

See above. EDIT: the issue with holes in cases like ` [{foo: 'bar'}
{}].foo.0` versus ` [{foo: 'bar'} {}].0.foo` has been resolved.

# 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.
This commit is contained in:
Leon
2023-01-03 08:45:43 +10:00
committed by GitHub
parent 614bc2a943
commit 65d0b5b9d9
22 changed files with 326 additions and 222 deletions
Download Patch File Download Diff File Expand all files Collapse all files

172
crates/nu-protocol/src/value/mod.rs
View File

@ -468,6 +468,18 @@ impl Value {
}
}
// Convert Value into String, but propagate errors.
pub fn nonerror_into_string(
&self,
separator: &str,
config: &Config,
) -> Result<String, ShellError> {
match self {
Value::Error { error } => Err(error.to_owned()),
_ => Ok(self.into_string(separator, config)),
}
}
/// Convert Value into string. Note that Streams will be consumed.
pub fn into_string(&self, separator: &str, config: &Config) -> String {
match self {
@ -627,8 +639,9 @@ impl Value {
self,
cell_path: &[PathMember],
insensitive: bool,
nullify_errors: bool,
) -> Result<Value, ShellError> {
self.follow_cell_path_helper(cell_path, insensitive, true)
self.follow_cell_path_helper(cell_path, insensitive, nullify_errors, true)
}
pub fn follow_cell_path_not_from_user_input(
@ -636,16 +649,26 @@ impl Value {
cell_path: &[PathMember],
insensitive: bool,
) -> Result<Value, ShellError> {
self.follow_cell_path_helper(cell_path, insensitive, false)
self.follow_cell_path_helper(cell_path, insensitive, false, false)
}
fn follow_cell_path_helper(
self,
cell_path: &[PathMember],
insensitive: bool,
nullify_errors: bool, // Turn all errors into Value::Nothing
from_user_input: bool,
) -> Result<Value, ShellError> {
let mut current = self;
macro_rules! err_or_null {
($e:expr, $span:expr) => {
return if nullify_errors {
Ok(Value::nothing($span))
} else {
Err($e)
}
};
}
for member in cell_path {
// FIXME: this uses a few extra clones for simplicity, but there may be a way
// to traverse the path without them
@ -660,34 +683,61 @@ impl Value {
if let Some(item) = val.get(*count) {
current = item.clone();
} else if val.is_empty() {
return Err(ShellError::AccessEmptyContent(*origin_span))
err_or_null!(
ShellError::AccessEmptyContent(*origin_span),
*origin_span
)
} else {
return Err(ShellError::AccessBeyondEnd(val.len() - 1, *origin_span));
err_or_null!(
ShellError::AccessBeyondEnd(val.len() - 1, *origin_span),
*origin_span
);
}
}
Value::Binary { val, .. } => {
if let Some(item) = val.get(*count) {
current = Value::int(*item as i64, *origin_span);
} else if val.is_empty() {
return Err(ShellError::AccessEmptyContent(*origin_span))
err_or_null!(
ShellError::AccessEmptyContent(*origin_span),
*origin_span
)
} else {
return Err(ShellError::AccessBeyondEnd(val.len() - 1, *origin_span));
err_or_null!(
ShellError::AccessBeyondEnd(val.len() - 1, *origin_span),
*origin_span
);
}
}
Value::Range { val, .. } => {
if let Some(item) = val.clone().into_range_iter(None)?.nth(*count) {
current = item.clone();
} else {
return Err(ShellError::AccessBeyondEndOfStream(*origin_span));
err_or_null!(
ShellError::AccessBeyondEndOfStream(*origin_span),
*origin_span
);
}
}
Value::CustomValue { val, .. } => {
current = val.follow_path_int(*count, *origin_span)?;
}
// Records (and tables) are the only built-in which support column names,
// so only use this message for them.
Value::Record { .. } => {
err_or_null!(ShellError::TypeMismatchGenericMessage {
err_message: "Can't access record values with a row index. Try specifying a column name instead".into(),
span: *origin_span, }, *origin_span)
}
Value::Error { error } => return Err(error.to_owned()),
x => {
return Err(ShellError::TypeMismatchGenericMessage {
err_message: format!("Can't access {} values with a row index. Try specifying a column name instead", x.get_type().to_shape()),
span: *origin_span, })
err_or_null!(
ShellError::IncompatiblePathAccess(
format!("{}", x.get_type()),
*origin_span,
),
*origin_span
)
}
}
}
@ -711,16 +761,26 @@ impl Value {
} else {
if from_user_input {
if let Some(suggestion) = did_you_mean(&cols, column_name) {
return Err(ShellError::DidYouMean(suggestion, *origin_span));
err_or_null!(
ShellError::DidYouMean(suggestion, *origin_span),
*origin_span
);
}
}
return Err(ShellError::CantFindColumn(
column_name.to_string(),
*origin_span,
span,
));
err_or_null!(
ShellError::CantFindColumn(
column_name.to_string(),
*origin_span,
span,
),
*origin_span
);
}
}
// String access of Lists always means Table access.
// Create a List which contains each matching value for contained
// records in the source list.
// If nullify_errors is true, table holes are converted to null.
Value::List { vals, span } => {
let mut output = vec![];
let mut found_at_least_1_value = false;
@ -733,14 +793,50 @@ impl Value {
span: *origin_span,
}],
insensitive,
nullify_errors,
) {
found_at_least_1_value = true;
output.push(result);
} else {
output.push(Value::Nothing { span: *span });
// Consider [{a:1 b:2} {a:3}]:
// [{a:1 b:2} {a:3}].b should error.
// [{a:1 b:2} {a:3}].b.1 should error.
// [{a:1 b:2} {a:3}].b.0 should NOT error because the path can find a proper value (2)
// but if this returns an error immediately, it will.
//
// Solution: push a Value::Error into this result list instead of returning it.
// This also means that `[{a:1 b:2} {a:2}].b | reject 1` also doesn't error.
// Anything that needs to use every value inside the list should propagate
// the error outward, though.
output.push(if nullify_errors {
Value::nothing(*origin_span)
} else {
Value::Error {
error: ShellError::CantFindColumn(
column_name.to_string(),
*origin_span,
// Get the exact span of the value, falling back to
// the list's span if it's a Value::Empty
val.span().unwrap_or(*span),
),
}
});
}
} else {
output.push(Value::Nothing { span: *span });
// See comment above.
output.push(if nullify_errors {
Value::nothing(*origin_span)
} else {
Value::Error {
error: ShellError::CantFindColumn(
column_name.to_string(),
*origin_span,
// Get the exact span of the value, falling back to
// the list's span if it's a Value::Empty
val.span().unwrap_or(*span),
),
}
});
}
}
if found_at_least_1_value {
@ -749,23 +845,39 @@ impl Value {
span: *span,
};
} else {
return Err(ShellError::NotFound(*span));
err_or_null!(
ShellError::CantFindColumn(
column_name.to_string(),
*origin_span,
*span,
),
*origin_span
);
}
}
Value::CustomValue { val, .. } => {
current = val.follow_path_string(column_name.clone(), *origin_span)?;
}
Value::Error { error } => err_or_null!(error.to_owned(), *origin_span),
x => {
return Err(ShellError::IncompatiblePathAccess(
format!("{}", x.get_type()),
*origin_span,
))
err_or_null!(
ShellError::IncompatiblePathAccess(
format!("{}", x.get_type()),
*origin_span,
),
*origin_span
)
}
},
}
}
Ok(current)
// If a single Value::Error was produced by the above (which won't happen if nullify_errors is true), unwrap it now.
// Note that Value::Errors inside Lists remain as they are, so that the rest of the list can still potentially be used.
if let Value::Error { error } = current {
Err(error)
} else {
Ok(current)
}
}
/// Follow a given cell path into the value: for example accessing select elements in a stream or list
@ -776,7 +888,7 @@ impl Value {
) -> Result<(), ShellError> {
let orig = self.clone();
let new_val = callback(&orig.follow_cell_path(cell_path, false)?);
let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?);
match new_val {
Value::Error { error } => Err(error),
@ -829,6 +941,7 @@ impl Value {
}
}
}
Value::Error { error } => return Err(error.to_owned()),
v => {
return Err(ShellError::CantFindColumn(
col_name.to_string(),
@ -865,6 +978,7 @@ impl Value {
}
}
}
Value::Error { error } => return Err(error.to_owned()),
v => {
return Err(ShellError::CantFindColumn(
col_name.to_string(),
@ -885,6 +999,7 @@ impl Value {
return Err(ShellError::InsertAfterNextFreeIndex(vals.len(), *span));
}
}
Value::Error { error } => return Err(error.to_owned()),
v => return Err(ShellError::NotAList(*span, v.span()?)),
},
},
@ -903,7 +1018,7 @@ impl Value {
) -> Result<(), ShellError> {
let orig = self.clone();
let new_val = callback(&orig.follow_cell_path(cell_path, false)?);
let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?);
match new_val {
Value::Error { error } => Err(error),
@ -948,6 +1063,7 @@ impl Value {
));
}
}
Value::Error { error } => return Err(error.to_owned()),
v => {
return Err(ShellError::CantFindColumn(
col_name.to_string(),
@ -981,6 +1097,7 @@ impl Value {
));
}
}
Value::Error { error } => return Err(error.to_owned()),
v => {
return Err(ShellError::CantFindColumn(
col_name.to_string(),
@ -999,6 +1116,7 @@ impl Value {
return Err(ShellError::AccessBeyondEnd(vals.len() - 1, *span));
}
}
Value::Error { error } => return Err(error.to_owned()),
v => return Err(ShellError::NotAList(*span, v.span()?)),
},
},