REFACTOR: move the banner from the `rust` source to the standard library by amtoine · Pull Request #8406 · nushell/nushell

amtoine · 2023-03-11T15:51:54Z

Related to:

Description

with the new $nu.startup-time from #8353 and as mentionned in #8311, we are now able to fully move the nushell banner from the rust source base to the standard library.

this PR

removes all the rust source code for the banner
rewrites a perfect clone of the banner to std.nu, called std banner
call std banner from default_config.nu

User-Facing Changes

see the demo: https://asciinema.org/a/566521

no config will show the banner (e.g. cargo run --release -- --no-config-file)
a custom config without the if $env.config.show_banner block and no call to std banner would never show the banner
a custom config with the block and config.show_banner = true will show the banner
a custom config with the block and config.show_banner = false will NOT show the banner

Tests + Formatting

a new test line has been added to tests.nu to check the length of the std banner output.

🟢 toolkit fmt
🟢 toolkit clippy
🟢 toolkit test
🟢 toolkit test stdlib

After Submitting

$nothing

fdncred · 2023-03-11T15:58:48Z

Nice work @amtoine! However, I don't want to remove this until we start distributing the stdlib as default (just like you said above). So, with that in mind, I'm marking this a draft to prevent accidental merging.

amtoine · 2023-03-11T16:00:57Z

Nice work @amtoine! However, I don't want to remove this until we start distributing the stdlib as default (just like you said above). So, with that in mind, I'm marking this a draft to prevent accidental merging.

oh yeah you're right!

let's avoid an accidental merge 😌

codecov · 2023-03-11T16:18:46Z

Codecov Report

Merging #8406 (b611da8) into main (a92949b) will increase coverage by 0.07%.
The diff coverage is 16.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8406      +/-   ##
==========================================
+ Coverage   68.84%   68.92%   +0.07%     
==========================================
  Files         641      641              
  Lines      102321   102231      -90     
==========================================
+ Hits        70444    70463      +19     
+ Misses      31877    31768     -109

Impacted Files	Coverage Δ
crates/nu-cli/src/repl.rs	`3.35% <0.00%> (+0.42%)`	⬆️
src/run.rs	`73.91% <0.00%> (-0.33%)`	⬇️
crates/nu-std/src/lib.rs	`80.58% <100.00%> (+0.19%)`	⬆️
src/test_bins.rs	`96.96% <100.00%> (+0.02%)`	⬆️

... and 3 files with indirect coverage changes

amtoine · 2023-03-12T11:59:56Z

@kubouch
as in #8415, maybe this PR should be marked with the wait-until-after-nushell-release tag as well?

sophiajt · 2023-03-12T21:27:49Z

crates/nu-utils/standard_library/std.nu

+Learn how to remove this at: (ansi green)https://nushell.sh/book/configuration.html#remove-welcome-message(ansi reset)
+
+It's been this long since (ansi green)Nushell(ansi reset)'s first commit:
+((date now) - ('2019-05-10 09:59:12-0700' | into datetime))


You can use the datetime value here instead of using into datetime

(date now) - 2019-05-10T09:59:12-07:00)

ooh yes indeed!

let me switch to this simpler solution 👍

should be simpler in 838ca95, thanks 😌

@jntrnr

This is from a comment of @jntrnr.

This should solve the merge conflicts in nushell#8406.

amtoine · 2023-04-28T08:03:04Z

the three commits above should

solve the merge conflicts
move the implementation of banner and its test to the new nu-std crate
make the CI pass again

This is to avoid printing something on the same line as the time. I have a hook that triggers automatically when entering a directory and running `cargo run --release` gave me ``` ... Startup Time: -1nstoolkit module detected... activating toolkit module with `use toolkit.nu` ``` before this commit. now i get ``` ... Startup Time: -1ns toolkit module detected... activating toolkit module with `use toolkit.nu` ```

amtoine · 2023-04-28T08:30:20Z

