-
Notifications
You must be signed in to change notification settings - Fork 320
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support nushell on linux #1606
base: master
Are you sure you want to change the base?
Conversation
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.
This will break other shells because --
here doesn't mean to treat the remaining arguments as positional arguments. It is a placeholder which gets assigned to $0
when executing the script, and everything after will be assigned to $1
, $2
, etc. as expected. Try running the following command and see what happens:
bash -c 'echo "$@"' foo bar baz
You can also test this by adding the following to your lfrc
file:
cmd mkdir %mkdir "$@"
Unfortunately Nushell isn't designed to be a POSIX-compatible shell, and I think the only way is to add shell-specific code similar to what is done for Windows CMD:
Lines 104 to 117 in cf99626
// Windows CMD requires special handling to deal with quoted arguments | |
if strings.ToLower(gOpts.shell) == "cmd" { | |
var builder strings.Builder | |
builder.WriteString(s) | |
for _, arg := range args { | |
fmt.Fprintf(&builder, ` "%s"`, arg) | |
} | |
shellOpts := strings.Join(gOpts.shellopts, " ") | |
cmdline := fmt.Sprintf(`%s %s %s "%s"`, gOpts.shell, shellOpts, gOpts.shellflag, builder.String()) | |
cmd := exec.Command(gOpts.shell) | |
cmd.SysProcAttr = &windows.SysProcAttr{CmdLine: cmdline} | |
return cmd | |
} |
EDIT: Actually I suspect the following change might be OK too. If it works then I would prefer to avoid having custom code just to handle a specific shell.
diff --git a/os.go b/os.go
index 962bb41..ccddb75 100644
--- a/os.go
+++ b/os.go
@@ -139,7 +139,7 @@ func shellCommand(s string, args []string) *exec.Cmd {
s = fmt.Sprintf("IFS='%s'; %s", gOpts.ifs, s)
}
- args = append([]string{gOpts.shellflag, s, "--"}, args...)
+ args = append([]string{gOpts.shellflag, s, gOpts.shell}, args...)
args = append(gOpts.shellopts, args...)
Also please add something like |
Maybe we could add a new option like |
Sorry for randomly popping in here. I just wanted to chime in here to note that maybe you could take notes from how I think what would be desired here is to have a new option for positional arguments (e.g., |
@klesh I'm not sure if you understood my first comment, but to be more clear: With your changes the command will still run but the arguments will be shifted by one, that is the first argument will now be assigned to When I suggested to replace
@jameschensmith I guess we can consider adding an option (or just check the value of Correct me if I'm wrong on any of this, I'm not an expert on using Nushell. |
@joelim-work Thanks for the clarification, I believe I understood your comment about the argument shifting. However, I would recommend adding an option instead of checking the |
In command mode ( Because Nushell doesn't support positional arguments (i.e., |
@jameschensmith Aah, I see, you are correct. So, there are a couple of ways to achieve the goal:
|
So I had a look at the fish -c 'mkdir $argv' foo bar And the corresponding
Based on this, I think I agree with adding an option to control this behavior. The only question is, does it make sense to prevent adding As for the option name, I haven't really given it much thought, but I think it should start with |
How about |
@klesh Sorry I didn't understand your comment, could you please write some code/pseudocode to explain? |
Oh, sorry for the mess. Here is the idea, take the
|
Thanks for the clarification. In that case I have some questions about
I think your intended answer here is to not use
Now you can't add arguments onto the end of the command string. Also you make the assumption the arguments will be used together, when instead the command could theoretically use |
@joelim-work Aah, the case is legit, it didn't occured to me because I would write a script for the case which is easier and can be used both in |
|
I think that could work, to allow the following possibilities:
The problem with the third scenario is that One other idea I have is, instead of using
#!/bin/sh
# get rid of '-c' flag
shift
# create script file
echo "$1" > /tmp/lfcmd.nu
shift
# get rid of '--' flag
shift
# invoke nu on script file
nu /tmp/lfcmd.nu "$@" It is a bit hacky, but it does allow arguments to be passed and also doesn't require changes to |
I agree with you that However, your solution won't work for me because the reason I use |
Well theoretically you could add a customized The problem is that the supporting work has to be done somewhere, whether it is in I am generally more conservative when it comes to adding new features/options, but I think it's worth to ping @gokcehan for an additional opinion. It may be possible that we can add such a |
I agree it is not perfect, but I would like to add that it is possible to pass arguments for
but I don't think it is a big deal, one can write a script file to avoid it. Is it safe to assume the shell would be POXIS-compliant when CrossPlatform is a main feature of the project? Especially when |
@joelim-work Speaking of which, would you like to share your way of writing this |
I originally had another idea to not include the
But it looks like there's no way to avoid adding Also my Windows enviornment is borked so I don't really have on anymore. But theoretically #!/usr/bin/env nu
def --wrapped main [_, script: string, _, ...args: string] {
let file = '/tmp/lfcmd.nu'
$script | save --force $file
nu $file ...$args
} Anyway I saw you create an issue in the |
@joelim-work Thanks for the information. |
Thanks for being understanding - I think it is important to spend time investigating what is the best way to support Nushell integration, instead of committing to a potentially incomplete solution right now. For now I will remove my Changes requested status and convert the PR to a draft, as this change is still under discussion. |
No problem. |
I am also interested in this use case. Are there any new results? |
@msMatix I ended up writing a wrapper in Golang like @joelim-work suggested. |
ah I see. Thank you! |
@klesh TBH I don't think In the meantime, it would be good if you can add your wrapper somewhere in the wiki (perhaps as an integration) so that other |
@joelim-work It was written in Golang and has a couple of files, not sure it is suitable for the wiki. |
What it does
Fixes #1605
Why it happend
nushell
doesn't accept--
at all, it would throw an exception when it was givenHow does the fix work
Remove the
--
argumentWill it cause other problems?
I don't think so, the
--
forsh
is to tell it that the rest of the arguments are meant for the command to be executed, which is not useful in this case because there is only one argument left.I might be wrong though, 馃槉