Skip to content
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

feat: Add Nushell support #389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

colececil
Copy link

@colececil colececil commented Jan 4, 2025

Implements #207

Though this implementation includes everything that needs done according to my understanding, it still doesn't seem to be working completely. I'd appreciate some help in figuring out what I might be missing. I believe the environment variables that are passed in to Export are being set correctly in the pre_prompt hook, but I'm not getting the expected SDKs and versions when I try to run them.

Also, note that this implementation wasn't quite as straightforward as I'd hoped, because Nushell doesn't have eval-like functionality. See https://www.nushell.sh/book/how_nushell_code_gets_run.html for more details, and see my comments below that point out how this impacts the implementation.

@@ -55,6 +55,9 @@ if (-not (Test-Path -Path $PROFILE)) { New-Item -Type File -Path $PROFILE -Force
# Or Install cmder: https://github.com/cmderdev/cmder/releases
# 2. Find script path: clink info | findstr scripts
# 3. copy internal/shell/clink_vfox.lua to script path

# For Nushell:
vfox activate nushell | save --append $nu.config-path
Copy link
Author

@colececil colececil Jan 4, 2025

Choose a reason for hiding this comment

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

Because Nushell doesn't have eval-like functionality, the output of the vfox activate nushell command needs to be added to the Nushell config file, rather than adding the command itself.

" $env.__VFOX_SHELL = 'nushell'" + env.Newline +
" $env.__VFOX_PID = $nu.pid" + env.Newline +
" ^'{{.SelfPath}}' env --cleanup | ignore" + env.Newline +
" updateVfoxEnvironment" + env.Newline +
Copy link
Author

@colececil colececil Jan 4, 2025

Choose a reason for hiding this comment

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

Because we can't run eval on vfox activate nushell, including EnvContent from activate.go here would mean it would never get updated after this script is added to the Nushell config file.

It seemed less than ideal to essentially have hard-coded environment variables in here, so I decided to leave EnvContent out and instead do the following on startup:

  1. Explicitly set __VFOX_SHELL.
  2. Call vfox env -s nushell and update the environment variables with its output (using the updateVfoxEnvironment function at the top of the script here).

I'm assuming those two things combined cover everything EnvContent would do, but please let me know if that assumption is incorrect. I'm still not clear on all the inner workings of the vfox environment stuff.

@@ -126,7 +126,7 @@ func envFlag(ctx *cli.Context) error {
return err
}

if len(sdkEnvs) == 0 {
if len(sdkEnvs) == 0 && shellName != "nushell" {
Copy link
Author

@colececil colececil Jan 4, 2025

Choose a reason for hiding this comment

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

Because we need to have the Export method generate data rather than commands for Nushell (since it doesn't have eval), I was running into issues in the case where sdkEnvs here was empty. Because the script in the Nushell config file expects data to parse, getting an empty string from vfox env -s nushell causes an error.

That's why I added the condition to prevent from returning here if the shell is Nushell. This allows Export to still be run and generate JSON for the Nushell script to consume.

Let me know if you can think of a better approach to get around this. Actually, now that I think about it again, maybe I could put a condition in the Nushell script to check if the output from vfox env -s nushell is empty before trying to parse it...

@@ -30,6 +30,8 @@ import (
"golang.org/x/sys/windows/registry"
)

const Newline = "\r\n"
Copy link
Author

@colececil colececil Jan 4, 2025

Choose a reason for hiding this comment

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

When using the backtick-style string literal for the Nushell script, running vfox activate nushell | save --append $nu.config-path would mess up the line endings in my Nushell config file. It seems that on Windows, the Nushell config file uses \r\n line endings, and the Go backtick-style string literal uses \n line endings. I haven't been able to check this, but I'm assuming Nushell uses \n line endings for its config file on macOS and Linux.

I didn't want the vfox config to mess up the config file line endings on any of the platforms, so I had to figure out a way to dynamically determine which newline style to use. The best I could think of was to just base it on which platform vfox is running on. I added this Newline constant in the platform-specific windows_env.go and macos_env.go files, but I'm not sure if this really fits into the "env" theme of these files. Let me know if there's a better place to move this to.

Activate() (string, error)

// Export generates a string that can be used by the shell to set or unset the given environment variables. (The
// input specifies environment variables to be unset by giving them a nil value.)
Copy link
Author

Choose a reason for hiding this comment

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

I thought adding some method documentation here could be helpful for new contributors in the future.

@colececil colececil marked this pull request as ready for review January 4, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant