move dataframe commands to nu-cmd-dataframe by stormasm · Pull Request #9241 · nushell/nushell

stormasm · 2023-05-19T14:34:09Z

All of the dataframe commands ported over with no issues...

11 tests are commented out (for now)

So 100 of the original 111 tests are passing with only 11 tests being ignored for now..

As per our conversation in the core team meeting on Wednesday
I took @jntrnr suggestion and just commented out the tests dealing
with IntoDatetime

Later on we can move this functionality out of nu-command if we decide it makes sense...

The following tests were ignored...

modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_day.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_hour.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_minute.rs

modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_month.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_nanosecond.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_ordinal.rs

modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_second.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_week.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_weekday.rs

modified:   crates/nu-cmd-dataframe/src/dataframe/series/date/get_year.rs
modified:   crates/nu-cmd-dataframe/src/dataframe/series/string/strftime.rs

amtoine

i've started by looking at your last commit related to the tests 😋

could we, instead of commenting them out, which makes them harder to find and reenable, mark them as #[ignore]?
next to the #[test] or #[cfg(test)] 😉

amtoine

this looks great and simple, thanks a lot @stormasm for tackling the refactoring ❤️

i've got some remarks after looking at your changes:

✔️ crates/nu-command/src/dataframe/ is now gone
❌ an idea about the commenting of tests (see my other review)
❓ in the main Cargo.toml, we have

[features]
#...
dataframe = ["nu-command/dataframe"]

, maybe the path should change? 😮
i'm quite confused cargo build --features dataframe still works with this path 🤔

crates/nu-cmd-dataframe/Cargo.toml

stormasm · 2023-05-19T16:28:43Z

i've started by looking at your last commit related to the tests 😋

could we, instead of commenting them out, which makes them harder to find and reenable, mark them as #[ignore]? next to the #[test] or #[cfg(test)] 😉

so unfortunately you can only add

#[ignore]

to individual tests and not to the whole

