changing default display_output hook disable ls_colors

[kurokirasama]

Describe the bug

hey, I'm trying to change the default display_output hook in this way:

let my_config = ($env.config)
let hooks = {
    pre_prompt: [{
        print $"Time elapsed: (($env.CMD_DURATION_MS | into decimal) / 1000) s"
        }]
    pre_execution: [{
      $nothing  
    }]
    env_change: {
      PWD: [{|before, after|
        $nothing 
      }]
    }
    display_output: {
      let-env LAST_OUTPUT = $in;
      print ($env.LAST_OUTPUT | table);
    }
  }

let my_config = ($my_config | upsert hooks $hooks)
let-env config = $my_config  

This is in a file named nu_config.nu that I source in config.nu via:

source-env /path/to/nu_config.nu

However, now ls output doesn't have colors.
If I leave the default:

let hooks = {
    pre_prompt: [{
        print $"Time elapsed: (($env.CMD_DURATION_MS | into decimal) / 1000) s"
        }]
    pre_execution: [{
      $nothing  
    }]
    env_change: {
      PWD: [{|before, after|
        $nothing 
      }]
    }
    display_output: {
     if (term size).columns >= 100 { table -e } else { table }
    }
  }

lscolors works again

How to reproduce

Already explained

Expected behavior

lscolors should works

Screenshots

No response

Configuration

key	value
version	0.72.1
branch	main
commit_hash	0621ab6
build_os	linux-x86_64
build_target	x86_64-unknown-linux-gnu
rust_version	rustc 1.65.0 (897e37553 2022-11-02)
rust_channel	1.65.0-x86_64-unknown-linux-gnu
cargo_version	cargo 1.65.0 (4bc8f24d3 2022-10-20)
pkg_version	0.72.1
build_time	2022-12-01 18:28:24 -03:00
build_rust_channel	release
features	database, dataframe, default, trash, which, zip
installed_plugins	custom-value generate, gstat, inc, query

Additional context

No response

[fdncred]

I have two thoughts on this right now.

1. For anyone looking to fix this. IIRC, in order to have LS_COLORS in your ls output, there needs to be a PipelineMetadata with a DataSource::Ls. Without this bit of metadata, the output isn't colored properly. There are several commands that do something like let metadata = input.metadata(); and then pass that metadata on through the output pipelinedata. Something like that will have to happen in order to get the colors back. I'm not exactly sure where to start looking though.

2. This could also be a problem because in the example above it's using print which is just printing strings instead of values.

[kurokirasama]

I tried without print and is the same

[Mehrbod2002]

@fdncred
Thanks for your information.
I want to work on it.
Reviewed the code and realized anyway we lose out metadata on subexpression , instead we fix the total core on Call and Value options .
Can we detect running command like ls and put a metadata from history or something like this ?
Is there any right access to history by available arguments , like engine_state ?

[fdncred]

Can we detect running command like ls and put a metadata from history or something like this ?

I'm not sure what you mean but we know when ls is ran. I'm just not sure what metadata from history means or would look like. Right now there's only metadata from DataSource I think.

Is there any right access to history by available arguments , like engine_state ?

You can look at how the history command works and maybe that will give you an idea.

[Mehrbod2002]

I'm not sure what you mean but we know when ls is ran. I'm just not sure what metadata from history means or would look like. Right now there's only metadata from DataSource I think.

No , actually we know when ls runs , but when we want to render it and use ls_color , no , we don't know .
If I am going to use history to retrieve a last command and attach metadata while rendering , it means for every command which is going to be table , do you think is it a good idea ?
There is no another choice . Another way is attach new feature which is metadata on Value enum for all types . which is not logical for just one ls command .
Waiting for your approval .
Any question , about this just ask me .
Thanks

[fdncred]

I'm up for alternate solutions to help with this but I'm not sure what a good one would look like exactly. Maybe @kubouch or @sholderbach know if what you're suggesting sounds reasonable.

Also, I'm sure there are a few other issues other than this one that talk about ls colors like maybe #4319 #4501 #7169 and maybe others. I mention this because there could be more information in other issues.

Thanks @Mehrbod2002 for jumping in and trying to help!

[sholderbach]

No , actually we know when ls runs , but when we want to render it and use ls_color , no , we don't know . If I am going to use history to retrieve a last command and attach metadata while rendering , it means for every command which is going to be table , do you think is it a good idea ?

