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.

Sign up for GitHub

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

Jump to bottom

(ls | sort-by name) and other subexpressions lose ls_colors #4501

Open
Tracked by #4356
fdncred opened this issue Feb 16, 2022 · 19 comments
Open
Tracked by #4356

(ls | sort-by name) and other subexpressions lose ls_colors #4501

fdncred opened this issue Feb 16, 2022 · 19 comments
Labels
🐛 bug Something isn't working pipeline-metadata Additional information passed along that is not directly attached to the values (e.g. LSCOLORS)

Comments

@fdncred
Copy link
Collaborator

fdncred commented Feb 16, 2022

Describe the bug

alias ls = (ls | sort-by name)
names are not colored with ls_colors

How to reproduce

see above

Expected behavior

retain ls_colors

Screenshots

No response

Configuration

key value
version 0.59.0
branch main
short_commit b64ac9e
commit_hash b64ac9e
commit_date 2022-02-16 18:24:45 +00:00
build_os windows-x86_64
rust_version rustc 1.58.1 (db9d1b20b 2022-01-20)
rust_channel stable-x86_64-pc-windows-msvc
cargo_version cargo 1.58.0 (f01b232bc 2022-01-19)
pkg_version 0.59.0
build_time 2022-02-16 12:33:29 -06:00
build_rust_channel debug
features dataframe, default, which, zip
installed_plugins gstat, inc, nu-example-1, nu-example-2, nu-example-3, query, query json, query web, query xml

Additional context

No response
@fdncred fdncred mentioned this issue Feb 16, 2022
Closed
59 tasks
@sophiajt
Copy link
Member

I have a hunch. (ls | sort-by name) also loses it, so I suspect subexpression evaluation is to blame.

@sophiajt
Copy link
Member

Yup - eval_subexpression returns a PipelineData, which should be fine. The problem is here:

                eval_subexpression(engine_state, stack, block, PipelineData::new(expr.span))?
                    .into_value(expr.span),

The .into_value will convert PipelineData into a Value, and in doing so will lose the metadata

@fdncred
Copy link
Collaborator Author

fdncred commented Feb 16, 2022 •
edited

ya, you're right.. sometimes?
image

as an alias
image

i'm so confused <--- EDIT - it's using the alias here - the first one was not
image

@sophiajt
Copy link
Member

Is that last image after you aliased ls?

@fdncred
Copy link
Collaborator Author

fdncred commented Feb 16, 2022 •
edited

Is that last image after you aliased ls?

yup - i just put an EDIT above it to explain it. But before i aliased it, the colors are there.

@sophiajt sophiajt changed the title ls | sort-by name when called via alias loses ls_colors (ls | sort-by name) and other subexpressions lose ls_colors Feb 16, 2022
@onthebridgetonowhere onthebridgetonowhere added the 🐛 bug Something isn't working label Feb 17, 2022
@fdncred
Copy link
Collaborator Author

fdncred commented Feb 17, 2022

Yup - eval_subexpression returns a PipelineData, which should be fine. The problem is here:

                eval_subexpression(engine_state, stack, block, PipelineData::new(expr.span))?
                    .into_value(expr.span),

The .into_value will convert PipelineData into a Value, and in doing so will lose the metadata

I don't see where there is an available pipelinedata.metdata to use around here. The closest I could find would be maybe using eval_expression_with_input() which has an input with metadata. Am I missing something?

@hustcer hustcer added the 0.60 label Mar 4, 2022
@hustcer hustcer added this to the v0.60.0 milestone Mar 5, 2022
@hustcer hustcer modified the milestones: v0.60.0, v0.61.0 Mar 18, 2022
@hustcer hustcer removed the 0.60 label Mar 18, 2022
@hustcer hustcer removed this from the v0.61.0 milestone Mar 26, 2022
Closed
@naosense
Copy link

naosense commented Nov 8, 2022

What's the status of this issue?

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 9, 2022

What's the status of this issue?

it's still not fixed

@naosense
Copy link

naosense commented Nov 9, 2022

has any plan for this issue?

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 9, 2022

There are other priorities at the moment. We're always open to people making nushell better with PRs.

@naosense
Copy link

I would like to help, but I'm afraid that this is beyond my current level. looks like it's not simple based on this thread

@fdncred fdncred mentioned this issue Nov 15, 2022
Closed
@fdncred fdncred mentioned this issue Dec 29, 2022
Open
@sholderbach sholderbach added the pipeline-metadata Additional information passed along that is not directly attached to the values (e.g. LSCOLORS) label Dec 30, 2022
@Mehrbod2002 Mehrbod2002 mentioned this issue Jan 5, 2023
Closed
@fdncred fdncred mentioned this issue Feb 2, 2023
Closed
This was referenced Feb 9, 2023
Merged
Open
kubouch added a commit that referenced this issue Feb 11, 2023
@kubouch @fdncred
Benchmark each pipeline element (#7854)
58529aa 
# Description

Adds a `profile` command that profiles each pipeline element of a block
and can also recursively step into child blocks.

# Limitations
* It is implemented using pipeline metadata which currently get lost in
some circumstances (e.g.,
#4501). This means that the
profiler will lose data coming from subexpressions. This issue will
hopefully be solved in the future.
* It also does not step into individual loop iteration which I'm not
sure why but maybe that's a good thing.

# User-Facing Changes

Shouldn't change any existing behavior.

# Tests + Formatting

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](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.

---------

Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
@fdncred fdncred mentioned this issue Apr 19, 2023
Closed
@sophiajt
Copy link
Member

(ls | sort-by name) still loses LS_COLORS in latest main

@amtoine
Copy link
Member

amtoine commented Aug 11, 2023

and ls | sort-by name does not 🤔

@fdncred
Copy link
Collaborator Author

fdncred commented Aug 11, 2023

and ls | sort-by name does not 🤔

JT explains why here #4501 (comment) - it would be so nice to have this fixed once and for all.

@izuzak
Copy link

izuzak commented Oct 8, 2023

👋 I was encouraged in Discord to add a case I hit to this issue.

  • Output of ls shows both filenames and dates with color (name and modified columns)
  • Output of ls | reject ... shows filenames without color (unexpected), but dates with color (expected)
  • Output of ls | move ... shows both filenames and dates with color (both expected)

Image

@stroborobo
Copy link

Seems like this is fixed in 0.92.

@fdncred
Copy link
Collaborator Author

fdncred commented Apr 12, 2024

Seems like this is fixed in 0.92.

Not all of it, you can't even do the alias that I originally mentioned anymore. However, nushell has got better about passing metadata around which affects the highlighting.

@stroborobo
Copy link

stroborobo commented Apr 12, 2024 •
edited

Fair, the nature of aliases changed since that time (#8123), so does it really matter? I thought this issue was about pipelining in subexpressions loosing color metadata.

I'm just happy that is working now, so thanks <3

@fdncred
Copy link
Collaborator Author

fdncred commented Apr 12, 2024

It's better but it's not complete. I saw one the other day that was still broken and made me think we need to have a metadata set command.

@poliorcetics poliorcetics mentioned this issue Apr 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛 bug Something isn't working pipeline-metadata Additional information passed along that is not directly attached to the values (e.g. LSCOLORS)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@izuzak @fdncred @sophiajt @stroborobo @hustcer @naosense @sholderbach @amtoine @onthebridgetonowhere