Refactor Nushell script #1763

texastoland · 2024-03-10T19:08:09Z

Test script locally
Test saving script locally
Check unit tests
Update docs (post merge/release) Update Nushell integration mise-docs#46

Resolves the following issues:

source will eventually be deprecated
- export def main makes mise the default import from use mise.nu
mise.nu mistakenly overrides user $env.config.hooks.display_output (others too)
- proposed [stdlib-candidate] set-env nushell/nu_scripts#787 for built-in solution to $env collisions
- add-hook workaround explained below
PATH configuration more resilient in case it's a list
- using std path add API (still undocumented Generate documentation for std commands nushell/nushell.github.io#1210)

# usage
add-hook env_change.PWD {
  condition: { "MISE_SHELL" in $env }
  code: { mise_hook }
}

def --env add-hook [field hook] {
  # prepend 'hooks.' to the cell path enabling deep updates
  let field = $field | split row . | prepend hooks | into cell-path

  # get current hooks or empty list
  let hooks = $env.config | get --ignore-errors $field | default []

  # gracefully append new hook without erroring on missing cell paths
  $env.config = ($env.config | upsert $field ($hooks ++ $hook))
}

It would facilitate maintenance if the script could be extracted with include_bytes! and use vanilla str::replace instead of formatdoc! to avoid escaping braces.

Closes nushell#12148. Will simplify jdx/mise#1763. Request for discussion.

Rewrite of nushell/nushell#12156 for jdx/mise#1763. ### Why? Nushell philosophically omits a `set` list mutation. But `$env` is inherently mutable leading to issues described in nushell/nushell#12148. `set-env` provides such an operation exclusively for `$env`. ### What changed? 1. Explicit flag instead of implicit list concatenation 2. Expands updates to any `$env` field not only `$env.config` ### How is it used? ```yaml ❯ set-env -h Gracefully set an environment variable or merge a nested option. Examples: Set $env.NUPM_HOME > set-env NUPM_HOME $'($nu.home-path)/.local/share/nupm' Add to $env.NU_LIB_DIRS > set-env --append NU_LIB_DIRS $'($env.NUPM_HOME)/modules' Set a nested config option > set-env config.filesize.metric true Add a config hook > set-env -a config.hooks.pre_prompt 'ellie | print' Usage: > main {flags} <field> <value> Flags: -a, --append - Append to the previous value or wrap in a new list -h, --help - Display the help message for this command Parameters: field <cell-path>: The environment variable name or nested option cell path value <any>: The value to set or append Input/output types: ╭───┬─────────┬─────────╮ │ # │ input │ output │ ├───┼─────────┼─────────┤ │ 0 │ nothing │ nothing │ ╰───┴─────────┴─────────╯ ``` ### How does it work? ```nushell export def --env main [ field: cell-path value: any --append (-a) ]: nothing -> nothing { # just an alias def 'get or' [default field] { get --ignore-errors $field | default $default } let value = if $append { # append to the previous value or empty list $env | get or [] $field | append $value } else { $value } # work around nushell/nushell#12168 let field = $field | to text | split row . let value = match $field { [_] => $value # if cell path is nested [$root, ..$field] => { let field = $field | into cell-path # reassigning $env would be an error # merging reserved names like PWD would be an error # so merge from 1 level deep instead $env | get or {} $root | upsert $field $value } } # avoid issues noted above load-env { ($field | first): $value } } ``` ### Where are the tests? Pending next PR for nupm integration.

texastoland · 2024-03-13T18:50:35Z

@jdx Any feedback to progress? @toadslop can review if interested!

toadslop · 2024-03-16T06:54:08Z

Nice improvement!

src/shell/nushell.rs

jdx · 2024-03-16T13:57:37Z

thanks for the contribution, and sorry for the delay, I have a lot on my plate right now. That said, I think this generally looks good, I had some clarifying questions and there is a test failure probably related to snapshots that need to be updated.

texastoland · 2024-03-16T15:03:17Z

sorry for the delay, I have a lot on my plate right now.

No worries I just didn't want to leave the impression this was a drive-by PR!

+          def --env add-hook [field hook] {{

similar to below, if these are exporting global functions I wouldn't want to pollute that namespace and face potential conflicts with another function named "add-hook"

My apology this deserved more documentation! Writing it up outside review comments so it doesn't get lost for future maintenance. Nu originally used source mise.nu similar to other shells. As it evolved they adopted first-class modules like other programming environments and will ultimately deprecate the source builtin.

use mise.nu can only impact the target file in the following ways:

Only definitions (like main) marked export (like pub in Rust) are importable.
Any block in the source file wrapped in export-env can modify the current $env (shared namespace like environment variables).
Any command annotated def --env (like add-hook) can also modify the current $env.

TLDR: add_hook is local to this file.

-          def --wrapped mise [command?: string, --help, ...rest: string] {{
+          export def --wrapped main [command?: string, --help, ...rest: string] {{

are functions global in nushell? if so does that mean it would conflict with something else named main?

Files are modules and commands named main are their default export. Therefore use mise would be similar to use mise::{main as mise} in Rust.

TLDR: main is really mise and global only because it's in config.nu.

It would facilitate maintenance if the script could be extracted with include_bytes! and use vanilla str::replace instead of formatdoc! to avoid escaping braces.

I also wanted to add types to this mise command (for integration like help and completions) but it wouldn't be maintainable inlined in Rust code. Separate PR?

jdx · 2024-03-16T15:35:15Z

thanks for the explanation!

jdx · 2024-03-16T15:35:44Z

I also wanted to add types to mise (for integration like help and completions) but it wouldn't be maintainable inlined in Rust code. Separate PR?

yeah I think that's quite a bit beyond "refactor"

texastoland · 2024-03-16T16:31:34Z

Requested change complete and added related issue to the description

yeah I think that's quite a bit beyond "refactor"

Would you be open to such a change? Extract other scripts too or only Nu? Happy to continue on Discord 💬

jdx · 2024-03-16T17:29:59Z

Would you be open to such a change? Extract other scripts too or only Nu? Happy to continue on Discord 💬

possibly... though it may conflict with some of the work I'm doing on usage so I'm not sure right now

texastoland · 2024-03-16T17:34:05Z

Then I'll start with a minimal draft and continue if it plays nice with your usage ✌🏼

Depends on jdx/mise#1763 in release.

texastoland · 2024-03-17T16:10:18Z

PS ready to merge I believe and linked a docs PR when it's released

jdx · 2024-03-18T15:21:10Z

you've got a test failure

Rename Nushell param based on core team feedback

texastoland · 2024-03-18T20:50:31Z

I think I fixed the snapshot but CI says This workflow is awaiting approval from a maintainer.

* Update Nushell integration Depends on jdx/mise#1763 in release. * Automate reinstallation Manually picked from starship/starship#5845.

Rewrite of nushell/nushell#12156 for jdx/mise#1763. ### Why? Nushell philosophically omits a `set` list mutation. But `$env` is inherently mutable leading to issues described in nushell/nushell#12148. `set-env` provides such an operation exclusively for `$env`. ### What changed? 1. Explicit flag instead of implicit list concatenation 2. Expands updates to any `$env` field not only `$env.config` ### How is it used? ```yaml ❯ set-env -h Gracefully set an environment variable or merge a nested option. Examples: Set $env.NUPM_HOME > set-env NUPM_HOME $'($nu.home-path)/.local/share/nupm' Add to $env.NU_LIB_DIRS > set-env --append NU_LIB_DIRS $'($env.NUPM_HOME)/modules' Set a nested config option > set-env config.filesize.metric true Add a config hook > set-env -a config.hooks.pre_prompt 'ellie | print' Usage: > main {flags} <field> <value> Flags: -a, --append - Append to the previous value or wrap in a new list -h, --help - Display the help message for this command Parameters: field <cell-path>: The environment variable name or nested option cell path value <any>: The value to set or append Input/output types: ╭───┬─────────┬─────────╮ │ # │ input │ output │ ├───┼─────────┼─────────┤ │ 0 │ nothing │ nothing │ ╰───┴─────────┴─────────╯ ``` ### How does it work? ```nushell export def --env main [ field: cell-path value: any --append (-a) ]: nothing -> nothing { # just an alias def 'get or' [default field] { get --ignore-errors $field | default $default } let value = if $append { # append to the previous value or empty list $env | get or [] $field | append $value } else { $value } # work around nushell/nushell#12168 let field = $field | to text | split row . let value = match $field { [_] => $value # if cell path is nested [$root, ..$field] => { let field = $field | into cell-path # reassigning $env would be an error # merging reserved names like PWD would be an error # so merge from 1 level deep instead $env | get or {} $root | upsert $field $value } } # avoid issues noted above load-env { ($field | first): $value } } ``` ### Where are the tests? Pending next PR for nupm integration.

texastoland force-pushed the nushell-hooks branch 2 times, most recently from 7489ac2 to 6c90f5c Compare March 10, 2024 20:02

texastoland added a commit to texastoland/nushell that referenced this pull request Mar 11, 2024

std config add for gracefully merging deeply nested options

1b1ea08

Closes nushell#12148. Will simplify jdx/mise#1763. Request for discussion.

texastoland added a commit to texastoland/nushell that referenced this pull request Mar 11, 2024

std config add for gracefully merging deeply nested options

c883a55

Closes nushell#12148. Will simplify jdx/mise#1763. Request for discussion.

texastoland mentioned this pull request Mar 11, 2024

std config add for gracefully merging deeply nested options nushell/nushell#12156

Closed

6 tasks

texastoland added a commit to texastoland/nushell that referenced this pull request Mar 11, 2024

std config add for gracefully merging deeply nested options

f0b41f7

Closes nushell#12148. Will simplify jdx/mise#1763. Request for discussion.

texastoland mentioned this pull request Mar 12, 2024

[stdlib-candidate] set-env nushell/nu_scripts#787

Merged

jdx reviewed Mar 16, 2024

View reviewed changes

src/shell/nushell.rs Outdated Show resolved Hide resolved

jdx reviewed Mar 16, 2024

View reviewed changes

src/shell/nushell.rs Show resolved Hide resolved

jdx reviewed Mar 16, 2024

View reviewed changes

src/shell/nushell.rs Show resolved Hide resolved

texastoland mentioned this pull request Mar 16, 2024

Nushell config breaks repeated tab completion of executables on PATH #589

Closed

texastoland force-pushed the nushell-hooks branch from 2eaafe3 to e47d857 Compare March 16, 2024 16:28

jdx approved these changes Mar 16, 2024

View reviewed changes

texastoland added a commit to texastoland/mise-docs that referenced this pull request Mar 16, 2024

Update Nushell integration

03a2568

Depends on jdx/mise#1763 in release.

texastoland mentioned this pull request Mar 16, 2024

Update Nushell integration jdx/mise-docs#46

Merged

texastoland added a commit to texastoland/mise-docs that referenced this pull request Mar 16, 2024

Update Nushell integration

ec6df65

Depends on jdx/mise#1763 in release.

texastoland added a commit to texastoland/mise-docs that referenced this pull request Mar 16, 2024

Update Nushell integration

75bb6b2

Depends on jdx/mise#1763 in release.

texastoland added a commit to texastoland/mise-docs that referenced this pull request Mar 16, 2024

Update Nushell integration

a63d3b9

Depends on jdx/mise#1763 in release.

Refactor Nushell script

bade126

Rename Nushell param based on core team feedback

texastoland force-pushed the nushell-hooks branch from e47d857 to bade126 Compare March 18, 2024 20:39

Merge branch 'main' into nushell-hooks

d5762c8

jdx enabled auto-merge (squash) March 18, 2024 21:37

jdx merged commit 4f13c63 into jdx:main Mar 18, 2024
7 checks passed

texastoland mentioned this pull request Mar 21, 2024

fix(nu): Gracefully handle missing $env.config #1809

Merged

jdx pushed a commit to jdx/mise-docs that referenced this pull request Mar 22, 2024

Update Nushell integration (#46)

12b0557

* Update Nushell integration Depends on jdx/mise#1763 in release. * Automate reinstallation Manually picked from starship/starship#5845.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Nushell script #1763

Refactor Nushell script #1763

texastoland commented Mar 10, 2024 •

edited

Loading

texastoland commented Mar 13, 2024 •

edited

Loading

toadslop commented Mar 16, 2024

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024 •

edited

Loading

jdx commented Mar 16, 2024

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024 •

edited

Loading

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024

texastoland commented Mar 17, 2024

jdx commented Mar 18, 2024

texastoland commented Mar 18, 2024 •

edited

Loading

Refactor Nushell script #1763

Refactor Nushell script #1763

Conversation

texastoland commented Mar 10, 2024 • edited Loading

texastoland commented Mar 13, 2024 • edited Loading

toadslop commented Mar 16, 2024

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024 • edited Loading

jdx commented Mar 16, 2024

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024 • edited Loading

jdx commented Mar 16, 2024

texastoland commented Mar 16, 2024

texastoland commented Mar 17, 2024

jdx commented Mar 18, 2024

texastoland commented Mar 18, 2024 • edited Loading

texastoland commented Mar 10, 2024 •

edited

Loading

texastoland commented Mar 13, 2024 •

edited

Loading

texastoland commented Mar 16, 2024 •

edited

Loading

texastoland commented Mar 16, 2024 •

edited

Loading

texastoland commented Mar 18, 2024 •

edited

Loading