💡 one thing we could do here is to remove the show_banner option from the config and just call std banner without the if block in default_config.nu.

to disable the banner, a user just have to either

remove the std banner call from their local copy of default_config.nu file
create a custom config file without std banner

what do you think? 😋

This should also fix the CI and the `test_config_path::test_alternate_config_path` test, see https://github.com/nushell/nushell/actions/runs/4828641085/jobs/8602701650?pr=8406

@fdncred

@fdncred found that 1. setting the PWD in the engine state solved the tests 2. not setting it in the stack did not change anything let's see what the CI thinks of this 😋

fdncred · 2023-04-28T14:16:35Z

what do you think? 😋

I'd prefer to implement with a clone with what we have today, 100% same functionality, just working from std lib vs rust code. Then, at a later time, if we want to make the changes you're suggestion, we could do that. I think it's better to just move one step at a time first.

fdncred · 2023-04-28T15:08:44Z

default_config.nu is not sourced by default?

It gets complicated, so complicated that i cannot remember when defaults are sourced and not. you'll have to do some research i guess.

amtoine · 2023-04-28T18:44:51Z

okey, i think i have a fix 😏

how it behaves now

>_ cargo run -- --no-config-file
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s
     Running `target/debug/nu --no-config-file`
     __  ,
 .--()°'.' Welcome to Nushell,