#[cfg(test)]
mod test {

which is ideally what we would want...
If there were a way to ignore the whole mod test or if you know a way to do it 😄
that would be better --- I just don't think there is a way...

stormasm · 2023-05-19T16:33:32Z

this looks great and simple, thanks a lot @stormasm for tackling the refactoring ❤️

i've got some remarks after looking at your changes:

✔️ crates/nu-command/src/dataframe/ is now gone

❌ an idea about the commenting of tests (see my other review)

❓ in the main Cargo.toml, we have
[features]
#...
dataframe = ["nu-command/dataframe"]
, maybe the path should change? 😮 i'm quite confused cargo build --features dataframe still works with this path 🤔

This was one of the trickiest parts of the refactor ---- but yes this is the magical incantation that is needed for this to work..

Getting all of the correct feature pieces in the right place was mainly the only challenging part of the refactor that took me quite a while to figure out... 👍

amtoine · 2023-05-19T16:44:04Z

so unfortunately you can only add
#[ignore]
to individual tests and not to the whole

then i'd say adding #[ignore] to the 11 individual tests is better 😕

i'm not a huge fan of whole blocks of dead code...
at least, with #[ignore] we can locate them easily later and we see them with cargo test 👍
with dead commented code, we can't do either 😭

as they are only 11 of them, this looks feasible? 😌

amtoine · 2023-05-19T16:45:55Z

EDIT: ✔️ i just understood this and it's fine => nu-cmd-dataframe is seen as a dataframe feature in the nu-command crate and thus appears as nu-command/dataframe in the top level Cargo.toml 😏

This was one of the trickiest parts of the refactor ---- but yes this is the magical incantation that is needed for this to work..

ooh ok 😕
this is quite confusing because nu-command/dataframe looks at lot like the now removed crates/nu-command/src/dataframe/, doesn't it?
i really thought it would be called nu-cmd-dataframe 😮

Getting all of the correct feature pieces in the right place was mainly the only challenging part of the refactor that took me quite a while to figure out... +1

i trust you on this 😱 😆

stormasm · 2023-05-19T17:11:00Z

so unfortunately you can only add
#[ignore]
to individual tests and not to the whole
then i'd say adding #[ignore] to the 11 individual tests is better 😕

i'm not a huge fan of whole blocks of dead code... at least, with #[ignore] we can locate them easily later and we see them with cargo test 👍 with dead commented code, we can't do either 😭

as they are only 11 of them, this looks feasible? 😌

agreed on your point with

#[ignore = "comment why test is being ignored"]

but this will not solve the problem because this code is still there

#[cfg(test)]
mod test {
    use super::super::super::super::super::IntoDatetime;
    use super::super::super::super::test_dataframe::test_dataframe;
    use super::*;

and that is where the problem lies..

we need a way to tell the attribute to ignore the whole

#[cfg(test)]
mod test {

maybe @jntrnr or @fdncred knows a way to do that....

we can either delete the code entirely
which I don't want to do because after this PR lands we can explore possibly moving

IntoDatetime

out of nu-command

or once we determine its not worth doing then

then I could come back in and delete the whole block of code of where the test is located...

amtoine · 2023-05-19T17:24:29Z

oooh it's an issue with the imports...
ok i get it now 😢

then yeah if someone has a better idea 😌

EDIT: a quick and dirty way to do that might be to set a dummy cfg that will never happen
e.g. replacing #[cfg(test)] with #[cfg(dummy)] or better #[cfg(test_fixme)]?

yeah, cargo test does not complain at all with the following simple main.rs

fn main() {
    println!("Hello, world!");
}

#[cfg(test_fixme)]
mod tests {
    use euqwioeuwqioeu;
}

but does complain as soon as i turn on #[cfg(test)] 🥳 😏

stormasm · 2023-05-19T17:32:17Z

oooh it's an issue with the imports... ok i get it now 😢

then yeah if someone has a better idea 😌

EDIT: a quick and dirty way to do that might be to set a dummy cfg that will never happen e.g. replacing #[cfg(test)] with #[cfg(dummy)] or better #[cfg(test_fixme)]?

yeah, cargo test does not complain at all with the following simple main.rs
fn main() {
    println!("Hello, world!");
}

#[cfg(test_fixme)]
mod tests {
    use euqwioeuwqioeu;
}
but does complain as soon as i turn on #[cfg(test)] 🥳 😏

great idea !!
I will make those changes now 😄
thank you so much for reviewing this code
it is very much appreciated...
and you helped me solve the main issue
I was worried about which is how to deal with those tests for now...

amtoine · 2023-05-19T17:33:52Z

anytime @stormasm 😉 😌
this cratification is an important thing to do 😊

kubouch · 2023-05-19T18:19:45Z

Marked as a breaking change since we won't build with dataframes by default? (Or do we?)

stormasm · 2023-05-19T18:26:31Z

@kubouch we are good....

we were not building dataframes by default prior to my PR...

you had to pass in a feature flag in order to get dataframes...

its also currently documented here as well...

https://www.nushell.sh/book/dataframes.html

stormasm · 2023-05-19T20:21:45Z

When we publish our next release to crates.io we will have to add the crate

nu-cmd-dataframe

to our script...

kubouch · 2023-05-19T20:56:54Z

@stormasm See nushell/nu_scripts#507, I think that should be it?

stormasm · 2023-05-20T02:11:19Z

@stormasm See nushell/nu_scripts#507, I think that should be it?

@kubouch cool ! thanks for adding that...

amtoine · 2023-06-02T16:05:18Z

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

stormasm · 2023-06-02T16:59:11Z

@amtoine developers needing the bits command will not need dataframes 😄

cargo build --features=extra'

They will only have to build with the above command which does not include dataframes

amtoine · 2023-06-02T17:05:50Z

woooopsie, bad copy-paste 😱
i've changed the comment 😉

fdncred · 2023-06-02T18:07:44Z

it's not a breaking change. people who wanted dfrs already had to do --features=dataframes

amtoine · 2023-06-03T10:12:51Z

it's not a breaking change. people who wanted dfrs already had to do --features=dataframes

oooh you're right, it's just a refactor 👍

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`

Context from Discord: https://discord.com/channels/601130461678272522/615962413203718156/1138694933545504819 I was working on Nu for the first time in a while and I noticed that sometimes rust-analyzer takes a really long time to run `cargo check` on the entire workspace. I dug in and it was checking a bunch of dataframe-related dependencies even though the `dataframe` feature is not built by default. It looks like this is a regression of sorts, introduced by #9241. Thankfully the fix is pretty easy, we can make it so everything important in `nu-cmd-dataframe` is only used when the `dataframe` feature is enabled. ### Impact on `cargo check --workspace` Before this PR: 635 crates, 33.59s After this PR: 498 crates, ~20s (with the `mold` linker and a `cargo clean` before each run, the relative difference for incremental checks will likely be much larger)

stormasm added 2 commits May 19, 2023 06:36

infrastructure code in place, now move on to the tests

24a189d

ignore 11 dataframe tests dealing with IntoDatetime

663a321

amtoine requested changes May 19, 2023

View reviewed changes

amtoine reviewed May 19, 2023

View reviewed changes

crates/nu-cmd-dataframe/Cargo.toml Show resolved Hide resolved

remove polar dependencies from nu-command as it is no longer needed

564bd91

refactor the way we ignore the tests

0da66c5

stormasm merged commit c55b5c0 into nushell:main May 19, 2023

stormasm deleted the dfport10 branch May 19, 2023 17:56

kubouch added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label May 19, 2023

kubouch removed the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label May 19, 2023

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 removed the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Jun 3, 2023

amtoine added the cratification label Jun 11, 2023

rgwood mentioned this pull request Aug 10, 2023

Put heavy dataframe dependencies behind feature flag #9971

Merged

Conversation

stormasm commented May 19, 2023

11 tests are commented out (for now)

The following tests were ignored...

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stormasm commented May 19, 2023

Uh oh!

stormasm commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtoine commented May 19, 2023

Uh oh!

amtoine commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormasm commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtoine commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormasm commented May 19, 2023

Uh oh!

amtoine commented May 19, 2023

Uh oh!

kubouch commented May 19, 2023

Uh oh!

stormasm commented May 19, 2023

Uh oh!

stormasm commented May 19, 2023

Uh oh!

kubouch commented May 19, 2023

Uh oh!

stormasm commented May 20, 2023

Uh oh!

amtoine commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormasm commented Jun 2, 2023

Uh oh!

amtoine commented Jun 2, 2023

Uh oh!

fdncred commented Jun 2, 2023

Uh oh!

amtoine commented Jun 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amtoine left a comment •

edited

Loading

stormasm commented May 19, 2023 •

edited

Loading

amtoine commented May 19, 2023 •

edited

Loading

stormasm commented May 19, 2023 •

edited

Loading

amtoine commented May 19, 2023 •

edited

Loading

amtoine commented Jun 2, 2023 •

edited

Loading