Skip to content

Remove no-longer-needed convert_env_values calls#14681

Merged
fdncred merged 2 commits intonushell:mainfrom
NotTheDr01ds:remove-env-conversion-calls
Jan 2, 2025
Merged

Remove no-longer-needed convert_env_values calls#14681
fdncred merged 2 commits intonushell:mainfrom
NotTheDr01ds:remove-env-conversion-calls

Conversation

@NotTheDr01ds
Copy link
Copy Markdown
Contributor

@NotTheDr01ds NotTheDr01ds commented Dec 27, 2024

Description

Takes advantage of #14591 to remove the now-necessary calls to convert_env_values() that I added in #14249. The function is now just called once to convert PATH.

Also removed the Windows-build-time checks for ensure_path, since previous case-insensitivity fixes make this unnecessary as well.

User-Facing Changes

None - #14591 now handles conversion 'on-demand'.

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

N/A

@NotTheDr01ds NotTheDr01ds force-pushed the remove-env-conversion-calls branch from 916313d to c90177b Compare December 27, 2024 03:58
@NotTheDr01ds NotTheDr01ds deleted the remove-env-conversion-calls branch January 1, 2025 23:29
@NotTheDr01ds NotTheDr01ds restored the remove-env-conversion-calls branch January 2, 2025 00:25
@NotTheDr01ds NotTheDr01ds reopened this Jan 2, 2025
@NotTheDr01ds NotTheDr01ds requested a review from fdncred January 2, 2025 13:24
@NotTheDr01ds
Copy link
Copy Markdown
Contributor Author

@Bahex Does this look good to you, given your change in #14591?

@Bahex
Copy link
Copy Markdown
Member

Bahex commented Jan 2, 2025

PATH is explicitly handled and anything else is set in nu script by the user, which goes through the IR eval.
Looks good as far as I can tell.

@fdncred fdncred merged commit 0d3f76d into nushell:main Jan 2, 2025
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 2, 2025

Thanks

@github-actions github-actions bot added this to the v0.102.0 milestone Jan 2, 2025
fdncred added a commit that referenced this pull request Jan 6, 2025
fdncred pushed a commit that referenced this pull request Jan 10, 2025
# 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.
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.

3 participants