add function to make env vars case-insensitive #14390
Conversation
There was a problem hiding this comment.
Would you mind using str::eq_ignore_ascii_case instead of str::to_lowercase? This avoids creating a bunch of new strings. Alternatively, you should use str::eq_ignore_case depending on whether Windows is ASCII case insensitive or Unicode case insensitive for environment variables.
I"m not sure which of these is better. I chose to_lowercase because, from what I read, it works on ascii and unicode. So, I figured the broader application was better. I can look into eq_ignore_case, but I"ve never used it. |
Yeah, I"m not sure either. In cmd or powershell, do the env vars |
I can check but my PR is really about Path PATH path PaTh and I tried to be particular about only changing the functions that dealt with that vs making all env vars case insensitive. |
fdncred
force-pushed
the
path_insensitive
branch
from
32231cb to
31d3e37
Compare
|
From within Nushell, all reads of env vars are case insensitive now, right? Is there ever any reason to want to check env vars case sensitively from inside the codebase? |
Maybe internally but not at startup apparently, because if you"re on Windows and your HKLM is Path and your HKCU is path things break, or you get half your paths, weirdness ensues. I think this may be only at startup though, which is why I tried to change only those parts. All other internal reading of env vars may be case insensitive. I know I can do $env.path/PATH/PaTh on windows and it reads fine. Seems like some folks don"t want case-insensitive everything env vars but I"m not sure why. |
|
We need to dogfood this a bit, let"s land and do that. |
# Description Fixes multiple issues related to `ENV_CONVERSION` and path-conversion-to-list. * #14681 removed some calls to `convert_env_values()`, but we found that this caused `nu -n` to no longer convert the path properly. * `ENV_CONVERSIONS` have apparently never preserved case, meaning a conversion with a key of `foo` would not update `$env.FOO` but rather create a new environment variable with a different case. * There was a partial code-path that attempted to solve this for `PATH`, but it only worked for `PATH` and `Path`. * `convert_env_values()`, which handled `ENV_CONVERSIONS` was called in multiple places in the startup depending on flags. This PR: * Refactors the startup to handle the conversion in `main()` rather than in each potential startup path * Updates `get_env_var_insensitive()` functions added in #14390 to return the name of the environment variable with its original case. This allows code that updates environment variables to preserve the case. * Makes use of the updated function in `ENV_CONVERSIONS` to preserve the case of any updated environment variables. The `ENV_CONVERSION` key itself is still case **insensitive**. * Makes use of the updated function to preserve the case of the `PATH` environment variable (normally handled separately, regardless of whether or not there was an `ENV_CONVERSION` for it). ## Before `env_convert_values` was run: * Before the user `env.nu` ran, which included `nu -c <commandstring>` and `nu <script.nu>` * Before the REPL loaded, which included `nu -n` ## After `env_convert_values` always runs once in `main()` before any config file is processed or the REPL is started # User-Facing Changes Bug fixes # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` Added additional tests to prevent future regression. # After Submitting There is additional cleanup that should probably be done in `convert_env_values()`. This function previously handled `ENV_CONVERSIONS`, but there is no longer any need for this since `convert_env_vars()` runs whenever `$env.ENV_CONVERSIONS` changes now. This means that the only relevant task in the old `convert_env_values()` is to convert the `PATH` to a list, and ensure that it is a list of strings. It"s still calling the `from_string` conversion on every variable (just once) even though there are no `ENV_CONVERSIONS` at this point. Leaving that to another PR though, while we get the core issue fixed with this one.
Description
This PR adds a new function that allows one to get an env var case-insensitively. I did this so we can hopefully stop having problems when Windows has HKLM as path and HKCU as Path.
Instead of just changing every function that used the original one, I chose the ones that I thought were specific to getting the path. I didn"t want to go all in and make every env get case insensitive, but maybe we should? 🤷🏻♂️
closes #12676
User-Facing Changes
Tests + Formatting
After Submitting