fix argument type

[WindSoilder]

Description

Fixes: #5056

Contains two part:

1. while parsing input args, if we get list, parse it to Value::List rather than Value::String.
2. Inside run_external, convert from Value::List to String Vec<Spanned<String>> to pass right arguments to external commands

Tests

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 --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all the tests pass

[WindSoilder]

Hmm... closing first until I get better solution

[kubouch]

You need to be able to run some-external ["--some" "--arg"] as some-external --some --arg instead of some-external "--some --arg". In the second case, the external would interpret "--some --arg" as one argument instead of two arguments and would fail.

[WindSoilder]

Thanks for your comment, but that would make the following code in can't be run as expect:

nu -c [seq 1 2]

They seems to be conflict requirements -.-

Another solution maybe making nu as inner command ?

[kubouch]

Yeah, that's expected. The nu -c supports string as an argument so you need to str collect it first. See my reply here: #5056 (comment)

[kubouch]

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?

[WindSoilder]

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...

[sophiajt]

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.

[kubouch]

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

[sophiajt]

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?

[kubouch]

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

[sophiajt]

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

[kubouch]

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?

[WindSoilder]

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);
 }

[kubouch]

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 [...].

[kubouch]

@WindSoilder I added some comments: I'd restore the original condition for parse_dollar_expr, then try to remove the if contents.starts_with(b"(") check for calling parse_full_cell_path and see if it breaks anything (seems like it shouldn't).

Could you also please add some tests covering this? We should test calling nu ([ '-c' 'version' ]) and nu ['-c' 'version'] and make sure both work. You can use the nu-test-support crate for this: See the nushell/tests directory for examples.

[WindSoilder]

@kubouch Thanks, I've added new tests for them and revert my recent change on parser.

[kubouch]

Nice! Ok, let's give it a try. Thanks again for the PR!