'|, . ,'   based on the nu language,
 !_-(_\    where all data is structured!

Please join our Discord community at https://discord.gg/NtAbbGn
Our GitHub repository is at https://github.com/nushell/nushell
Our Documentation is located at https://nushell.sh
Tweet us at @nu_shell
Learn how to remove this at: https://nushell.sh/book/configuration.html#remove-welcome-message

It's been this long since Nushell's first commit:
3yr 11month 3wk 3day 1hr 40min 36sec 411ms 210µs 441ns

Startup Time: 562ms 481µs 659ns
>                                                                            04/28/2023 08:39:49 PM

>_ cargo run -- --config config.show_banner.nu
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s
     Running `target/debug/nu --config config.show_banner.nu`
     __  ,
 .--()°'.' Welcome to Nushell,
'|, . ,'   based on the nu language,
 !_-(_\    where all data is structured!

Please join our Discord community at https://discord.gg/NtAbbGn
Our GitHub repository is at https://github.com/nushell/nushell
Our Documentation is located at https://nushell.sh
Tweet us at @nu_shell
Learn how to remove this at: https://nushell.sh/book/configuration.html#remove-welcome-message

It's been this long since Nushell's first commit:
3yr 11month 3wk 3day 1hr 40min 42sec 521ms 991µs 924ns

Startup Time: 727ms 981µs 929ns
>                                                                            04/28/2023 08:39:54 PM

>_ cargo run -- --config config.no-show_banner.nu
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s
     Running `target/debug/nu --config config.no-show_banner.nu`
>                                                                            04/28/2023 08:40:01 PM

where

# config.show_banner.nu
let-env config = {
    show_banner: true
}

and

# config.no-show_banner.nu
let-env config = {
    show_banner: false
}

the actual two changes

dba796b is here to have a $nu.startup-time not equal to -1 ns when the banner shows up
2b356d6 moves the banner call to the REPL evaluation, before the REPL loop

amtoine · 2023-04-29T07:32:37Z

it might be not perfect but that works and the banner has been moved to std 🥳

fdncred

I've tested this and it looks identical to the original. Good work!

amtoine · 2023-04-29T13:51:57Z

I've tested this and it looks identical to the original. Good work!

glad you like it 🥳

do we want the point of view of another member of the team?

sholderbach

You can now not run the default config with --no-std-lib:

As this call is in the rust code and not in the config this should be fixed on the Rust side in my view.

~/nushell> cargo run -- --no-std-lib
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s
     Running `target/debug/nu --no-std-lib`
Error: nu::parser::module_not_found

  × Module not found.
   ╭─[show_banner:1:1]
 1 │ use std banner; banner
   ·     ─┬─
   ·      ╰── module not found
   ╰────
  help: module files and their paths must be available before your script is run as parsing occurs before anything
        is evaluated

amtoine · 2023-05-04T17:04:07Z

You can now not run the default config with --no-std-lib:

As this call is in the rust code and not in the config this should be fixed on the Rust side in my view.

very good catch! 😮

what should be the behaviour of the banner with --no-std-lib then?
no banner? 🤔

fdncred · 2023-05-04T18:14:32Z

At a minimum, I think it's helpful to always have startup time. It's a benchmark metric that need to keep track of like this. It's an easy way to measure some startup performance areas.

nu
nu --no-std-lib
nu -n
nu -n --no-std-lib

amtoine · 2023-05-06T08:26:43Z

@sholderbach
i think i've found a patch for this, see b99654c 😋

Note
i wanted to add a test for this but could not find a way to simulate running nu --no-std-lib and check whether it fails or not 🤔

@fdncred

At a minimum, I think it's helpful to always have startup time. It's a benchmark metric that need to keep track of like this. It's an easy way to measure some startup performance areas.

we always can run the following to run performance benchmarks!

nu -c "print $nu.startup-time"
nu --no-std-lib -c "print $nu.startup-time"
nu -n -c "print $nu.startup-time"
nu -n --no-std-lib -c "print $nu.startup-time"

or run $nu.startup-time from the history when entering the shell.

i do not think we should require this from the banner 🤔

fdncred · 2023-05-06T13:16:38Z

That's fine. I forgot we were storing it in $nu.

fdncred · 2023-05-08T16:14:39Z

I have another change I'd like to make. I may try to add to this PR if that's ok?

amtoine · 2023-05-08T16:18:18Z

I have another change I'd like to make.

sure go ahead 😌

I may try to add to this PR if that's ok?

i've ticked the Allow edits and access to secrets by maintainers box above, tell me if that's ok to edit this branch 😋

fdncred · 2023-05-08T16:54:16Z

Updated with JT's better date math script + some tweaks.

But note, there's a bug somewhere I think because nanoseconds is always zero, which is why it's left out. I think it's something internal to how we're handling dates versus the scripting.

@amtoine Feel free to reorganize/rename or whatever. I just wanted to get the better math in there until Bob can fix our internal rust date math.

amtoine · 2023-05-08T17:02:53Z

i'll make this a DRAFT again for now.

just a question, where does this script come from? 😋

fdncred · 2023-05-08T17:04:41Z

just a question, where does this script come from? 😋

I said above. JT wrote most of it and I tweaked it.

I'm ready to land it once you give it a look.

fdncred · 2023-05-08T17:10:40Z

I'm seeing a little bit of cleanup work that needs to be done. There's a comment that I need to remove and we should think about maybe making a function in dt.nu that formats the duration string so we don't end up with this (which i just happened to get in Windows)

3yr 11month 29day 0hr 7min 48sec 641ms 830.8µs

we shouldn't have 0 items
we shouldn't have decimal items (which means nanos are working on windows)

fdncred · 2023-05-08T17:27:33Z

ok, cleaned it up a bit and added a pretty-print custom command.

fdncred · 2023-05-10T12:04:56Z

Let's give this a try while we still have a few days before the release.

amtoine added 3 commits March 11, 2023 16:31

remove the whole show banner on startup

186578f

add the banner clone to the standard library

af0d731

add a test to check the size of the banner output

bb814e7

amtoine marked this pull request as ready for review March 11, 2023 15:52

fdncred marked this pull request as draft March 11, 2023 15:58

sholderbach added the A:std-library Defining and improving the standard library written in Nu label Mar 11, 2023

amtoine mentioned this pull request Mar 12, 2023

standard library: a few ideas of commands #8311

Open

14 tasks

amtoine mentioned this pull request Mar 12, 2023

REFACTOR: remove the shell commands #8415

Merged

kubouch added the status:wait-until-after-nushell-release label Mar 12, 2023

sophiajt reviewed Mar 12, 2023

View reviewed changes

use the first-commit date directly instead of into datetime

838ca95

This is from a comment of @jntrnr.

amtoine mentioned this pull request Apr 2, 2023

stdlib: add an automatic loading of "prelude" commands #8627

Merged

amtoine added 2 commits April 28, 2023 09:47

Merge commit '4c4c1f6147' into nushell#8406

7838242

This should solve the merge conflicts in nushell#8406.

move the std banner command to the new nu-std crate

342d053

amtoine removed the status:wait-until-after-nushell-release label Apr 28, 2023

fix the test_banner test by calling std assert

5bb4fa7

amtoine self-assigned this Apr 28, 2023

amtoine added 2 commits April 28, 2023 10:24

call std banner in the default config file

5fc23a1

amtoine marked this pull request as ready for review April 28, 2023 08:28

amtoine added 3 commits April 28, 2023 10:50

show the banner in interactive mode only

ee0373c

This should also fix the CI and the `test_config_path::test_alternate_config_path` test, see https://github.com/nushell/nushell/actions/runs/4828641085/jobs/8602701650?pr=8406

load the standard library in nu_repl test bin

54bab35

set the PWD in the engine state before loading std in nu_repl

9de4e01

@fdncred found that 1. setting the PWD in the engine state solved the tests 2. not setting it in the stack did not change anything let's see what the CI thinks of this 😋

amtoine marked this pull request as draft April 28, 2023 17:12

amtoine added 2 commits April 28, 2023 19:53

set the startup time before the main REPL loop

dba796b

move the banner call to the rust REPL

2b356d6

amtoine marked this pull request as ready for review April 29, 2023 07:32

fdncred approved these changes Apr 29, 2023

View reviewed changes

sholderbach requested changes May 3, 2023

View reviewed changes

do not run the std banner when --no-std-lib is raised

b99654c

fdncred added 2 commits May 8, 2023 11:41

Merge branch 'main' into pr/amtoine/8406

c34d992

better date math

907ab91

amtoine marked this pull request as draft May 8, 2023 17:03

cleanup, add pretty-print

b611da8

fdncred marked this pull request as ready for review May 10, 2023 12:02

fdncred merged commit 43a3983 into nushell:main May 10, 2023

amtoine deleted the refactor/move-banner-from-rust-to-standard-library branch May 10, 2023 16:07

Conversation

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

Uh oh!

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

fdncred commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtoine commented Mar 11, 2023

Uh oh!

codecov bot commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amtoine commented Mar 12, 2023

Uh oh!

sophiajt Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine commented Apr 28, 2023

Uh oh!

amtoine commented Apr 28, 2023

Uh oh!

fdncred commented Apr 28, 2023

Uh oh!

fdncred commented Apr 28, 2023

Uh oh!

amtoine commented Apr 28, 2023

how it behaves now

the actual two changes

Uh oh!

amtoine commented Apr 29, 2023

Uh oh!

fdncred left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine commented Apr 29, 2023

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine commented May 4, 2023

Uh oh!

fdncred commented May 4, 2023

Uh oh!

amtoine commented May 6, 2023

Uh oh!

fdncred commented May 6, 2023

Uh oh!

fdncred commented May 8, 2023

Uh oh!

amtoine commented May 8, 2023

Uh oh!

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

Uh oh!

Uh oh!

amtoine commented May 8, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

fdncred commented May 8, 2023

Uh oh!

fdncred commented May 10, 2023

Uh oh!

Reviewers

Assignees

Labels

amtoine commented Mar 11, 2023 •

edited

Loading

fdncred commented Mar 11, 2023 •

edited

Loading

codecov bot commented Mar 11, 2023 •

edited

Loading

fdncred commented May 8, 2023 •

edited

Loading

fdncred commented May 8, 2023 •

edited

Loading

fdncred commented May 8, 2023 •

edited

Loading