From 2388e1e80b06cc25b530d7b561f8f69dd882c413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Thu, 24 Nov 2022 23:45:24 +0100 Subject: [PATCH] Reorder export-env eval and allow reloading an overlay (#7231) # Description This PR is a response to the issues raised in https://github.com/nushell/nushell/pull/7087. It consists of two changes: * `export-env`, when evaluated in `overlay use`, will see the original environment. Previously, it would see the environment from previous overlay activation. * Added a new `--reload` flag that reloads the overlay. Custom definitions will be kept but the original definitions and environment will be reloaded. This enables a pattern when an overlay is supposed to shadow an existing environment variable, such as `PROMPT_COMMAND`, but `overlay use` would keep loading the value from the first activation. You can easily test it by defining a module ``` module prompt { export-env { let-env PROMPT_COMMAND = (date now | into string) } } ``` Calling `overlay use prompt` for the first time changes the prompt to the current time, however, subsequent calls of `overlay use` won't change the time. That's because overlays, once activated, store their state so they can be hidden and restored at later time. To force-reload the environment, use the new flag: Calling `overlay use --reload prompt` repeatedly now updates the prompt with the current time each time. # User-Facing Changes * When calling `overlay use`, if the module has an `export-env` block, the block will see the environment as it is _before_ the overlay is activated. Previously, it was _after_. * A new `overlay use --reload` flag. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- .../src/core_commands/overlay/use_.rs | 18 +++-- crates/nu-parser/src/parse_keywords.rs | 19 ++++-- tests/overlays/mod.rs | 67 +++++++++++++++++++ 3 files changed, 93 insertions(+), 11 deletions(-) diff --git a/crates/nu-command/src/core_commands/overlay/use_.rs b/crates/nu-command/src/core_commands/overlay/use_.rs index aff8747e29..4c541882ba 100644 --- a/crates/nu-command/src/core_commands/overlay/use_.rs +++ b/crates/nu-command/src/core_commands/overlay/use_.rs @@ -37,6 +37,11 @@ impl Command for OverlayUse { "Prepend module name to the imported commands and aliases", Some('p'), ) + .switch( + "reload", + "If the overlay already exists, reload its definitions and environment.", + Some('r'), + ) .category(Category::Core) } @@ -59,7 +64,7 @@ impl Command for OverlayUse { let mut name_arg: Spanned = call.req(engine_state, caller_stack, 0)?; name_arg.item = trim_quotes_str(&name_arg.item).to_string(); - let origin_module_id = if let Some(overlay_expr) = call.positional_nth(0) { + let maybe_origin_module_id = if let Some(overlay_expr) = call.positional_nth(0) { if let Expr::Overlay(module_id) = overlay_expr.expr { module_id } else { @@ -114,9 +119,7 @@ impl Command for OverlayUse { )); }; - caller_stack.add_overlay(overlay_name); - - if let Some(module_id) = origin_module_id { + if let Some(module_id) = maybe_origin_module_id { // Add environment variables only if: // a) adding a new overlay // b) refreshing an active overlay (the origin module changed) @@ -152,6 +155,9 @@ impl Command for OverlayUse { call.redirect_stderr, ); + // The export-env block should see the env vars *before* activating this overlay + caller_stack.add_overlay(overlay_name); + // Merge the block's environment to the current stack redirect_env(engine_state, caller_stack, &callee_stack); @@ -159,7 +165,11 @@ impl Command for OverlayUse { // Remove the file-relative PWD, if the argument is a valid path caller_stack.remove_env_var(engine_state, "FILE_PWD"); } + } else { + caller_stack.add_overlay(overlay_name); } + } else { + caller_stack.add_overlay(overlay_name); } Ok(PipelineData::new(call.head)) diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index a413b89904..fcc88ac1fe 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -2441,6 +2441,7 @@ pub fn parse_overlay_use( }; let has_prefix = call.has_flag("prefix"); + let do_reload = call.has_flag("reload"); let pipeline = Pipeline::from_vec(vec![Expression { expr: Expr::Call(call.clone()), @@ -2498,7 +2499,7 @@ pub fn parse_overlay_use( let module_id = overlay_frame.origin; if let Some(new_module_id) = working_set.find_module(overlay_name.as_bytes()) { - if module_id == new_module_id { + if !do_reload && (module_id == new_module_id) { (overlay_name, Module::new(), module_id, false) } else { // The origin module of an overlay changed => update it @@ -2594,13 +2595,17 @@ pub fn parse_overlay_use( } }; - let (decls_to_lay, aliases_to_lay) = if has_prefix { - ( - origin_module.decls_with_head(final_overlay_name.as_bytes()), - origin_module.aliases_with_head(final_overlay_name.as_bytes()), - ) + let (decls_to_lay, aliases_to_lay) = if is_module_updated { + if has_prefix { + ( + origin_module.decls_with_head(final_overlay_name.as_bytes()), + origin_module.aliases_with_head(final_overlay_name.as_bytes()), + ) + } else { + (origin_module.decls(), origin_module.aliases()) + } } else { - (origin_module.decls(), origin_module.aliases()) + (vec![], vec![]) }; working_set.add_overlay( diff --git a/tests/overlays/mod.rs b/tests/overlays/mod.rs index a39b0f4fbb..75d75fb813 100644 --- a/tests/overlays/mod.rs +++ b/tests/overlays/mod.rs @@ -1055,3 +1055,70 @@ fn overlay_trim_double_quote_hide() { #[cfg(not(windows))] assert!(!actual_repl.err.is_empty()); } + +#[test] +fn overlay_use_and_restore_older_env_vars() { + let inp = &[ + r#"module spam { + export-env { + let old_baz = $env.BAZ; + let-env BAZ = $old_baz + 'baz' + } + }"#, + r#"let-env BAZ = 'baz'"#, + r#"overlay use spam"#, + r#"overlay hide spam"#, + r#"let-env BAZ = 'new-baz'"#, + r#"overlay use --reload spam"#, + r#"$env.BAZ"#, + ]; + + let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; "))); + let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp)); + + assert_eq!(actual.out, "new-bazbaz"); + assert_eq!(actual_repl.out, "new-bazbaz"); +} + +#[test] +fn overlay_use_and_reload() { + let inp = &[ + r#"module spam { + export def foo [] { 'foo' }; + export alias fooalias = 'foo'; + export-env { + let-env FOO = 'foo' + } + }"#, + r#"overlay use spam"#, + r#"def foo [] { 'newfoo' }"#, + r#"alias fooalias = 'newfoo'"#, + r#"let-env FOO = 'newfoo'"#, + r#"overlay use --reload spam"#, + r#"$'(foo)(fooalias)($env.FOO)'"#, + ]; + + let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; "))); + let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp)); + + assert_eq!(actual.out, "foofoofoo"); + assert_eq!(actual_repl.out, "foofoofoo"); +} + +#[test] +fn overlay_use_and_reolad_keep_custom() { + let inp = &[ + r#"overlay new spam"#, + r#"def foo [] { 'newfoo' }"#, + r#"alias fooalias = 'newfoo'"#, + r#"let-env FOO = 'newfoo'"#, + r#"overlay use --reload spam"#, + r#"$'(foo)(fooalias)($env.FOO)'"#, + ]; + + let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; "))); + let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp)); + + assert_eq!(actual.out, "newfoonewfoonewfoo"); + assert_eq!(actual_repl.out, "newfoonewfoonewfoo"); +}