Do you mean by history using the command history to do the metadata propagation or do you want to look at the previous steps in the current pipeline?
Here I think with the current design eagerly propagating the metadata at each step is easier with the current design.

Or is this rather related to some metatdata you want to express for the history command and its display?

There is no another choice . Another way is attach new feature which is metadata on Value enum for all types . which is not logical for just one ls command . Waiting for your approval . Any question , about this just ask me . Thanks

Yeah adding more metadata on the Value would cause bloat and also performance regressions. doing it on the level of PipelineMetadata at the moment is a compromise to only use it for particular commands and don't suffer from the performance impact if you don't need it.

So I think, it boils down to correctly implementing that for more commands/add utilities to do that properly without too much boilerplate.

Might be worth a little tracking label/issue/project, feels like a good issue for contributors starting out with command implementations and nu-protocol

[Mehrbod2002]

Do you mean by history using the command history to do the metadata propagation or do you want to look at the previous steps in the current pipeline?
Here I think with the current design eagerly propagating the metadata at each step is easier with the current design.

Can we look at the previous step in the current pipeline ?
As you said with this design it's no worth to manipulate core .
If we can look at steps it will be doable easily.
I can test it with all position and examples and aware you

[sholderbach]

Can we look at the previous step in the current pipeline ?

Not out of the box and this requires explicitly using the pipeline metadata at the moment:

See how for ls this is available as it is a special case in metadata

nushell/crates/nu-command/src/core_commands/metadata.rs

Lines 78 to 93 in 35b12fe

	if let Some(x) = &input.metadata() {
	match x {
	PipelineMetadata {
	data_source: DataSource::Ls,
	} => {
	cols.push("source".into());
	vals.push(Value::string("ls", head))
	}
	PipelineMetadata {
	data_source: DataSource::HtmlThemes,
	} => {
	cols.push("source".into());
	vals.push(Value::string("into html --list", head))
	}
	}
	}

The last example shows that such metadata also gets lost if data travels between different pipelines in nu evaluation (closure, subexpression)

[Mehrbod2002]

The last example shows that such metadata also gets lost if data travels between different pipelines in nu evaluation (closure, subexpression)

Thanks a lot .
At another way , for example :
{a: 1 ,b: 2} | rename x y | select x
When we reach to select is there any option can we get last step , which it is in this example rename ?
Something like using engine_state , stack , call or others to retrieve last step ?
So do you say it is impossible . yes ?

[sholderbach]

Not absolutely impossible as the pipeline gets parsed initially but technically not implemented in any flexible form at the moment at the evaluation time which basically does a bunch of Command::run() calls in order.

[Mehrbod2002]

Not absolutely impossible as the pipeline gets parsed initially but technically not implemented in any flexible form at the moment at the evaluation time which basically does a bunch of Command::run() calls in order.

As I understood we finally should detect ls command , do you agree ?
Because if we try to manipulate eval_subexpression , finally we need to detect command with not conflict with core system.
One thing I know is we can detect a ls columns by the name of columns in list , or other way if you know .
Or implement something to save a previous step in struct and use it .
If there is a option we can attach metadada while rendering .
Thanks @sholderbach

[KoviRobi]

I have got something like you want using #7950:

let-env v = []
def v [index:int=0] { $env.v | get -i $index }
def-env store_last_output [] {
  let-with-metadata data metadata = $in
  let-env LAST_OUTPUT = $data
  $data |  set-metadata $metadata
}
let-env config = $env.config | update hooks.display_output store_last_output

[fdncred]

I have got something like you want using #7950:

let-env v = []
def v [index:int=0] { $env.v | get -i $index }
def-env store_last_output [] {
  let-with-metadata data metadata = $in
  let-env LAST_OUTPUT = $data
  $data |  set-metadata $metadata
}
let-env config = $env.config | update hooks.display_output store_last_output

I couldn't get this to work.

[KoviRobi]

Yeah I've simplified my actual config and I cannot get it to work, I don't know why yet. Here is my actual config

https://github.com/KoviRobi/nixos-config/blob/6c7083da35d0b1eb7b59b6855464118e5f5902c5/home/shell.nix#L30-L54

