Restore tilde expansion on external command names #13001
Conversation
|
Running through some quick scenarios and stumbled upon this one that I think should work but doesn't. We should be able to construct a path to an external and execute it, I believe. let bin = "pastel" #<- fill this in with whatever bin you have in your .cargo/bin
❯ ^$"~/.cargo/bin/($bin)"
Error: nu::shell::external_command
× External command failed
╭─[entry #7:1:2]
1 │ ^$"~/.cargo/bin/($bin)"
· ───────────┬──────────
· ╰── Command `~/.cargo/bin/pastel` not found
╰────
help: `~/.cargo/bin/pastel` is neither a Nushell built-in or a known external commandbut this works fine ❯ ~/.cargo/bin/pastel
pastel 0.9.0
A command-line tool to generate, analyze, convert and manipulate colors |
|
Related to the PR but not my last comment necessarily, changing quoting rules kind of worries me. As I recall, windows/cmd has some very strange quoting scenarios. Not sure about other operating systems. What I've learned about quoting is it's less about doing what's "right" and more about doing what other commands expect like cmd.exe, bash, etc. |
|
We don't usually expand tilde and glob on quoted strings, and interpolated strings count. So I think it's most consistent this way. For example: > ^echo ~/.cargo/bin/exa
/home/devyn/.cargo/bin/exa
> ^echo "~/.cargo/bin/exa"
~/.cargo/bin/exa
> ^echo $"~/.cargo/bin/exa"
~/.cargo/bin/exaI just changed it to work the same way the other arguments do (using the same code), so if there's a bug there (e.g. with external escaping) it'll also be a bug here, but we also only have one place to fix it. |
|
If it's an interpolated string (I think) we need to expand it. This is a very common thing in nushell. Constructing a path to an executable so you can run it. |
|
Hmm, but it isn't the same behaviour we use for arguments. In that case, should I just change it so that it doesn't attempt to detect bare strings at all, and always expands? |
|
With regards to my previous comment about string interpolation and expansion, I'm fine with going along with those changes. And since devyn assures me that these quoting rules don't touch things like cmd.exe's builtin mklink, I'm good to go here. |
Do we really need to do that? Ideally, we want to be removing hacks like this, not adding more of it. Expanding the command name is just different from expanding arguments, and we don't have to force them to be the same. For example, we perform glob expansion on arguments, but it doesn't make sense to perform glob expansion on the command name.
I agree. |
)" This reverts commit 0e15530.
Right, I completely agree. I just want something that has the behavior we want for a patch release, and then we can try to make it less hacky after. This seemed like the most consistent option (for now) even though it's being more consistently janky. |
…he parser (#13089) # Description We've had a lot of different issues and PRs related to arg handling with externals since the rewrite of `run-external` in #12921: - #12950 - #12955 - #13000 - #13001 - #13021 - #13027 - #13028 - #13073 Many of these are caused by the argument handling of external calls and `run-external` being very special and involving the parser handing quoted strings over to `run-external` so that it knows whether to expand tildes and globs and so on. This is really unusual and also makes it harder to use `run-external`, and also harder to understand it (and probably is part of the reason why it was rewritten in the first place). This PR moves a lot more of that work over to the parser, so that by the time `run-external` gets it, it's dealing with much more normal Nushell values. In particular: - Unquoted strings are handled as globs with no expand - The unescaped-but-quoted handling of strings was removed, and the parser constructs normal looking strings instead, removing internal quotes so that `run-external` doesn't have to do it - Bare word interpolation is now supported and expansion is done in this case - Expressions typed as `Glob` containing `Expr::StringInterpolation` now produce `Value::Glob` instead, with the quoted status from the expr passed through so we know if it was a bare word - Bare word interpolation for values typed as `glob` now possible, but not implemented - Because expansion is now triggered by `Value::Glob(_, false)` instead of looking at the expr, externals now support glob types # User-Facing Changes - Bare word interpolation works for external command options, and otherwise embedded in other strings: ```nushell ^echo --foo=(2 + 2) # prints --foo=4 ^echo -foo=$"(2 + 2)" # prints -foo=4 ^echo foo="(2 + 2)" # prints (no interpolation!) foo=(2 + 2) ^echo foo,(2 + 2),bar # prints foo,4,bar ``` - Bare word interpolation expands for external command head/args: ```nushell let name = "exa" ~/.cargo/bin/($name) # this works, and expands the tilde ^$"~/.cargo/bin/($name)" # this doesn't expand the tilde ^echo ~/($name)/* # this glob is expanded ^echo $"~/($name)/*" # this isn't expanded ``` - Ndots are now supported for the head of an external command (`^.../foo` works) - Glob values are now supported for head/args of an external command, and expanded appropriately: ```nushell ^("~/.cargo/bin/exa" | into glob) # the tilde is expanded ^echo ("*.txt" | into glob) # this glob is expanded ``` - `run-external` now works more like any other command, without expecting a special call convention for its args: ```nushell run-external echo "'foo'" # before PR: 'foo' # after PR: foo run-external echo "*.txt" # before PR: (glob is expanded) # after PR: *.txt ``` # Tests + Formatting Lots of tests added and cleaned up. Some tests that weren't active on Windows changed to use `nu --testbin cococo` so that they can work. Added a test for Linux only to make sure tilde expansion of commands works, because changing `HOME` there causes `~` to reliably change. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting - [ ] release notes: make sure to mention the new syntaxes that are supported
Description
Fix a regression introduced by #12921, where tilde expansion was no longer done on the external command name, breaking things like
This properly handles quoted strings, so they don't expand:
This required a change to the parser, so the command name is also parsed in the same way the arguments are - i.e. the quotes on the outside remain in the expression. Hopefully that doesn't break anything else. 🤞
Fixes #13000. Should include in patch release 0.94.1
cc @YizhePKU
User-Facing Changes
commandofrun-externalwill now have its quotes removed like the other arguments if it is a literal stringExternalCallif they were presentTests + Formatting
I would like to add a regression test for this, but it's complicated because we need a well-known binary within the home directory, which just isn't a thing. We could drop one there, but that's kind of a bad behavior for a test to do. I also considered changing the home directory for the test, but that's so platform-specific - potentially could get it working on specific platforms though. Changing
HOMEenv on Linux definitely works as far as tilde expansion works.toolkit fmttoolkit clippytoolkit testtoolkit test stdlib