Skip to content

Remove required positional arguments from run-external and exec#14765

Merged
WindSoilder merged 6 commits intonushell:mainfrom
132ikl:run-external-rest
Jan 15, 2025
Merged

Remove required positional arguments from run-external and exec#14765
WindSoilder merged 6 commits intonushell:mainfrom
132ikl:run-external-rest

Conversation

@132ikl
Copy link
Copy Markdown
Member

@132ikl 132ikl commented Jan 6, 2025

Description

This PR removes the required positional argument from run-external and exec in favor of the rest arguments, meaning lists of external commands can be spread directly into run-external and exec. This does have the drawback of making calling run-external and exec with no arguments a run-time error rather than a parse error, but I don't imagine that is an issue.

Before (for both run-external and exec):

run-external
# => Error: nu::parser::missing_positional
# => 
# =>   × Missing required positional argument.
# =>    ╭─[entry #9:1:13]
# =>  1 │ run-external
# =>    ╰────
# =>   help: Usage: run-external <command> ...(args) . Use `--help` for more
# =>         information.

let command = ["cat" "hello.txt"]
run-external ...$command
# => Error: nu::parser::missing_positional
# => 
# =>   × Missing required positional argument.
# =>    ╭─[entry #11:1:14]
# =>  1 │ run-external ...$command
# =>    ·              ▲
# =>    ·              ╰── missing command
# =>    ╰────
# =>   help: Usage: run-external <command> ...(args) . Use `--help` for more
# =>         information.
run-external ($command | first) ...($command | skip 1)
# => hello world!

After (for both run-external and exec):

run-external
# => Error: nu::shell::missing_parameter
# => 
# =>   × Missing parameter: no command given.
# =>    ╭─[entry #2:1:1]
# =>  1 │ run-external
# =>    · ──────┬─────
# =>    ·       ╰── missing parameter: no command given
# =>    ╰────
# => 

let command = ["cat" "hello.txt"]
run-external ...$command
# => hello world!

User-Facing Changes

Lists can now be spread directly into run-external and exec:

let command = [cat hello.txt]
run-external ...$command
# => hello world!

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

N/A

@fdncred fdncred added notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way labels Jan 6, 2025
@132ikl
Copy link
Copy Markdown
Member Author

132ikl commented Jan 6, 2025

Actually wait, this isn't a breaking change. I'm not sure why I thought it was. The old format still works in the after the PR, since it's still just rest arguments.

let command = ["cat" "hello.txt"]
run-external ...$command
# => hello world!
run-external ($command | first) ...($command | skip 1)
# => hello world!

@132ikl 132ikl marked this pull request as draft January 7, 2025 00:48
@132ikl
Copy link
Copy Markdown
Member Author

132ikl commented Jan 9, 2025

I added test permutations in tests/commands/run_external.rs to run each test with just the binary name, with ^, and with run-external. There were a couple special cases where not all permutations could be used (notably the parser seems to handle GlobPatterns strangely, which only affects run-external), and I left a comment to explain the issue for each of these. I also verified that the updated tests caught the bug I accidentally introduced by switching SyntaxShape::GlobPattern and SyntaxShape::Any in run-external's signature (and reverted that change).

Additionally, I pulled in a new dependency to help with these tests, rstest_reuse. This crate is actively maintained directly in the main rstest repo, and it didn't pull any further dependencies which we don't already have, so hopefully this shouldn't be a major concern.

@132ikl 132ikl marked this pull request as ready for review January 9, 2025 23:06
.input_output_types(vec![(Type::Any, Type::Any)])
.required(
"command",
SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be honest I'm little worry about changing from SyntaxShape::String to SyntaxShape::Any.

However, I don't have a clue how it's going to work internally.

Copy link
Copy Markdown
Member Author

@132ikl 132ikl Jan 15, 2025

Choose a reason for hiding this comment

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

I think it should just turn these parse errors into runtime errors, but I'll test it out

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It seems like putting literals for most types in the command position just attempt to run that literal as a command anyways, which is fine. If it can't be coerced into a string it will simply error at runtime. The only difference I found was bools:

Before:

$ run-external false
Error: nu::parser::parse_mismatch_with_full_string_msg

  × Parse mismatch during operation.
   ╭─[entry #18:1:14]
 1 │ run-external false
   ·              ──┬──
   ·                ╰── expected one of a list of accepted shapes: [GlobPattern, String]
   ╰────

After:

run-external false

(runs false executable)

Seems like the after behavior is preferable here.

@132ikl 132ikl marked this pull request as draft January 15, 2025 17:35
@132ikl 132ikl marked this pull request as ready for review January 15, 2025 20:15
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@WindSoilder WindSoilder merged commit 0693865 into nushell:main Jan 15, 2025
@github-actions github-actions bot added this to the v0.102.0 milestone Jan 15, 2025
@132ikl 132ikl removed the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants