remove let-env, focus on mutating $env

[sophiajt]

Description

For years, Nushell has used let-env to set a single environment variable. As our work on scoping continued, we refined what it meant for a variable to be in scope using let but never updated how let-env would work. Instead, let-env confusingly created mutations to the command's copy of $env.

So, to help fix the mental model and point people to the right way of thinking about what changing the environment means, this PR removes let-env to encourage people to think of it as updating the command's environment variable via mutation.

Before:

let-env FOO = "BAR"

Now:

$env.FOO = "BAR"

It's also a good reminder that the environment owned by the command is in the $env variable rather than global like it is in other shells.

User-Facing Changes

BREAKING CHANGE BREAKING CHANGE

This completely removes let-env FOO = "BAR" so that we can focus on $env.FOO = "BAR".

Tests + Formatting

After / Before Submitting

integration scripts to update:

• ✔️ starship
• ✔️ virtualenv
• ✔️ atuin (PR: Update Nushell support to use supported $env update syntax atuinsh/atuin#1080)
• ✔️ zoxide (PR: Update Nushell env update syntax ajeetdsouza/zoxide#587)
• ✔️ oh-my-posh (pr: chore: update Nushell support to use $env updates JanDeDobbeleer/oh-my-posh#4011)

[amtoine]

for now it looks good to me 👌

of course this breaks integration with other tools 👍

[amtoine]

@jntrnr, the failing test is here in test_std test_path_add, looks like the behaviour of $env.FUIQW = ... has changed 🤔

[sophiajt]

Looks like starship and virtual already use load-env and aren't affected.

[sophiajt]

atuin looks like it will break: https://github.com/ellie/atuin/blob/main/atuin/src/shell/atuin.nu

[amtoine]

i do not understand why we need to use load-env in the last commit 😕

[sophiajt]

@amtoine because the path variable is coming from a variable. $env.$FOO = ... isn't supported, only $env.FOO = ...

[amtoine]

@jntrnr, i've added the integration scripts above to the body of the PR 👌

[amtoine]

@amtoine because the path variable is coming from a variable. $env.$FOO = ... isn't supported, only $env.FOO = ...

oooooh very good catch 😱 👏 👏
i did not even see the $ 👀

[sophiajt]

Zoxide looks like it will also break: https://github.com/ajeetdsouza/zoxide/blob/main/templates/nushell.txt

[amtoine]

i propose to make this a draft to avoid merging the PR before we have fixed the integration scripts 😮

[amtoine]

maybe LetEnvDeprecated should go inside crates/nu-command/src/deprecated/ as the other deprecated commands? 🤔

[amtoine]

looks about right to me... LET'S BREAK IT 💪 😤

[hyiltiz]

This is a great change, thank you! I was getting quite confused, but this also breaks quite some scripts. Since the fix is mechanical, would it be meaningful to produce script.nu.orig -> script.nu by applying the let-env FOO = ... -> $env.FOO = ... conversion automatically to help ease the BREAKING-CHANGE transition. Similar procedure can be used for other breaking changes as well.

[amtoine]

This is a great change, thank you! I was getting quite confused, but this also breaks quite some scripts. Since the fix is mechanical, would it be meaningful to produce script.nu.orig -> script.nu by applying the let-env FOO = ... -> $env.FOO = ... conversion automatically to help ease the BREAKING-CHANGE transition. Similar procedure can be used for other breaking changes as well.

a few commands i know that should do this migration 😋

• sed -i 's/let-env /$env./' ...
• sd 'let-env ' '$env.' ...
• open ... | str replace --all --string 'let-env ' '$env.' | save --force ...

i did not try them but i think they should do it 😋

[hyiltiz]

I had an alias alias "go home" = let-env JAVA_HOME = (/usr/libexec/java_home -a x86_64 -v 11) before. After simply rewriting as alias "go home" = $env.JAVA_HOME = (/usr/libexec/java_home -a x86_64 -v 11), I get the helpful message that aliasing BinaryOp is not supported (I really hope aliases are agnostic to the content of what they are being aliased to.)

I can probably work around this by defining go home as a function instead in general, and maybe with the load-env trick, but aliasing env variable updates probably should be possible in a more straightforward way.

$ # create alias then test
$ # ---------------------
$ alias set-py =       $env.PY = (which python)
$ alias set-py =      ($env.PY = (which python))
$ alias set-py =      {$env.PY = (which python)}
$ # all of the above are invalid syntax
$ 
$ alias set-py = do {$env.PY = (which python)}
$ set-py  # should create $env.PY
$ $env.PY # no errors but also does not work since closures don't mutate
$
$ # the one below works as a workaround, though maybe a bit ugly
$ alias set-py = load-env {XY: (which python)}
$ set-py # no error here
$ $env.XY # works!

[amtoine]

@hyiltiz
yeah, aliases should be command calls, this is why $env.PY = ... does not work. and do { ... } does not work either because i'd say do is a bit like a keyword?

load-env is a great solution 👌
you can also write a custom def-env command for such things 😋

[hyiltiz]

Is there a benefit for defining it as a command vs. alias, except the fact that in a command there is much larger flexibility to do arbitrarily complex things (may not be desirable since by alias I meant to show the shallowness of implementation).

I'll just use load-env and maybe convert them to def-env if needs arise. Thanks, @amtoine!

[amtoine]

Is there a benefit for defining it as a command vs. alias, except the fact that in a command there is much larger flexibility to do arbitrarily complex things (may not be desirable since by alias I meant to show the shallowness of implementation).

maybe @kubouch might have more information about the aliases as he rewrote them completely not so long ago, but i'd say that by design aliases do alias to commands, not language constructs.
e.g. you can alias ll = ls -l because ls -l is a simple command call but you cannot alias foo = if ... because this is not a command 🤔

I'll just use load-env and maybe convert them to def-env if needs arise. Thanks, @amtoine!

yeah, as load-env is a command, it does the trick 😏

[EnricoGhiorzi]

This did break compatibility with some other tools (I experience it with Starship) because the updated scripts have not been included into a release yet (not to mention it required hand-fixing the env and config files). I guess it's too late now, but instead of turning the old sytnax into a hard error it would have been better to deprecate it (as in, just rise a warning) for some time. Hopefully this will fix itself as the new scripts are released. Overall, not a great user experience.

[amtoine]

This did break compatibility with some other tools (I experience it with Starship) because the updated scripts have not been included into a release yet (not to mention it required hand-fixing the env and config files).

yeah, the fix i wrote on the Starship side is ahead the latest release by 7 commits 😭

I guess it's too late now, but instead of turning the old sytnax into a hard error it would have been better to deprecate it (as in, just rise a warning) for some time.

the deprecation system definitely needs love 😌
we will work on this in #9812!

Hopefully this will fix itself as the new scripts are released.

i can confirm you it will 👌
i build Starship from source and it works like a charm with latest release!

Overall, not a great user experience.

again, sorry for the inconvenience...
we are working on release a patch as soon as possible 🤞