mirror of
https://github.com/nushell/nushell.git
synced 2025-08-09 02:45:08 +02:00
Standardise the use of ShellError::UnsupportedInput and ShellError::TypeMismatch and add spans to every instance of the former (#7217)
# Description * I was dismayed to discover recently that UnsupportedInput and TypeMismatch are used *extremely* inconsistently across the codebase. UnsupportedInput is sometimes used for input type-checks (as per the name!!), but *also* used for argument type-checks. TypeMismatch is also used for both. I thus devised the following standard: input type-checking *only* uses UnsupportedInput, and argument type-checking *only* uses TypeMismatch. Moreover, to differentiate them, UnsupportedInput now has *two* error arrows (spans), one pointing at the command and the other at the input origin, while TypeMismatch only has the one (because the command should always be nearby) * In order to apply that standard, a very large number of UnsupportedInput uses were changed so that the input's span could be retrieved and delivered to it. * Additionally, I noticed many places where **errors are not propagated correctly**: there are lots of `match` sites which take a Value::Error, then throw it away and replace it with a new Value::Error with less/misleading information (such as reporting the error as an "incorrect type"). I believe that the earliest errors are the most important, and should always be propagated where possible. * Also, to standardise one broad subset of UnsupportedInput error messages, who all used slightly different wordings of "expected `<type>`, got `<type>`", I created OnlySupportsThisInputType as a variant of it. * Finally, a bunch of error sites that had "repeated spans" - i.e. where an error expected two spans, but `call.head` was given for both - were fixed to use different spans. # Example BEFORE ``` 〉20b | str starts-with 'a' Error: nu:🐚:unsupported_input (link) × Unsupported input ╭─[entry #31:1:1] 1 │ 20b | str starts-with 'a' · ┬ · ╰── Input's type is filesize. This command only works with strings. ╰──── 〉'a' | math cos Error: nu:🐚:unsupported_input (link) × Unsupported input ╭─[entry #33:1:1] 1 │ 'a' | math cos · ─┬─ · ╰── Only numerical values are supported, input type: String ╰──── 〉0x[12] | encode utf8 Error: nu:🐚:unsupported_input (link) × Unsupported input ╭─[entry #38:1:1] 1 │ 0x[12] | encode utf8 · ───┬── · ╰── non-string input ╰──── ``` AFTER ``` 〉20b | str starts-with 'a' Error: nu:🐚:pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #1:1:1] 1 │ 20b | str starts-with 'a' · ┬ ───────┬─────── · │ ╰── only string input data is supported · ╰── input type: filesize ╰──── 〉'a' | math cos Error: nu:🐚:pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #2:1:1] 1 │ 'a' | math cos · ─┬─ ────┬─── · │ ╰── only numeric input data is supported · ╰── input type: string ╰──── 〉0x[12] | encode utf8 Error: nu:🐚:pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #3:1:1] 1 │ 0x[12] | encode utf8 · ───┬── ───┬── · │ ╰── only string input data is supported · ╰── input type: binary ╰──── ``` # User-Facing Changes Various error messages suddenly make more sense (i.e. have two arrows instead of one). # 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:
@ -64,7 +64,7 @@ pub fn from_delimited_data(
|
||||
input: PipelineData,
|
||||
name: Span,
|
||||
) -> Result<PipelineData, ShellError> {
|
||||
let (concat_string, metadata) = input.collect_string_strict(name)?;
|
||||
let (concat_string, _span, metadata) = input.collect_string_strict(name)?;
|
||||
|
||||
Ok(
|
||||
from_delimited_string_to_value(concat_string, noheaders, no_infer, sep, trim, name)
|
||||
@ -80,7 +80,7 @@ pub fn trim_from_str(trim: Option<Value>) -> Result<Trim, ShellError> {
|
||||
"headers" => Ok(Trim::Headers),
|
||||
"fields" => Ok(Trim::Fields),
|
||||
"none" => Ok(Trim::None),
|
||||
_ => Err(ShellError::UnsupportedInput(
|
||||
_ => Err(ShellError::TypeMismatch(
|
||||
"the only possible values for trim are 'all', 'headers', 'fields' and 'none'"
|
||||
.into(),
|
||||
span,
|
||||
|
@ -178,7 +178,7 @@ fn from_eml(
|
||||
preview_body: Option<Spanned<i64>>,
|
||||
head: Span,
|
||||
) -> Result<PipelineData, ShellError> {
|
||||
let (value, metadata) = input.collect_string_strict(head)?;
|
||||
let (value, _span, metadata, ..) = input.collect_string_strict(head)?;
|
||||
|
||||
let body_preview = preview_body
|
||||
.map(|b| b.item as usize)
|
||||
|
@ -94,7 +94,7 @@ END:VCALENDAR' | from ics",
|
||||
}
|
||||
|
||||
fn from_ics(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (input_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (input_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
let input_string = input_string
|
||||
.lines()
|
||||
@ -114,7 +114,9 @@ fn from_ics(input: PipelineData, head: Span) -> Result<PipelineData, ShellError>
|
||||
Err(e) => output.push(Value::Error {
|
||||
error: ShellError::UnsupportedInput(
|
||||
format!("input cannot be parsed as .ics ({})", e),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span,
|
||||
),
|
||||
}),
|
||||
}
|
||||
|
@ -56,7 +56,11 @@ b=2' | from ini",
|
||||
}
|
||||
}
|
||||
|
||||
pub fn from_ini_string_to_value(s: String, span: Span) -> Result<Value, ShellError> {
|
||||
pub fn from_ini_string_to_value(
|
||||
s: String,
|
||||
span: Span,
|
||||
val_span: Span,
|
||||
) -> Result<Value, ShellError> {
|
||||
let v: Result<IndexMap<String, IndexMap<String, String>>, serde_ini::de::Error> =
|
||||
serde_ini::from_str(&s);
|
||||
match v {
|
||||
@ -77,15 +81,17 @@ pub fn from_ini_string_to_value(s: String, span: Span) -> Result<Value, ShellErr
|
||||
}
|
||||
Err(err) => Err(ShellError::UnsupportedInput(
|
||||
format!("Could not load ini: {}", err),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
val_span,
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
fn from_ini(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (concat_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (concat_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
match from_ini_string_to_value(concat_string, head) {
|
||||
match from_ini_string_to_value(concat_string, head, span) {
|
||||
Ok(x) => Ok(x.into_pipeline_data_with_metadata(metadata)),
|
||||
Err(other) => Err(other),
|
||||
}
|
||||
|
@ -64,7 +64,7 @@ impl Command for FromJson {
|
||||
input: PipelineData,
|
||||
) -> Result<nu_protocol::PipelineData, ShellError> {
|
||||
let span = call.head;
|
||||
let (string_input, metadata) = input.collect_string_strict(span)?;
|
||||
let (string_input, span, metadata) = input.collect_string_strict(span)?;
|
||||
|
||||
if string_input.is_empty() {
|
||||
return Ok(PipelineData::new_with_metadata(metadata, span));
|
||||
|
@ -62,7 +62,7 @@ impl Command for FromNuon {
|
||||
input: PipelineData,
|
||||
) -> Result<nu_protocol::PipelineData, ShellError> {
|
||||
let head = call.head;
|
||||
let (string_input, metadata) = input.collect_string_strict(head)?;
|
||||
let (string_input, _span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
let engine_state = engine_state.clone();
|
||||
|
||||
|
@ -93,10 +93,13 @@ fn collect_binary(input: PipelineData, span: Span) -> Result<Vec<u8>, ShellError
|
||||
Some(Value::Binary { val: b, .. }) => {
|
||||
bytes.extend_from_slice(&b);
|
||||
}
|
||||
Some(Value::Error { error }) => return Err(error),
|
||||
Some(x) => {
|
||||
return Err(ShellError::UnsupportedInput(
|
||||
"Expected binary from pipeline".to_string(),
|
||||
x.span().unwrap_or(span),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
x.expect_span(),
|
||||
))
|
||||
}
|
||||
None => break,
|
||||
@ -111,10 +114,17 @@ fn from_ods(
|
||||
head: Span,
|
||||
sel_sheets: Vec<String>,
|
||||
) -> Result<PipelineData, ShellError> {
|
||||
let span = input.span();
|
||||
let bytes = collect_binary(input, head)?;
|
||||
let buf: Cursor<Vec<u8>> = Cursor::new(bytes);
|
||||
let mut ods = Ods::<_>::new(buf)
|
||||
.map_err(|_| ShellError::UnsupportedInput("Could not load ods file".to_string(), head))?;
|
||||
let mut ods = Ods::<_>::new(buf).map_err(|_| {
|
||||
ShellError::UnsupportedInput(
|
||||
"Could not load ODS file".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span.unwrap_or(head),
|
||||
)
|
||||
})?;
|
||||
|
||||
let mut dict = IndexMap::new();
|
||||
|
||||
@ -170,7 +180,9 @@ fn from_ods(
|
||||
} else {
|
||||
return Err(ShellError::UnsupportedInput(
|
||||
"Could not load sheet".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span.unwrap_or(head),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
@ -275,7 +275,7 @@ fn from_ssv(
|
||||
let minimum_spaces: Option<Spanned<usize>> =
|
||||
call.get_flag(engine_state, stack, "minimum-spaces")?;
|
||||
|
||||
let (concat_string, metadata) = input.collect_string_strict(name)?;
|
||||
let (concat_string, _span, metadata) = input.collect_string_strict(name)?;
|
||||
let split_at = match minimum_spaces {
|
||||
Some(number) => number.item,
|
||||
None => DEFAULT_MINIMUM_SPACES,
|
||||
|
@ -63,7 +63,7 @@ b = [1, 2]' | from toml",
|
||||
input: PipelineData,
|
||||
) -> Result<nu_protocol::PipelineData, ShellError> {
|
||||
let span = call.head;
|
||||
let (mut string_input, metadata) = input.collect_string_strict(span)?;
|
||||
let (mut string_input, span, metadata) = input.collect_string_strict(span)?;
|
||||
string_input.push('\n');
|
||||
Ok(convert_string_to_value(string_input, span)?.into_pipeline_data_with_metadata(metadata))
|
||||
}
|
||||
|
@ -55,7 +55,7 @@ impl Command for FromUrl {
|
||||
}
|
||||
|
||||
fn from_url(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (concat_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (concat_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
let result = serde_urlencoded::from_str::<Vec<(String, String)>>(&concat_string);
|
||||
|
||||
@ -78,8 +78,10 @@ fn from_url(input: PipelineData, head: Span) -> Result<PipelineData, ShellError>
|
||||
))
|
||||
}
|
||||
_ => Err(ShellError::UnsupportedInput(
|
||||
"String not compatible with url-encoding".to_string(),
|
||||
"String not compatible with URL encoding".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span,
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
@ -107,7 +107,7 @@ END:VCARD' | from vcf",
|
||||
}
|
||||
|
||||
fn from_vcf(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (input_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (input_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
let input_string = input_string
|
||||
.lines()
|
||||
@ -124,7 +124,9 @@ fn from_vcf(input: PipelineData, head: Span) -> Result<PipelineData, ShellError>
|
||||
Err(e) => Value::Error {
|
||||
error: ShellError::UnsupportedInput(
|
||||
format!("input cannot be parsed as .vcf ({})", e),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span,
|
||||
),
|
||||
},
|
||||
});
|
||||
|
@ -96,7 +96,9 @@ fn collect_binary(input: PipelineData, span: Span) -> Result<Vec<u8>, ShellError
|
||||
Some(x) => {
|
||||
return Err(ShellError::UnsupportedInput(
|
||||
"Expected binary from pipeline".to_string(),
|
||||
x.span().unwrap_or(span),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
x.expect_span(),
|
||||
))
|
||||
}
|
||||
None => break,
|
||||
@ -111,10 +113,17 @@ fn from_xlsx(
|
||||
head: Span,
|
||||
sel_sheets: Vec<String>,
|
||||
) -> Result<PipelineData, ShellError> {
|
||||
let span = input.span();
|
||||
let bytes = collect_binary(input, head)?;
|
||||
let buf: Cursor<Vec<u8>> = Cursor::new(bytes);
|
||||
let mut xlsx = Xlsx::<_>::new(buf)
|
||||
.map_err(|_| ShellError::UnsupportedInput("Could not load xlsx file".to_string(), head))?;
|
||||
let mut xlsx = Xlsx::<_>::new(buf).map_err(|_| {
|
||||
ShellError::UnsupportedInput(
|
||||
"Could not load XLSX file".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span.unwrap_or(head),
|
||||
)
|
||||
})?;
|
||||
|
||||
let mut dict = IndexMap::new();
|
||||
|
||||
@ -170,7 +179,9 @@ fn from_xlsx(
|
||||
} else {
|
||||
return Err(ShellError::UnsupportedInput(
|
||||
"Could not load sheet".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span.unwrap_or(head),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
@ -178,13 +178,15 @@ pub fn from_xml_string_to_value(s: String, span: Span) -> Result<Value, roxmltre
|
||||
}
|
||||
|
||||
fn from_xml(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (concat_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (concat_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
match from_xml_string_to_value(concat_string, head) {
|
||||
Ok(x) => Ok(x.into_pipeline_data_with_metadata(metadata)),
|
||||
_ => Err(ShellError::UnsupportedInput(
|
||||
"Could not parse string as xml".to_string(),
|
||||
"Could not parse string as XML".to_string(),
|
||||
"value originates from here".into(),
|
||||
head,
|
||||
span,
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
@ -76,9 +76,17 @@ impl Command for FromYml {
|
||||
}
|
||||
}
|
||||
|
||||
fn convert_yaml_value_to_nu_value(v: &serde_yaml::Value, span: Span) -> Result<Value, ShellError> {
|
||||
let err_not_compatible_number =
|
||||
ShellError::UnsupportedInput("Expected a compatible number".to_string(), span);
|
||||
fn convert_yaml_value_to_nu_value(
|
||||
v: &serde_yaml::Value,
|
||||
span: Span,
|
||||
val_span: Span,
|
||||
) -> Result<Value, ShellError> {
|
||||
let err_not_compatible_number = ShellError::UnsupportedInput(
|
||||
"Expected a nu-compatible number in YAML input".to_string(),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
val_span,
|
||||
);
|
||||
Ok(match v {
|
||||
serde_yaml::Value::Bool(b) => Value::Bool { val: *b, span },
|
||||
serde_yaml::Value::Number(n) if n.is_i64() => Value::Int {
|
||||
@ -96,7 +104,7 @@ fn convert_yaml_value_to_nu_value(v: &serde_yaml::Value, span: Span) -> Result<V
|
||||
serde_yaml::Value::Sequence(a) => {
|
||||
let result: Result<Vec<Value>, ShellError> = a
|
||||
.iter()
|
||||
.map(|x| convert_yaml_value_to_nu_value(x, span))
|
||||
.map(|x| convert_yaml_value_to_nu_value(x, span, val_span))
|
||||
.collect();
|
||||
Value::List {
|
||||
vals: result?,
|
||||
@ -113,13 +121,16 @@ fn convert_yaml_value_to_nu_value(v: &serde_yaml::Value, span: Span) -> Result<V
|
||||
// A ShellError that we re-use multiple times in the Mapping scenario
|
||||
let err_unexpected_map = ShellError::UnsupportedInput(
|
||||
format!("Unexpected YAML:\nKey: {:?}\nValue: {:?}", k, v),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
val_span,
|
||||
);
|
||||
match (k, v) {
|
||||
(serde_yaml::Value::String(k), _) => {
|
||||
collected
|
||||
.item
|
||||
.insert(k.clone(), convert_yaml_value_to_nu_value(v, span)?);
|
||||
collected.item.insert(
|
||||
k.clone(),
|
||||
convert_yaml_value_to_nu_value(v, span, val_span)?,
|
||||
);
|
||||
}
|
||||
// Hard-code fix for cases where "v" is a string without quotations with double curly braces
|
||||
// e.g. k = value
|
||||
@ -152,18 +163,27 @@ fn convert_yaml_value_to_nu_value(v: &serde_yaml::Value, span: Span) -> Result<V
|
||||
Value::from(collected)
|
||||
}
|
||||
serde_yaml::Value::Null => Value::nothing(span),
|
||||
x => unimplemented!("Unsupported yaml case: {:?}", x),
|
||||
x => unimplemented!("Unsupported YAML case: {:?}", x),
|
||||
})
|
||||
}
|
||||
|
||||
pub fn from_yaml_string_to_value(s: String, span: Span) -> Result<Value, ShellError> {
|
||||
pub fn from_yaml_string_to_value(
|
||||
s: String,
|
||||
span: Span,
|
||||
val_span: Span,
|
||||
) -> Result<Value, ShellError> {
|
||||
let mut documents = vec![];
|
||||
|
||||
for document in serde_yaml::Deserializer::from_str(&s) {
|
||||
let v: serde_yaml::Value = serde_yaml::Value::deserialize(document).map_err(|x| {
|
||||
ShellError::UnsupportedInput(format!("Could not load yaml: {}", x), span)
|
||||
ShellError::UnsupportedInput(
|
||||
format!("Could not load YAML: {}", x),
|
||||
"value originates from here".into(),
|
||||
span,
|
||||
val_span,
|
||||
)
|
||||
})?;
|
||||
documents.push(convert_yaml_value_to_nu_value(&v, span)?);
|
||||
documents.push(convert_yaml_value_to_nu_value(&v, span, val_span)?);
|
||||
}
|
||||
|
||||
match documents.len() {
|
||||
@ -213,9 +233,9 @@ pub fn get_examples() -> Vec<Example> {
|
||||
}
|
||||
|
||||
fn from_yaml(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
|
||||
let (concat_string, metadata) = input.collect_string_strict(head)?;
|
||||
let (concat_string, span, metadata) = input.collect_string_strict(head)?;
|
||||
|
||||
match from_yaml_string_to_value(concat_string, head) {
|
||||
match from_yaml_string_to_value(concat_string, head, span) {
|
||||
Ok(x) => Ok(x.into_pipeline_data_with_metadata(metadata)),
|
||||
Err(other) => Err(other),
|
||||
}
|
||||
@ -255,7 +275,11 @@ mod test {
|
||||
];
|
||||
let config = Config::default();
|
||||
for tc in tt {
|
||||
let actual = from_yaml_string_to_value(tc.input.to_owned(), Span::test_data());
|
||||
let actual = from_yaml_string_to_value(
|
||||
tc.input.to_owned(),
|
||||
Span::test_data(),
|
||||
Span::test_data(),
|
||||
);
|
||||
if actual.is_err() {
|
||||
assert!(
|
||||
tc.expected.is_err(),
|
||||
|
Reference in New Issue
Block a user