Skip to content

nu-cmd-extra crate infrastructure in place with the Bits command as the model for adding other commands#9327

Merged
stormasm merged 8 commits intonushell:mainfrom
stormasm:bits
Jun 1, 2023
Merged

nu-cmd-extra crate infrastructure in place with the Bits command as the model for adding other commands#9327
stormasm merged 8 commits intonushell:mainfrom
stormasm:bits

Conversation

@stormasm
Copy link
Copy Markdown
Contributor

I wanted to get the infrastructure in place for starters for our nu-cmd-extra crate...

The plan is to put inside here the following commands...

  • bits
  • bytes
  • math

I thought it would be easier to do one at a time as well as get the nu-cmd-extra crate out there on crates.io
for this upcoming release...

Once this lands the infrastructure will be in place to move over the other noted commands for now...
And then add other stuff we do NOT want to be in 1.0.

@stormasm stormasm requested a review from amtoine May 31, 2023 21:15
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeaaah, loooks great, as with the dataframes 👌

no tests disabled and looks it's working 💪

cargo run --features "" -- -n

does not have the bits commands but

cargo run --features extra -- -n

does 🥳

👇 a little question in a thread down below

❓ and an extra question, why do we have the code in nu-cmd-extra/src/extra/ and not nu-cmd-extra/src/? 😮

@stormasm
Copy link
Copy Markdown
Contributor Author

stormasm commented Jun 1, 2023

and an extra question, why do we have the code in nu-cmd-extra/src/extra/ and not nu-cmd-extra/src/

I tried to model it after the dataframes crate...

But in reality I tried doing exactly what you said and just have

nu-cmd-extra/src/

But the way features work in Rust and Cargo it makes it way easier to have that "extra" level of indirection, no pun intended with the word extra 😄

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jun 1, 2023

@stormasm
mm ok i see, that's fine and we can always get rid of that "extra" level later 😌

@stormasm stormasm merged commit 356e051 into nushell:main Jun 1, 2023
@stormasm stormasm deleted the bits branch June 1, 2023 17:47
@stormasm
Copy link
Copy Markdown
Contributor Author

stormasm commented Jun 1, 2023

@stormasm mm ok i see, that's fine and we can always get rid of that "extra" level later 😌

@amtoine

The way features work its easy to create a feature in rust if the code lives in a particular directory...

So for example later on we can come along and bucket things up differently...

We may possibly want to have a math feature and a bits-bytes feature...

But initially I think it will be easier just to lump all of the nu-cmd-extra commands into one feature...

I always find features to be confusing to end user developers who are not intimately familiar
with the code base...

So my thinking for now is lets just keep this simple --- and have one feature called extra...

In the future if we have lots and lots of commands in nu-cmd-extra then we can come back
and re-address the number of features we want to complicate the nushell community with 😄

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jun 1, 2023

@stormasm
that sounds perfect to me 😊

@amtoine amtoine added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Jun 2, 2023
@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jun 2, 2023

@stormasm
can you confirm me that this is a breaking change, as it requires people in need of the bits commands to build with --features extra? 😋

@stormasm
Copy link
Copy Markdown
Contributor Author

stormasm commented Jun 2, 2023

Yes it is a breaking change as they will need to build with this command to get the bits command...

cargo build --features=extra'

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jun 2, 2023

thanks 😌

stormasm pushed a commit that referenced this pull request Jul 6, 2023
requires
- #9455

# ⚙️ Description
in this PR i move the commands we've all agreed, in the core team, to
move out of the core Nushell to the `extra` feature.

> **Warning**
> in the first commits here, i've
> - moved the implementations to `nu-cmd-extra`
> - removed the declaration of all the commands below from `nu-command`
> - made sure the commands were not available anymore with `cargo run --
-n`

## the list of commands to move
with the current command table downloaded as `commands.csv`, i've run
```bash
let commands = (
    open commands.csv
    | where is_plugin == "FALSE" and category != "deprecated"
    | select name category "approv. %"
    | rename name category approval
    | insert treated {|it| (
        ($it.approval == 100) or                # all the core team agreed on them
        ($it.name | str starts-with "bits") or  # see #9241
        ($it.name | str starts-with "dfr")      # see #9327
    )}
)
```
to preprocess them and then
```bash
$commands | where {|it| (not $it.treated) and ($it.approval == 0)}
```
to get all untreated commands with no approval, which gives
```
╭────┬───────────────┬─────────┬─────────────┬──────────╮
│  # │     name      │ treated │  category   │ approval │
├────┼───────────────┼─────────┼─────────────┼──────────┤
│  0 │ fmt           │ false   │ conversions │        0 │
│  1 │ each while    │ false   │ filters     │        0 │
│  2 │ roll          │ false   │ filters     │        0 │
│  3 │ roll down     │ false   │ filters     │        0 │
│  4 │ roll left     │ false   │ filters     │        0 │
│  5 │ roll right    │ false   │ filters     │        0 │
│  6 │ roll up       │ false   │ filters     │        0 │
│  7 │ rotate        │ false   │ filters     │        0 │
│  8 │ update cells  │ false   │ filters     │        0 │
│  9 │ decode hex    │ false   │ formats     │        0 │
│ 10 │ encode hex    │ false   │ formats     │        0 │
│ 11 │ from url      │ false   │ formats     │        0 │
│ 12 │ to html       │ false   │ formats     │        0 │
│ 13 │ ansi gradient │ false   │ platform    │        0 │
│ 14 │ ansi link     │ false   │ platform    │        0 │
│ 15 │ format        │ false   │ strings     │        0 │
╰────┴───────────────┴─────────┴─────────────┴──────────╯
```
# 🖌️ User-Facing Changes
```
$nothing
```

# 🧪 Tests + Formatting
- ⚫ `toolkit fmt`
- ⚫ `toolkit clippy`
- ⚫ `toolkit test`
- ⚫ `toolkit test stdlib`

# 📖 After Submitting
```
$nothing
```

# 🔍 For reviewers
```bash
$commands | where {|it| (not $it.treated) and ($it.approval == 0)} | each {|command|
    try {
        help $command.name | ignore
    } catch {|e|
        $"($command.name): ($e.msg)"
    }
}
```
should give no output in `cargo run --features extra -- -n` and a table
with 16 lines in `cargo run -- -n`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants