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.

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

make cd, cp, ls, mv, open and rm automatically strip ansi codes #6220

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

merelymyself
Copy link
Contributor

@merelymyself merelymyself commented Aug 3, 2022

Description

Following the discussion in discord over interaction of these filesystem commands with find, which highlights the matching portions, this PR makes filesystem commands automatically strip the ansi codes from the input. Because some filesystems allow ansi codes to be part of the filename, this PR also adds a switch to allow these commands to work like they used to.

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

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 --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

@fdncred
Copy link
Collaborator

fdncred commented Aug 3, 2022

Thanks @merelymyself! The core-team needs to discuss this more, so I'm going to make this a draft until we have a decision. Also, I'm not a fan of having ls in this group because I don't want to have to do ls --include-ansi just to keep the coloring.

The way I see it there are a few options here.

  1. Accept this PR and live with the parameter --include-ansi
  2. Accept this PR but remove the --include-ansi so it's essentially hard-coding removing ansi escape sequences
  3. Change find to have a opt-in parameter to show highlighting of hits, something like find -H
  4. Do nothing which forces people to do command | ansi strip | other command

@fdncred fdncred marked this pull request as draft August 3, 2022 11:35
@kubouch
Copy link
Contributor

kubouch commented Aug 3, 2022

I'd vote for option 2 because I don't see much use in keeping ANSI codes in filesystem commands.

My take on this in general (not necessarily for this PR):

Also, we should expand this in all commands that would get screwed by ANSI codes. For example, $'(ansi red)9(ansi reset)' | into int doesn't work. into int should also strip the ANSI codes. Basically any "sink" command should strip ANSI codes.

On the other hand ls outputs colored strings, it should keep ANSI codes IMO. The same is for "filter" commands like where. These just filter the data and pass it along, they should keep the ANSI codes.

@merelymyself
Copy link
Contributor Author

merelymyself commented Aug 3, 2022

To clarify, the --include-ansi does not affect the output of the command at all, only how the input is managed. For instance:
image
the colors still work. The only difference is when the input has ansi and would fail otherwise:

