Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix argument type #5695

Merged
merged 7 commits into from
Jun 6, 2022
Merged

fix argument type #5695

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec43c5b
fix argument type
WindSoilder Jun 2, 2022
0e18566
while run external, convert list argument to str
WindSoilder Jun 2, 2022
c51b9ea
fix argument converting logic
WindSoilder Jun 2, 2022
cb785c7
using parse_list_expression instead of parse_full_cell_path
WindSoilder Jun 2, 2022
31a5cc4
make parsing logic more explicit
WindSoilder Jun 4, 2022
a78474b
revert changes
WindSoilder Jun 6, 2022
fce8067
add tests
WindSoilder Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions crates/nu-command/src/system/run_external.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,20 +69,24 @@ impl Command for External {
})
}

let args = args
.into_iter()
.flat_map(|arg| match arg {
Value::List { vals, .. } => vals
.into_iter()
.map(value_as_spanned)
.collect::<Vec<Result<Spanned<String>, ShellError>>>(),
val => vec![value_as_spanned(val)],
})
.collect::<Result<Vec<Spanned<String>>, ShellError>>()?;
let mut spanned_args = vec![];
for one_arg in args {
match one_arg {
Value::List { vals, .. } => {
// turn all the strings in the array into params.
// Example: one_arg may be something like ["ls" "-a"]
// convert it to "ls" "-a"
for v in vals {
spanned_args.push(value_as_spanned(v)?)
}
}
val => spanned_args.push(value_as_spanned(val)?),
}
}
kubouch marked this conversation as resolved.
Show resolved Hide resolved
WindSoilder marked this conversation as resolved.
Show resolved Hide resolved

let command = ExternalCommand {
name,
args,
args: spanned_args,
redirect_stdout,
redirect_stderr,
env_vars: env_vars_str,
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-parser/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ pub fn parse_external_call(
let (arg, err) = parse_dollar_expr(working_set, *span, expand_aliases_denylist);
error = error.or(err);
args.push(arg);
} else if contents.starts_with(b"(") {
} else if contents.starts_with(b"[") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might work (so you can pass nu -c [ 'a' '-b' '--c' ], for example), but intstead of parse_full_cell_path(), I'd use parse_list_expression().

I'm not sure why this conditional was there in the first place since it's already checked above. Maybe @jntrnr knows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :-) parse_list_expression is explicit and better.

I'm not sure why this conditional was there in the first place since it's already checked above.

+1 for the not sure...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the parens were originally so that you can do stuff like:

./external ("foo" + "bar.txt")

I think we still want to support this. If we bring in support for the string arrays, that should probably be a separate thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jntrnr Isn't the subexpression checked on the line 293 though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, that 293 line looks odd. Are we really checking for ( as the start and then calling parse_dollar_expr? Does this cause any bugs in the current system?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_dollar_expr goes to parse_full_cell_path which checks for (. So to me it seems this line is a duplicate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if parse_dollar_expr just needs to be called something different now if it's doing more than parsing dollar expressions 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it doesn't seem very accurate anymore :-D. But I guess we can maybe rename it and leave the [ check for the list parsing?

Copy link
Collaborator Author

@WindSoilder WindSoilder Jun 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... should I change to something like this to make the logic more explicit?

if contents.starts_with(b"$") {
    let (arg, err) = parse_dollar_expr(working_set, *span, expand_aliases_denylist);
    error = error.or(err);
    args.push(arg);
} else if contents.starts_with(b"(") {
     let (arg, err) =
         parse_full_cell_path(working_set, None, *span, expand_aliases_denylist);
     error = error.or(err);
     args.push(arg);
 }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the parse_dollar_expr code it's actually parsing more than a dollar expression, so the original condition should be preserved. It's just a matter of adding for [...].

WindSoilder marked this conversation as resolved.
Show resolved Hide resolved
let (arg, err) =
parse_full_cell_path(working_set, None, *span, expand_aliases_denylist);
WindSoilder marked this conversation as resolved.
Show resolved Hide resolved
error = error.or(err);
Expand Down