Code inlined here

        let-env prev = (0..4 | each --keep-empty {null})
        def v [index:int=0] {
          $env.prev | get -i $index
        }
        def-env maybe_explore [] {
          let-with-metadata data metadata = $in
          $env.peek_output = (
            if ($data | describe) not-in [closure nothing] {
              let expanded = ($data | table -e)
              if (term size).rows < ($expanded | size).lines {
                $data | set-metadata $metadata | explore -p
              } else if ($data | describe) == closure {
                view-source $data
              }
            }
          )
          $env.prev.4 = $env.prev.3
          $env.prev.3 = $env.prev.2
          $env.prev.2 = $env.prev.1
          $env.prev.1 = $env.prev.0
          $env.prev.0 = $data
          $data |
            set-metadata $metadata |
            if (term size).columns >= 100 { table -e } else { table }
        }

(And yeah I could replace the bubbling of the values with a reduce, it just grew from one to a couple of values)

[KoviRobi]

I have got something like you want using #7950:

let-env v = []
def v [index:int=0] { $env.v | get -i $index }
def-env store_last_output [] {
  let-with-metadata data metadata = $in
  let-env LAST_OUTPUT = $data
  $data |  set-metadata $metadata
}
let-env config = $env.config | update hooks.display_output store_last_output

I couldn't get this to work.

D'oh figured out where I messed up, forgot the parentheses for the let-env. Also need double braces for the update

let-env LAST_OUTPUT = null
def-env store_last_output [] {
  let-with-metadata data metadata = $in
  let-env LAST_OUTPUT = $data
  $data |  set-metadata $metadata
}
let-env config = ($env.config | update hooks.display_output {{store_last_output}})

[fdncred]

I haven't tried this yet, but I worry about the way you're treating an environment variable as a mutable variable. There are unknown consequences and known consequences to doing this. The one known one that I can rattle off the top of my head is that, on Windows, there is a limit to the size of the environment. That limit is 32,767 bytes. That is the entire environment, not just one variable.

All environment variables must live together in a single environment block, which itself has a limit of 32767 characters.

https://devblogs.microsoft.com/oldnewthing/20100203-00/?p=15083

We've talked about having something like $nu.LAST_OUTPUT but it would have to be truncated to a certain limit as well since nushell can have infinite streams. I, personally, would like to see $nu.LAST_OUTPUT along with a configuration point to limit it and maybe a not-to-exceed limit in the code. Not sure exactly about the not-to-exceed limit.

[KoviRobi]

It's also what the OP is doing -- and yeah it can cause problems, on Linux this seems to happen when starting a process, e.g. my direnv and starship hooks break.

But since I cannot use a mutable variable that is captured by blocks, this is the only option for mutability I think.

~ at 17:20:06 nu
❯ let-env foo = (0..(2 ** 15) | each {"hello"} | str join ":")
Error: nu::shell::external_command (link)

  × External command failed
   ╭─[hook:1:1]
 1 │
 2 │     let direnv = (/nix/store/9ikwbnl8c5sniawwn8rai5bi6pgp6mxq-direnv-2.32.2/bin/direnv export json | from json)
   ·                   ──────────────────────────────────┬─────────────────────────────────
   ·                                                     ╰── can't run executable
 3 │     let direnv = if ($direnv | length) == 1 { $direnv } else { {} }
   ╰────
  help: Argument list too long (os error 7)

Error: nu::shell::external_command (link)

  × External command failed
    ╭─[init.nu:11:1]
 11 │     let width = (term size).columns
 12 │     ^/etc/profiles/per-user/rmk/bin/starship prompt $"--cmd-duration=($env.CMD_DURATION_MS)" $"--status=($env.LAST_EXIT_CODE)" $"--terminal-width=($width)"
    ·      ───────────────────┬───────────────────
    ·                         ╰── can't run executable
 13 │ }
    ╰────
  help: Argument list too long (os error 7)

Error: nu::shell::external_command (link)

  × External command failed
    ╭─[init.nu:34:1]
 34 │         let width = (term size).columns
 35 │         ^/etc/profiles/per-user/rmk/bin/starship prompt --right $"--cmd-duration=($env.CMD_DURATION_MS)" $"--status=($env.LAST_EXIT_CODE)" $"--terminal-width=($width)"
    ·          ───────────────────┬───────────────────
    ·                             ╰── can't run executable
 36 │     } else {
    ╰────
  help: Argument list too long (os error 7)

/home/rmk

I can get it back though, by setting $env.foo to something else though:

/home/rmk$env.foo = ""

~ at 17:20:15 nu
❯

[KoviRobi]

But you can use ENV_CONVERSIONS to get around this by making it convert to the empty string

let-env ENV_CONVERSIONS = ($env.ENV_CONVERSIONS | insert LAST_OUTPUT ({
  from_string: {|str| ""}
  to_string: {|str| ""}
}))