~/CodingProjects/nushell〉ls | find test.txt | $in.0.name | rm --include-ansi $in                           08/03/2022 07:55:30 PM
Error:
  × No valid paths
   ╭─[entry #7:1:1]
 1 │ ls | find test.txt | $in.0.name | rm --include-ansi $in
   ·                                   ─┬
   ·                                    ╰── no valid paths
   ╰────

vs

~/CodingProjects/nushell〉ls | find test.txt | $in.0.name | rm $in                                          08/03/2022 07:55:47 PM
~/CodingProjects/nushell〉                                                                                  08/03/2022 07:56:05 PM

In this case, the ansi is added by the find command.

Also note that currently, most methods of getting values from a table/record end up stripping the ansi codes anyway:
image

@fdncred
Copy link
Collaborator

fdncred commented Aug 3, 2022

@merelymyself thanks for the clarification but I'm sorry I still don't understand the ls use case. in your example, ls | find test.txt | $in.0.name | rm $in doesn't rm strip the ansi? What is the use case of ls --include-ansi?

@merelymyself
Copy link
Contributor Author

Because ls also can take in a directory as an argument.
In the case that someone wants to do something like this:

~/CodingProjects/nushell/crates〉ls | find nu- | get name | each { |dir| ls $dir } | flatten
╭────┬────────────────────────────┬──────┬──────────┬───────────────╮
│  # │            name            │ type │   size   │   modified    │
├────┼────────────────────────────┼──────┼──────────┼───────────────┤
│  0 │ nu-cli/Cargo.toml          │ file │  1.1 KiB │ 5 hours ago   │
│  1 │ nu-cli/LICENSE             │ file │  1.1 KiB │ 3 months ago  │
│  2 │ nu-cli/src                 │ dir  │  4.0 KiB │ 6 hours ago   │
│  3 │ nu-cli/tests               │ dir  │  4.0 KiB │ 8 minutes ago │
│  4 │ nu-color-config/Cargo.toml │ file │    455 B │ 6 hours ago   │
│  5 │ nu-color-config/LICENSE    │ file │  1.1 KiB │ 3 months ago  │
│  6 │ nu-color-config/src        │ dir  │  4.0 KiB │ 2 months ago  │
│  7 │ nu-command/Cargo.toml      │ file │  4.1 KiB │ 5 hours ago   │
│  8 │ nu-command/LICENSE         │ file │  1.1 KiB │ 3 months ago  │
│  9 │ nu-command/assets          │ dir  │  4.0 KiB │ 3 months ago  │

Note that this errors if ansi codes are not stripped:

~/CodingProjects/nushell/crates〉ls | find nu- | get name | each { |dir| ls --include-ansi $dir } | flatten
Error: nu::shell::directory_not_found (link)

  × Directory not found
   ╭─[entry #31:1:1]
 1 │ ls | find nu- | get name | each { |dir| ls --include-ansi $dir } | flatten
   ·                                                           ──┬─
   ·                                                             ╰── directory not found
   ╰────

@kubouch
Copy link
Contributor

kubouch commented Aug 3, 2022

Ah, yeah, so the ls should also strip the input ansi codes. I don't think we need the --include-ansi flag, though.

@fdncred
Copy link
Collaborator

fdncred commented Aug 3, 2022

Ah, as input. Thanks. I see your point now. Seems like kubouch and I are in agreement in supporting number 2.

@fdncred
Copy link
Collaborator

fdncred commented Aug 4, 2022

@merelymyself could you go ahead and implement number 2 above, i.e. remove the --include-ansi flag?

@merelymyself
Copy link
Contributor Author

merelymyself commented Aug 4, 2022

I did so, but just leaving my concerns here: some filesystems do allow for ansi escapes in their filenames, as seen here:
image

image

I would prefer there to still be an option for this edge case, although I will admit it is exceedingly rare.

In this edge case, commands fail without the --include-ansi arg:

~〉rm (echo [(ansi rb) Hello " " (ansi gb) Nu " " (ansi pb) World (ansi reset)] | str collect)                                                                   
Error:
  × No valid paths
   ╭─[entry #3:1:1]
 1 │ rm (echo [(ansi rb) Hello " " (ansi gb) Nu " " (ansi pb) World (ansi reset)] | str collect)
   · ─┬
   ·  ╰── no valid paths
   ╰────

@merelymyself merelymyself marked this pull request as ready for review August 4, 2022 10:25
@fdncred
Copy link
Collaborator

fdncred commented Aug 4, 2022

here's hoping we never see this edge case. thanks!

@fdncred fdncred merged commit 3b809b3 into nushell:main Aug 4, 2022
panicbit added a commit to panicbit/nushell that referenced this pull request Aug 18, 2022
@crides
Copy link
Contributor

crides commented Jan 17, 2024

Came here after accidently creating a file with only an escape. But I'm not sure why we need to have this restriction anyway. If somehow the original intention is to allow highlights to function properly, then just do what most shell/language (e.g. bash, zsh, python) does: only use the value literally when not presenting to the user (in other words, use repr() (from python) when presenting visually).

zsh processes files with escapes with 0 problems (filenames are not entered; they are there because of the completion):

maim-2024 01 16 23 59 12

eza/exa have no problem presenting them either (albeit the highlighting is off on the second one):

maim-2024 01 16 23 59 31

bash has no problem accepting literal escapes as filenames:

maim-2024 01 17 00 00 50

not much of an example, but python does normal python things:
maim-2024 01 17 00 00 03

@crides
Copy link
Contributor

crides commented Jan 17, 2024

I should mention that, this isn't only about escapes. Every one of the non-printable characters should be treated as the same, as they are valid parts of a file name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants