Run ENV_CONVERSIONS whenever it's modified #14591
Conversation
| ) -> Result<(), ShellError> { | ||
| let conversions = conversions.as_record()?; | ||
| for (key, conversion) in conversions.into_iter() { | ||
| if let Some(val) = stack.get_env_var_insensitive(engine_state, key) { |
There was a problem hiding this comment.
I'm not objecting to this but I'm wondering if all these should be insensitive. I know Path should but I'm not sure if there's any harm having all of them case insensitive. Thoughts?
There was a problem hiding this comment.
I chose to make it insensitive to make it consistent with how Instruction::LoadEnv loads environment variables.
nushell/crates/nu-engine/src/eval_ir.rs
Line 357 in baf86df
There was a problem hiding this comment.
People complaining about http_proxy and HTTP_PROXY is really why I was asking. Leaving it this way at least is not a breaking change. Thanks.
|
Thoughts on landing this before or after 0.101? I'm personally leaning towards after to give this one more time to dogfood, but if folks feel it can go in now, I'm okay with that, too. |
|
I'm on the fence. I could be talked into waiting or landing (once the ci is green again). |
|
I feel like the implementation can (and should) be refined but the core of the approach (hooking into the IR evaluation) is solid. I'm personally in favor of landing it as soon as possible because the current behavior is really unintuitive, where you either
If any issues come up it can just be reverted before the release. |
|
@Bahex i think we're too close to the release to land this. I think we're going to try and release sunday. |
Bahex
force-pushed
the
env_conversion_on_access
branch
from
de6ea84 to
5d558d1
Compare
|
@Bahex I'm working on getting rid of the older calls, and I noticed that the Overlay logic here doesn't seem to be present in your |
|
@NotTheDr01ds |
|
@Bahex Thanks! I also just noticed that Yours is more focused and just deals with the variables in the record, it seems. |
# 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
$env.ENV_CONVERSIONSchanges take place immediately #14514Description
Makes updates to
$env.ENV_CONVERSIONStake effect immediately.User-Facing Changes
No breaking change,
$env.ENV_CONVERSIONScan be set and its effect used in the same file.Tests + Formatting
After Submitting
N/A