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

REFACTOR: move the 0% commands to nu-cmd-extra #9404

Merged

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Jun 10, 2023

requires

⚙️ 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

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 https://github.com/nushell/nushell/pull/9241
        ($it.name | str starts-with "dfr")      # see https://github.com/nushell/nushell/pull/9327
    )}
)

to preprocess them and then

$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

$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

Commands used:
```nu
mkdir crates/nu-cmd-extra/src/extra/filters/ crates/nu-cmd-extra/src/extra/filters/roll/
mv crates/nu-command/src/filters/each_while.rs crates/nu-cmd-extra/src/extra/filters/each_while.rs
mv crates/nu-command/src/filters/update_cells.rs crates/nu-cmd-extra/src/extra/filters/update_cells.rs
mv crates/nu-command/src/filters/rotate.rs crates/nu-cmd-extra/src/extra/filters/rotate.rs
mv crates/nu-command/src/filters/roll/ crates/nu-cmd-extra/src/extra/filters/roll/

mkdir crates/nu-cmd-extra/src/extra/strings/encode_decode/
mv crates/nu-command/src/strings/encode_decode/hex.rs crates/nu-cmd-extra/src/extra/strings/encode_decode/_hex.rs
mv crates/nu-command/src/strings/encode_decode/encode_hex.rs crates/nu-cmd-extra/src/extra/strings/encode_decode/encode_hex.rs
mv crates/nu-command/src/strings/encode_decode/decode_hex.rs crates/nu-cmd-extra/src/extra/strings/encode_decode/decode_hex.rs
mv crates/nu-command/src/strings/format/ crates/nu-cmd-extra/src/extra/strings/format/

mkdir crates/nu-cmd-extra/src/extra/formats/to/ crates/nu-cmd-extra/src/extra/formats/from/
mv crates/nu-command/src/formats/to/html.rs crates/nu-cmd-extra/src/extra/formats/to/html.rs
mv crates/nu-command/src/formats/from/url.rs crates/nu-cmd-extra/src/extra/formats/from/url.rs

mkdir crates/nu-cmd-extra/src/extra/platform/ansi/
mv crates/nu-command/src/platform/ansi/gradient.rs crates/nu-cmd-extra/src/extra/platform/ansi/gradient.rs
```
@amtoine amtoine added the needs-core-team-attention An issue than needs the attention of other core-team members label Jun 10, 2023
@amtoine amtoine requested a review from stormasm June 10, 2023 17:42
@stormasm
Copy link
Contributor

@amtoine I thought we were going to move the math commands and the bytes commands first prior to considering doing these ?

That is why I started with bits...

In our core team meeting a few weeks back we all agreed to do those first...

So I will let others comment on which commands they want to move across besides
math and bytes which everyone has already signed off on 😄

@stormasm
Copy link
Contributor

However, I am OK with moving all of these commands across to nu-cmd-extra as well...

Copy link
Contributor

@stormasm stormasm left a comment

Choose a reason for hiding this comment

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

I am fine with moving these commands across to nu-cmd-extra...
However we should get feedback from some other folks as well...
Just to make sure they are OK with it too. 😄

@amtoine
Copy link
Member Author

amtoine commented Jun 11, 2023

@stormasm
i've asked this in the core team channel 😌

@amtoine
Copy link
Member Author

amtoine commented Jun 11, 2023

i'll continue working on this in the meantime, this won't be lost work 👌

@amtoine amtoine changed the title REFACTOR: move the 0% percent commands to nu-cmd-extra REFACTOR: move the 0% commands to nu-cmd-extra Jun 11, 2023
@amtoine amtoine added pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes cratification labels Jun 11, 2023
@amtoine amtoine self-assigned this Jun 16, 2023
stormasm pushed a commit that referenced this pull request Jun 22, 2023
related to 
- #9404

# Description
to support our cratification effort and moving non-1.0 commands outside
of the main focus, this PR
- creates a new `nu-cmd-base` crate to hold the common structs, traits
and functions used by all command-related crates
- to start the transition, moves the `input_handler` module from
`nu-command` to `nu-cmd-base`

# User-Facing Changes
```
$nothing
```

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

# After Submitting
```
$nothing
```
fnordpig pushed a commit to fnordpig/nushell that referenced this pull request Jun 23, 2023
related to 
- nushell#9404

# Description
to support our cratification effort and moving non-1.0 commands outside
of the main focus, this PR
- creates a new `nu-cmd-base` crate to hold the common structs, traits
and functions used by all command-related crates
- to start the transition, moves the `input_handler` module from
`nu-command` to `nu-cmd-base`

# User-Facing Changes
```
$nothing
```

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

# After Submitting
```
$nothing
```
this solves the following error
```
error: proc-macro derive panicked
  --> crates/nu-cmd-extra/src/extra/formats/to/html.rs:79:10
   |
79 | #[derive(RustEmbed)]
   |          ^^^^^^^^^
   |
   = help: message: #[derive(RustEmbed)] folder '/home/amtoine/.local/share/git/store/github.com/amtoine/nushell/crates/nu-cmd-extra/assets/' does not exist. cwd: '/home/amtoine/.local/share/git/store/github.com/amtoine/nushell'

error[E0599]: no function or associated item named `get` found for struct `Assets` in the current scope
   --> crates/nu-cmd-extra/src/extra/formats/to/html.rs:228:19
    |
81  | struct Assets;
    | ------------- function or associated item `get` not found for this struct
...
228 |     match Assets::get(json_name) {
    |                   ^^^ function or associated item not found in `Assets`
    |
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following traits define an item `get`, perhaps you need to implement one of them:
            candidate #1: `SliceIndex`
            candidate #2: `RustEmbed`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `nu-cmd-extra` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
```
@amtoine
Copy link
Member Author

amtoine commented Jul 1, 2023

@stormasm
some news and improvements here 😌

  • i've solved the conflicts
  • moved some functions and assets
  • moved the missing ansi link command

now i'm trying to fix the tests but i have some trouble...
i was able to solve help_works_with_missing_requirements but now the examples of each while and update cells are failing and i do not understand what is the problem here 😕

@stormasm
Copy link
Contributor

stormasm commented Jul 1, 2023

@amtoine see my comments to you in discord 😄

amtoine and others added 2 commits July 6, 2023 10:24
* bring `enumerate` and `if` into `extra` tests

* disable all failing tests when not in `extra`
@amtoine amtoine marked this pull request as ready for review July 6, 2023 08:33
@amtoine amtoine requested a review from stormasm July 6, 2023 08:33
@amtoine
Copy link
Member Author

amtoine commented Jul 6, 2023

cc/ @stormasm

the CI still is suspicious but i think this PR is somewhat unrelated to the CI issue.
as the jobs should all be 🟢 as in amtoine#4, i think we can land this, if you're still ok with the changes 😌

@stormasm stormasm merged commit 504eff7 into nushell:main Jul 6, 2023
20 checks passed
Copy link
Contributor

@stormasm stormasm left a comment

Choose a reason for hiding this comment

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

Great job @amtoine

@amtoine amtoine deleted the refactor/move-0-percent-commands-to-extra branch July 6, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cratification needs-core-team-attention An issue than needs the attention of other core-team members pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants