add `dirs` command to std lib by bobhy · Pull Request #8368 · nushell/nushell

bobhy · 2023-03-09T08:43:34Z

Description

Prototype replacement for enter, n, p, exit built-ins implemented as scripts in standard library.
MVP-level capabilities (rough hack), for feedback please. Not intended to merge and ship as is.

(Description of your pull request goes here. Provide examples and/or screenshots if your changes affect the user experience.)

User-Facing Changes

New command in standard library

〉use ~/src/rust/nushell/crates/nu-utils/standard_library/dirs.nu
---------------------------------------------- /home/bobhy ----------------------------------------------
〉help dirs
module dirs.nu -- maintain list of remembered directories + navigate them

todo:
* expand relative to absolute paths (or relative to some prefix?)
* what if user does `cd` by hand?

Module: dirs

Exported commands:
  add (dirs add), drop, next (dirs next), prev (dirs prev), show (dirs show)

This module exports environment.
---------------------------------------------- /home/bobhy ----------------------------------------------
〉dirs add ~/src/rust/nushell /etc ~/.cargo
-------------------------------------- /home/bobhy/src/rust/nushell --------------------------------------
〉dirs next 2
------------------------------------------- /home/bobhy/.cargo -------------------------------------------
〉dirs show
╭───┬─────────┬────────────────────╮
│ # │ current │        path        │
├───┼─────────┼────────────────────┤
│ 0 │         │ /home/bobhy        │
│ 1 │         │ ~/src/rust/nushell │
│ 2 │         │ /etc               │
│ 3 │ ==>     │ ~/.cargo           │
╰───┴─────────┴────────────────────╯
------------------------------------------- /home/bobhy/.cargo -------------------------------------------
〉dirs drop
---------------------------------------------- /home/bobhy ----------------------------------------------
〉dirs show
╭───┬─────────┬────────────────────╮
│ # │ current │        path        │
├───┼─────────┼────────────────────┤
│ 0 │ ==>     │ /home/bobhy        │
│ 1 │         │ ~/src/rust/nushell │
│ 2 │         │ /etc               │
╰───┴─────────┴────────────────────╯
---------------------------------------------- /home/bobhy ----------------------------------------------
〉

Tests + Formatting

Haven't even looked at stdlib tests.nu yet.

Other todos:

address module todos.
integrate into std lib, rather than as standalone module. Somehow arrange for use .../standard_library/std.nu to load this module without having to put all the source in std.nu?
Maybe command should be std dirs ...?
what else do enter and exit do that this should do? Then deprecate those commands.

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 to check that you're using the standard code style
cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

codecov · 2023-03-09T09:06:11Z

Codecov Report

Merging #8368 (54e6faa) into main (e435196) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8368   +/-   ##
=======================================
  Coverage   68.25%   68.25%           
=======================================
  Files         620      620           
  Lines       99355    99355           
=======================================
  Hits        67813    67813           
  Misses      31542    31542

see 3 files with indirect coverage changes

fdncred · 2023-03-09T13:47:44Z

Nice add. Just initial thoughts. There are a few changes here that do not allow this to be a drop-in replacement for our shells suite of commands.

We use the term shells for this. I see you named it dirs. Not saying we can't rename it but when people come looking for shells, this may be hard to find.
There is also the g command which will goto a shell at the provided index. g 1 changes to the shell that is at 1 in the list. I see you kind of have that in dirs next 2.
Our current indication of which shell is active is true under the active column. Having this named differently will cause errors for people with current implementations that check for where active == true.
It may be reasonable not to have an exit command, unless we're calling these commands like shells exit.
I think one of the questions we need to ask ourselves with this set of commands is, should this be a drop-in replacement for enter, exit, n, p, g? I'm not sure exactly but will be interested to hear other's opinions.

Generally speaking, this is a great start and covers most of the points we'd want to cover. Thanks for this submission!

bobhy · 2023-03-09T17:21:21Z

I wasn't aiming for a drop in replacement (although that would be technically feasible). While I was playing around with idea of how to write a stdlib package, I was also playing around with the user model of working in multiple directories at once.

For me, and I'm just riffing here, the problem with enter and exit is they are legacy terminology referring to stack semantics. like bash dirs originally had, even though the user community has advanced to expect "ring" semantics, with 'next' and 'previous' wrapping at each end, like bash dirs has now). My use of dirs for the command name is a similar problem. If you add the ability to enter multiple directories at once (as I did here), it becomes even clearer that you're simply managing entries in a list, and the command should be talking about lists not stacks.
Another problem I have with shells for a name is that it implies more than it delivers (as far as I can tell). From the name, I was thinking that it would create a new scope for variables as well as change working directory, but it doesn't seem to.

