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.

Sign up for GitHub

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

Jump to bottom

add a new toolkit install command with --features support #9288

Merged
merged 8 commits into from May 26, 2023
Merged

add a new toolkit install command with --features support #9288

merged 8 commits into from May 26, 2023

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented May 25, 2023

Description

i was installing Nushell and, as we have the dataframe feature and very soon at least the extra feature with more and more commands, i thought it could be cool to have a little toolkit install command 😋

User-Facing Changes

exposes the following command to developers

install Nushell and features you want

Usage:
  > install ...(features) 

Flags:
  -h, --help - Display the help message for this command

Parameters:
  ...features <string>: a space-separated list of feature to install with Nushell

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • ⚫ toolkit test
  • ⚫ toolkit test stdlib

After Submitting

$nothing

@fdncred
Copy link
Collaborator

fdncred commented May 25, 2023

I think this is a good idea too. In fact, I was thinking that maybe all the nushell scripts in the scripts folder should be apart of toolkit. Thoughts?

@amtoine
Copy link
Member Author

amtoine commented May 25, 2023

I think this is a good idea too.

🥳

In fact, I was thinking that maybe all the nushell scripts in the scripts folder should be apart of toolkit. Thoughts?

... 👀
guess what... 👀

i've thought about that exact same thing when we talked about the root refactoring 😬
and i agree we might want to break users once more, for the greater good 💪 😋

my questions would just be

  • in this PR
  • what about the non-nu scripts

@fdncred
Copy link
Collaborator

fdncred commented May 25, 2023

i'm fine with putting it in this PR or another. it's up to you. I'd also leave the nu scripts in place for backwards compatibility for a release or two.

the other scripts should stay.

@amtoine
Copy link
Member Author

amtoine commented May 26, 2023

@fdncred
i've simply

  • added an std log warning in the .nu scripts to mention the future deprecation of these scripts
  • moved and adapted the scripts to the toolkit as
    • toolkit build [--all]
    • toolkit cov
    • toolkit plugin register

😋

fdncred
fdncred reviewed May 26, 2023
View reviewed changes
toolkit.nu Outdated Show resolved Hide resolved
@fdncred
Copy link
Collaborator

fdncred commented May 26, 2023

I think it looks good except for the one custom command rename I suggested. Thanks!

@amtoine
Copy link
Member Author

amtoine commented May 26, 2023

thanks @fdncred for the review, let's wait for the CI and land this then 😊

@amtoine amtoine merged commit 15406a4 into nushell:main May 26, 2023
16 checks passed
@amtoine amtoine deleted the feature/toolkit/install-command branch May 26, 2023 09:22
@fdncred
Copy link
Collaborator

fdncred commented Jun 1, 2023

I was wanting to rebuild and install the plugins. I found toolkit build --all but I found no way to install the plugins other than using the install scripts install-all.sh or install-all.ps1. It would be nice if these were added to toolkit as well as uninstall-all.sh functionality.

@amtoine
Copy link
Member Author

amtoine commented Jun 1, 2023

good point 👍

looking at the commit before #9231 (check out on 8eece32), i think there has never been such an "install all" script written in nu, right? 🤔

but yeah that could be added 👌

@fdncred
Copy link
Collaborator

fdncred commented Jun 1, 2023

i think there has never been such an "install all" script written in nu, right?

No, there hasn't been. It's always been sh and ps1.

@amtoine
Copy link
Member Author

amtoine commented Jun 1, 2023

No, there hasn't been. It's always been sh and ps1.

i see 👍

i think the way right now is to

  • toolkit build --all
  • toolkit install ...(features)
  • toolkit register plugins

can you tell me if that works? 😊

@fdncred
Copy link
Collaborator

fdncred commented Jun 1, 2023

If I'm reading your code correctly, this is what is happening.

  • toolkit build --all will build nu and the plugins in debug mode but not release mode
  • toolkit install --features=$features will build and install nushell in release mode but not the plugins
  • toolkit register plugins will register plugins regardless of debug/release mode if they're in your path

What's missing is an install command that builds the plugins in release mode and installs them like cargo install --path . in each plugin dir.

@amtoine
Copy link
Member Author

amtoine commented Jun 2, 2023

i see 👍

thanks for the details, i'll have a look 😉

@amtoine amtoine mentioned this pull request Jun 2, 2023
Closed
@amtoine
Copy link
Member Author

amtoine commented Jun 2, 2023

i do not have the time tonight so i've filed an issue for this 😉

@SamantaTarun SamantaTarun mentioned this pull request Jun 5, 2023
Merged
fdncred added a commit that referenced this pull request Jun 13, 2023
@SamantaTarun @fdncred
enhancement(test)- complete the install command to install plugins #9342
54f8e34 
 (#9357)

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
related to
   closes #9342 
   complete the install command to install plugins
    [
](#9288)
the issue

    toolkit build only builds in debug mode
    toolkit install only installs Nushell
toolkit register plugins will install any plugins in the path, in debug
or release
# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# 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 -A clippy::result_large_err` to check that
you're using the standard code style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- crates/nu-std/tests/run.nu` to run the tests for the
standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# 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.
-->

---------

Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdncred fdncred fdncred left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@amtoine @fdncred