Skip to content
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

Update nushell.rs - Add explicit spread #1441

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Update nushell.rs - Add explicit spread #1441

merged 3 commits into from
Jan 12, 2024

Conversation

bnheise
Copy link
Contributor

@bnheise bnheise commented Jan 12, 2024

Running mise with nushell in the latest release of nushell (currently 0.89.0) results in the following warning:

Error:   × Automatically spreading lists is deprecated
    ╭─[/hREDACTED:35:1]
 35 │   } else {
 36 │     ^"rtx" $command $rest
    ·                     ──┬──
    ·                       ╰── Spreading lists automatically when calling external commands is deprecated and will be removed in 0.91.
 37 │   }
    ╰────
  help: Use the spread operator (put a '...' before the argument)

This change fixes the warning.

Running mise with nushell in the latest release of nushell (currently [0.89.0](https://www.nushell.sh/blog/2024-01-09-nushell_0_89_0.html#spread-operator-for-commands)) results in the following warning:

```shell
Error:   × Automatically spreading lists is deprecated
    ╭─[/hREDACTED:35:1]
 35 │   } else {
 36 │     ^"rtx" $command $rest
    ·                     ──┬──
    ·                       ╰── Spreading lists automatically when calling external commands is deprecated and will be removed in 0.91.
 37 │   }
    ╰────
  help: Use the spread operator (put a '...' before the argument)
```

This change fixes the warning.
@bnheise
Copy link
Contributor Author

bnheise commented Jan 12, 2024

It looks like some tests are failing, but it isn't clear to me why just by reading the output. Some input on how to fix the issues would be helpful.

@jdx
Copy link
Owner

jdx commented Jan 12, 2024

you need to update the snapshots with mise run snapshots

@bnheise
Copy link
Contributor Author

bnheise commented Jan 12, 2024

Got it, thanks!

@bnheise
Copy link
Contributor Author

bnheise commented Jan 12, 2024

@jdx Sorry, quick question. I ran mise run snapshots and it completed without errors, but it also didn't result in any changes to commit. Is this expected? Sorry, I don't know much about snapshots so I'm not sure what the expected result is.

@jdx
Copy link
Owner

jdx commented Jan 12, 2024

I'm surprised it didn't error. You may need to install cargo install cargo-insta. If that doesn't work: cargo insta test --accept

@jdx
Copy link
Owner

jdx commented Jan 12, 2024

I just mentioned in the Discord I think we should make it so you can mise install everything needed to develop mise but that's currently not done.

@bnheise
Copy link
Contributor Author

bnheise commented Jan 12, 2024

It did error because cargo insta wasn't installed at first, so I installed cargo-insta and tried again. After that, it worked.

@jdx
Copy link
Owner

jdx commented Jan 12, 2024

if nothing else, just manually edit the snapshots:

  • src/shell/snapshots/mise__shell__nushell__tests__hook_init_nix.snap
  • src/shell/snapshots/mise__shell__nushell__tests__hook_init.snap

@jdx jdx enabled auto-merge (squash) January 12, 2024 23:42
@jdx jdx merged commit 36fdc55 into jdx:main Jan 12, 2024
7 checks passed
@bnheise
Copy link
Contributor Author

bnheise commented Jan 15, 2024

Thanks for fixing that! And thanks for the great work on such a great tool. I introduced this at work recently and it got great reviews from the devs that tried it.

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.

None yet

2 participants