But I'm sure we can come up with a set of names that strike the right chord!

You're right about the show table, I'll fix it.
@amtoine showed me how to get the module loaded from std.nu, so I'll fix that, too.

fdncred · 2023-03-09T19:14:03Z

Just my two cents, I'd prefer to have a drop in replacement first and then tweak the naming later. Also, just for your information, shells used to be more full feature. We allowed you to enter files and do ls, cd, etc within a file structure. It was cool but not super practical. So, you're right that we've lost some functionality and may be ready for new command naming.

Let's see what others think.

bobhy · 2023-03-09T19:30:22Z

Recent commits finalize with stdlib (you can get the commands by use std.nu, fix the show output and track absolute paths so it continues to work even if you cd away from the list. Functionally, I think it's done.
But I need to add some tests, and more verbose help doc, will get going on that now.

Remaining to do is deprecating shells, enter .. which means bottoming out on naming, so let`s continue that discussion.

I'll take off the "draft" tag at that point and be ready to merge.

kubouch · 2023-03-09T20:29:58Z

I don't really mind the new naming since it's based on some state-of-the art, as you mentioned. But maybe we can add the n, p, etc. as aliases to the new commands, just for compatibility reasons. At least for a few releases. I personally would have aliased them anyway to one-letter aliases.

bobhy · 2023-03-10T00:28:23Z

Can't really add aliases to default config till we can rely on the std.nu and 'dirs.nuscripts being deployed in some well known place. Then user's config canusethem and *then* we can add aliases referencing them. It'd be sufficient for now ifcargo install` handled this.

bobhy · 2023-03-10T00:48:03Z

tracking #8311 for refinement of stdlib structure. This PR is dependent on choices made there.

amtoine

thanks a lot that's great 👍

i think there is not real format standard right now for nushell (and i really need to work on nufmt 👀) but here is a diff i think would improve a bit the library, only whitespaces and minor things 😉

diff --git a/crates/nu-utils/standard_library/dirs.nu b/crates/nu-utils/standard_library/dirs.nu
index dedf81d63..1c989c9d9 100644
--- a/crates/nu-utils/standard_library/dirs.nu
+++ b/crates/nu-utils/standard_library/dirs.nu
@@ -9,8 +9,8 @@ export-env {
 # Add one or more directories to the list.
 # PWD becomes first of the newly added directories.
 export def-env "add" [
-    ...paths: string    # directory or directories to add to working list
-    ] {
+    ...paths: string  # directory or directories to add to working list
+] {
         mut abspaths = []
         for p in $paths {
             if ($p | path type) != 'dir' {
@@ -28,23 +28,28 @@ export def-env "add" [
 
 # Advance to the next directory in the list or wrap to beginning.
 export def-env "next" [
-    N:int = 1 # number of positions to move.
+    N: int = 1  # number of positions to move.
 ] {
-    _fetch $N    
+    _fetch $N
 }
 
 # Back up to the previous directory or wrap to the end.
 export def-env "prev" [
-    N:int = 1 # number of positions to move.
+    N: int = 1  # number of positions to move.
 ] {
-    _fetch (-1 * $N)    
+    _fetch (-1 * $N)
 }
 
 # Drop the current directory from the list, if it's not the only one.
 # PWD becomes the next working directory
 export def-env "drop" [] {
     if ($env.DIRS_LIST | length) > 1 {
-        let-env DIRS_LIST = (($env.DIRS_LIST | take $env.DIRS_POSITION) | append ($env.DIRS_LIST | skip ($env.DIRS_POSITION + 1)))
+        let-env DIRS_LIST = (
+            ($env.DIRS_LIST | take $env.DIRS_POSITION)
+            | append (
+                $env.DIRS_LIST | skip ($env.DIRS_POSITION + 1)
+            )
+        )
     }
 
     _fetch 0
@@ -55,7 +60,7 @@ export def-env "show" [] {
     mut out = []
     for $p in ($env.DIRS_LIST | enumerate) {
         $out = ($out | append [
-            [active, path]; 
+            [active, path];
             [($p.index == $env.DIRS_POSITION), $p.item]
         ])
     }
@@ -63,18 +68,15 @@ export def-env "show" [] {
     $out
 }
 
-
-
 # fetch item helper
-def-env  _fetch [
-    offset: int,            # signed change to position
+def-env _fetch [
+    offset: int  # signed change to position
 ] {
     # nushell 'mod' operator is really 'remainder', can return negative values.
-    # see: https://stackoverflow.com/questions/13683563/whats-the-difference-between-mod-and-remainder    
+    # see: https://stackoverflow.com/questions/13683563/whats-the-difference-between-mod-and-remainder
     let pos = ($env.DIRS_POSITION + $offset + ($env.DIRS_LIST | length)) mod ($env.DIRS_LIST | length)
     # echo $"at ($pos); item is: ($env.DIRS_LIST |  get $pos)"
     let-env DIRS_POSITION = $pos
 
     cd ($env.DIRS_LIST | get $pos )
 }
-            
\ No newline at end of file

and now let's test it 😏

crates/nu-utils/standard_library/dirs.nu

amtoine

some feedback i have 😋

i think it's not related to your work but here's what i get 🤔

>_ use std.nu
>_ help std dirs
Error: nu::parser::not_found

  × Not found.
   ╭─[entry #2:1:1]
 1 │ help std dirs
   ·      ────┬───
   ·          ╰── did not find anything under this name
   ╰────

but

>_ help std
std.nu, `used` to load all standard library components

Module: std

Exported commands:
  assert (std assert), assert eq (std assert eq), assert ne (std assert ne), dirs add (std dirs add), dirs drop (std dirs drop), dirs next (std dirs next), dirs prev (std dirs prev), dirs show (std dirs show), match (std match), path add (std path add)

This module does not export environment.

and i cannot

>_ use std.nu dirs
Error: nu::parser::export_not_found

  × Export not found.
   ╭─[entry #7:1:1]
 1 │ use std.nu dirs
   ·            ──┬─
   ·              ╰── could not find imports
   ╰────

🤔

otherwise, this is quite cool, i've played a bit with it and it looks to be working fine for now 🥳

so left to be done

❌ the few changes propositions i've come up with
✔️ the path expand bug
✔️ the tests in tests.nu

crates/nu-utils/standard_library/dirs.nu

bobhy · 2023-03-10T22:40:31Z

(sorry, I don't know how to reply directly to your comment above about use weirdness, but here's what I found:)

Maybe it's a bug in how help builds the module-level help for a submodule? I can get subcommand level help from dirs as a submodule of std, but cannot get the submodule help itself.

〉use ~/src/rust/nushell/crates/nu-utils/standard_library/std.nu
-------------------------------------------- /home/bobhy/src/rust/nushell --------------------------------------------
〉help std
std.nu, `used` to load all standard library components

Module: std

Exported commands:
  assert (std assert), assert eq (std assert eq), assert ne (std assert ne), dirs add (std dirs add), dirs drop (std dirs drop), dirs next (std dirs next), dirs prev (std dirs prev), dirs show (std dirs show), match (std match), path add (std path add)

This module does not export environment.
-------------------------------------------- /home/bobhy/src/rust/nushell --------------------------------------------
〉help std dirs
Error: nu::parser::not_found

  × Not found.
   ╭─[entry #32:1:1]
 1 │ help std dirs
   ·      ────┬───
   ·          ╰── did not find anything under this name
   ╰────

-------------------------------------------- /home/bobhy/src/rust/nushell --------------------------------------------
〉help std dirs add
Add one or more directories to the list.
PWD becomes first of the newly added directories.

Usage:
  > std dirs add ...(paths) 

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

Parameters:
  ...paths <string>: directory or directories to add to working list

bobhy · 2023-03-11T04:10:50Z

I believe I've picked up all the (very good!) suggestions, and added credible tests (which all pass).
However, if a test fails, I can't clean up temporary directories. It's a problem with try {} catch {|e|}` (or my expectations for it), I'll raise a separate issue for that.

But I do believe the dirs code works, please review and LMK what you think.

amtoine

a few little things i think would improve the module and the tests right away 😉

apart from that, i like this a lot and the tests pass 👌

crates/nu-utils/standard_library/dirs.nu

crates/nu-utils/standard_library/tests.nu

amtoine · 2023-03-11T10:03:17Z

crates/nu-utils/standard_library/tests.nu

+    }
+
+    cd $base_path
+    cd ..


you do not need to undo the cds in a regular def function, 'cause the PWD does not change outside the scope of the function.

you might be familiar with subshells in bash where

# here you in some initial-place/ { cd somewhere # do something in somewhere/ } # and you're back to initial-place/ without any `cd`ing back

Is the localized CD true of changes within any closure (like a try/catch or if/then), or only within scope of function body?
I was also concerned that a failing test might leave the working directory indeterminate, which might make the directory cleanup at the end unreliable. So I think I have to CD into the parent directory before I try to rm -r the temporary directories, since there is some risk I might still have working directory set to one of the directories I'm trying to delete.
And really, the extra CD is cheap insurance in a test, it's not mainline code... I want to leave it as is.

this disk directory might be not trivial to work with 🤔

I would agree with @bobhy here that we probably should play it defensive for now. It is not totally unprecedented that our semantics in this area have changed. And if we can keep the blast radius minimal for stuff like that we get a little peace of mind.

but what about just putting all these directories inside $nu.temp-path instead? 🤔

that would solve the issue of cleaning them => they would be at next reboot for example and the amount of things being generated is minimal

crates/nu-utils/standard_library/tests.nu

…arvest; whitespace patrol

bobhy · 2023-03-11T18:50:45Z

I've maintained my local assert in the test, will refactor this out when the official one lands, in a new PR.

amtoine · 2023-03-11T21:15:15Z

I've maintained my local assert in the test, will refactor this out when the official one lands, in a new PR.

that sounds great, when #8405 is merged then 👍

the code is nice, the tests pass, let's wait for the core team to approve this 😉

fdncred · 2023-03-11T22:31:04Z

Looks fun to play with, let's do it!

amtoine · 2023-03-12T09:34:25Z

🥳

@bobhy

Related to #8368. # Description as planned in #8311, the `enter`, `shells`, `g`, `n` and `p` commands have been re-implemented in pure-`nushell` in the standard library. this PR removes the `rust` implementations of these commands. - all the "shells" tests have been removed from `crates/nu-commnand/tests/commands/` in 2cc6a82, except for the `exit` command - `cd` does not use the `shells` feature in its source code anymore => that does not change its single-shell behaviour - all the command implementations have been removed from `crates/nu-command/src/shells/`, except for `exit.rs` => `mod.rs` has been modified accordingly - the `exit` command now does not compute any "shell" related things - the `--now` option has been removed from `exit`, as it does not serve any purpose without sub-shells # User-Facing Changes users may now not use `enter`, `shells`, `g`, `n` and `p` now they would have to use the standard library to have access to equivalent features, thanks to the `dirs.nu` module introduced by @bobhy in #8368 # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - ⚫ `toolkit test` - ⚫ `toolkit test stdlib` # After Submitting the website will have to be regenerated to reflect the removed commands 👍

bobhy marked this pull request as draft March 9, 2023 08:47

bobhy mentioned this pull request Mar 10, 2023

Enter command: *multiple* directories #8357

Closed

bobhy force-pushed the std_dirs branch from 79905cd to 35fcd38 Compare March 10, 2023 01:21

amtoine mentioned this pull request Mar 10, 2023

standard library: a few ideas of commands #8311

Open

14 tasks

amtoine requested changes Mar 10, 2023

View reviewed changes

crates/nu-utils/standard_library/dirs.nu Show resolved Hide resolved

amtoine requested changes Mar 10, 2023

View reviewed changes

crates/nu-utils/standard_library/dirs.nu Outdated Show resolved Hide resolved

bobhy marked this pull request as ready for review March 11, 2023 04:04

bobhy changed the title ~~add dirs command to std lib (wip)~~ add dirs command to std lib Mar 11, 2023

amtoine requested changes Mar 11, 2023

View reviewed changes

amtoine mentioned this pull request Mar 11, 2023

std.nu: Rewrite assert method #8405

Merged

bobhy added 7 commits March 11, 2023 12:31

add dirs command to std lib (wip)

63db718

Load dirs automatically from use std.nu

0a9a0c1

trac absolute paths; check for valid directory paths

4f7ffe8

Copy export-env from submodule to std.nu (why needed)?

c2fb96c

bugfix and format changes per review

9732b35

tests work

2855187

per review: simplify local assert; use temp-path; common expression h…

54e6faa

…arvest; whitespace patrol

bobhy force-pushed the std_dirs branch from 9acd8ed to 54e6faa Compare March 11, 2023 18:42

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

amtoine approved these changes Mar 11, 2023

View reviewed changes

fdncred merged commit 2e01bf9 into nushell:main Mar 11, 2023

amtoine mentioned this pull request Mar 12, 2023

REFACTOR: remove the shell commands #8415

Merged

bobhy deleted the std_dirs branch August 25, 2023 04:21

Conversation

bobhy commented Mar 9, 2023

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

bobhy commented Mar 9, 2023

Uh oh!

fdncred commented Mar 9, 2023

Uh oh!

bobhy commented Mar 9, 2023

Uh oh!

kubouch commented Mar 9, 2023

Uh oh!

bobhy commented Mar 10, 2023

Uh oh!

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

Uh oh!

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

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!

bobhy commented Mar 10, 2023

Uh oh!

bobhy commented Mar 11, 2023

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amtoine Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

bobhy Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

sholderbach Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobhy commented Mar 11, 2023

Uh oh!

amtoine commented Mar 11, 2023

Uh oh!

fdncred commented Mar 11, 2023

Uh oh!

amtoine commented Mar 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Mar 9, 2023 •

edited

Loading

fdncred commented Mar 9, 2023 •

edited

Loading

bobhy commented Mar 10, 2023 •

edited

Loading

amtoine left a comment •

edited

Loading