-
-
Notifications
You must be signed in to change notification settings - Fork 769
fix(nushell): add missing | parse env | update-env for deactivation operations
#6994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(nushell): add missing | parse env | update-env for deactivation operations
#6994
Conversation
|
Thank you for doing this! My love for both Nushell and Mise has made me yearn to learn Rust so I can help out, but I haven't had a chance yet. :D |
| @@ -0,0 +1,38 @@ | |||
| #! /usr/bin/env sh | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding nushell to either the coverage github action tests or mise.toml so they actually get run in CI? these tests otherwise won't do any good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, any special reason why the Windows end-to-end tests were automated via GitHub Actions, but not the rest of the end-to-end tests?
edit: ignore me, i found them in the "nightly" Action, although they don't seem to log any output to stdout which is intriguing ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mise run e2e --list works on my machine and outputs the full list to stdout
i'm not sure mise run e2e (what is already in "nightly") or even mise run e2e --all actually does anything at the moment ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdx well, i've explicitly added the nushell end-to-end tests in "nightly", but i think a follow-up PR is needed to fix the rest of the end-to-end tests, unless you're okay with only the nushell ones running? 🤷
9100a4a to
bc83799
Compare
|
that biome test failure is unrelated. I am going to try to fix it tomorrow |
|
oh, hang on, all of the end-to-end tests, including the nushell ones, are spread out across the "coverage" Actions, so explicitly adding the nushell tests to the "nightly" Action wasn't necessary (but i do need to ensure that |
dd1a5f9 to
fa24ac2
Compare
|
yay, for fa24ac2 : |
### 📦 Registry - add tbls by @artemklevtsov in [#6987](#6987) ### 🐛 Bug Fixes - **(nushell)** add missing `| parse env | update-env` for deactivation operations by @jokeyrhyme in [#6994](#6994) - **(pwsh)** wrap the executable path with double quotes by @leosuncin in [#6993](#6993) - in `activate bash` output, wrap mise executable path in single-quotes by @cspotcode in [#7002](#7002) ### 📚 Documentation - simplify apt instructions by @scop in [#6986](#6986) - update idiomatic version files enablement info by @scop in [#6985](#6985) ### 🧪 Testing - **(aqua)** remove biome test due to version incompatibility by @jdx in [#7000](#7000) ### 📦️ Dependency Updates - lock file maintenance by @renovate[bot] in [#6997](#6997) ### New Contributors - @jokeyrhyme made their first contribution in [#6994](#6994) - @cspotcode made their first contribution in [#7002](#7002) - @artemklevtsov made their first contribution in [#6987](#6987) - @leosuncin made their first contribution in [#6993](#6993) ## 📦 Aqua Registry Updates #### New Packages (1) - [`houseabsolute/omegasort`](https://github.com/houseabsolute/omegasort) #### Updated Packages (5) - [`apache/maven`](https://github.com/apache/maven) - [`chaqchase/lla`](https://github.com/chaqchase/lla) - [`leoafarias/fvm`](https://github.com/leoafarias/fvm) - [`rustic-rs/rustic`](https://github.com/rustic-rs/rustic) - [`sigstore/rekor`](https://github.com/sigstore/rekor)
|
I just downloaded this and tried to use it, but it's giving a "Parse mismatch during operation" error, pointing to the definition for |
|
@theutz eek! i'm not getting anything like that on this machine, but it might be worth reverting this PR out of caution ... which version of nushell do you have? what is the full output of |
|
I'm using 0.108.0. Error MessageOutput from
|
|
very mysterious I'm not having any of that on my machine and nushell in the coverage tests does |
|
I'm also having this same issue with the latest nushell and mise on Windows. nushell: mise: |
|
wait, Windows? @theutz are you also on Windows? |
|
I'm not using Windows, no. I'm on macOS Tahoe. |
|
Yeah, I don't know where those errors would be coming from. I've gone over the nushell code, and it doesn't make any sense. I'm not an expert in nu, but nothing seems too out of place to me. I guess the only thing that might not make sense to me is the presence of these lines in the "set,PATH,/opt/homebrew/sbin:/opt/homebrew/bin:/Users/michael/.local/bin:/Users/michael/bin:/usr/local/bin:/Users/michael/.config/composer/vendor/bin:/Users/michael/.config/carapace/bin:/usr/bin:/bin:/usr/sbin:/sbin
hide,MISE_SHELL,
hide,__MISE_DIFF,
hide,__MISE_DIFF,
" | parse vars | update-envIf we're treating the output of this command as a module, should that code be beneath the |
|
i find nushell can occasionally be wildly confused about the origin coordinates of an error there was only one it'd be interesting to selectively delete/change parts of your output from |
|
Good idea! So, I tried commenting everything out, and the culprit seems to be fact that line. If the script is |
|
oh! i use that's the difference! if i replace i think because the atuin documentation says okay, excellent, i can reproduce locally and hence test locally and hence raise a PR to fix this confidently :) |
|
here we go: #7013 |
|
Thank you, sir! |
- the previous PR ( #6994 ) made the incorrect assumption that the output of `mise activate nu` will be `source`d from a nushell configuration, but our documentation specifically says it should be `use`d instead ( https://mise.jdx.dev/installing-mise.html#nushell ) - this PR changes our nushell end-to-end tests accordingly, and moves the generated lines relating to the CSV text down into the `export-env { ... }` block to fix the parser errors
misereverse the changes it made to the environment variables: fix(activate): reset PATH when activate is called multiple times #6829| parse env | update-envjust as with other places in the nushell script that make use of the raw operations CSV textparse envandupdate-envfunctions were re-ordered to always be above the deactivation statements