-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add basic support for nushell (including on Windows) #58413
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.
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
base: master
Are you sure you want to change the base?
Conversation
Replace regex with explicit replace. Remove @static macro and use `Sys.iswindows()` instead Co-authored-by: Jakob Nybo Nissen <[email protected]>
base/client.jl
Outdated
@static if !Sys.iswindows() | ||
if shell_name == "nu" | ||
# remove backticks and apostrophes that dont play nice with nushell | ||
shell_escape_cmd = replace(shell_escape(cmd), "'" => "", "`" => "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with nushell, but how does this not change the argument semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like mainly it makes them wrong. You're supposed to parse the cmd string with special=Base.shell_special
if you want this behavior, and the REPL does not (currently) do that, so it is expected (currently) to output the string instead unchanged:
shell> echo hi | cat
hi | cat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I'm not sure I fully understand your comment, but I will do my best to respond.
It looks like removing backticks was a mistake by me, and is actually not required (i've update that), but removing apostrophes seems to be required.
If I change to the following:
if shell_name == "nu"
println("just shell_escape(cmd):\t", shell_escape(cmd))
shell_escape_cmd = shell_escape(cmd)
shell_escape_cmd = "try { $shell_escape_cmd } catch { |err| \$err.rendered }"
cmd = `$shell -c $shell_escape_cmd`
# ...
Then most examples work well, except when the cmd contains "
or $
.
Here are 2 examples:
shell> let x = 5; print \$\"The value of x is (\$x)\"
just shell_escape(cmd): let x = 5; print '$"The' value of x is '($x)"'
$"The
value
of
x
is
($x)"
shell> \"hello\" | save test.txt -f
just shell_escape(cmd): '"hello"' | save test.txt -f
shell> open test.txt
just shell_escape(cmd): open test.txt
"hello"
whereas in nushell, what we would be expecting is
> let x = 5; print $"The value of x is ($x)"
The value of x is 5
> "hello" | save test.txt -f
> open test.txt
hello
If there is a better way to achieve this result, please let me know and I will try and amend it :)
This PR adds basic support for Nushell in shell mode. Should fix #54291.
I have also added this capability for Windows systems, although I know there is a long and ongoing discussion about how best to support shell-model on Windows (see #23597), so I'm happy to leave this out of the PR if that's preferred. However, I am getting exactly the same functionality on Linux and Windows :)
Here are some examples:
Linux
Windows
Limitations: Nushell is not POSIX-compliant, so there are some characters that the user must escape manually in order for them to carry through the Julia backtick syntax and
shell_escape(...)
function. As far as I can tell, the only times a user might came across this is for'
,"
and$
. The obvious workaround is to just escape them:Thanks!