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

Don't strip ANSI escapes before fs commands #12567

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sholderbach
Copy link
Member

Description

Don't strip ANSI escapes before executing file-sytem commands

Previously all our file system operations tried to remove ANSI escape
codes from their input (as well as other characters like \t).

  • This is bad if you want to deal with files that are allowed arbitrary names in the bounds of the OS/FS
  • This was done as we had ls inserting colors before we had the working PipelineMetadata system.
  • Currently find will still insert ANSI coloring into its result and messing with downstream commands (see find command adds colors to result also in scripts #11899)

Closes #6315

User-Facing Changes

The following commands will faithfully accept their paths as is:

  • mv
  • cp
  • ls
  • rm

You should not pipe the output of find into file-system commands until we change find to not emit ANSI in stream

ls | find bla | each {|entry| rm $entry.name }

Tests + Formatting

Drop tests that pipe find output into fs cmds

Previously all our file system operations tried to remove ANSI escape
codes from their input (as well as other characters like `\t`).
- This is bad if you want to deal with files that are allowed arbitrary
names in the bounds of the OS/FS
- This was done as we had `ls` inserting colors before we had the
working `PipelineMetadata` system.
- Currently `find` will still insert ANSI coloring into its result and
messing with downstream commands (see nushell#11899)

Closes nushell#6315
Reflect the breaking change.

See nushell#11899 for other problems with `find`
@sholderbach sholderbach added file-system Related to commands and core nushell behavior around the file system pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes labels Apr 18, 2024
@fdncred
Copy link
Collaborator

fdncred commented Apr 18, 2024

Although different, this PR reminds me of this one #11494, which I'd like to see landed when it's working.

@sholderbach
Copy link
Member Author

Although different, this PR reminds me of this one #11494, which I'd like to see landed when it's working.

That would be nice as well to have

Your #12564, reminded me of looking into reimplementing find with its own PipelineMetadata. There are a few things that I will probably change in its semantics for better consistency (and possibly on accident due to the overall complexity)

@fdncred
Copy link
Collaborator

fdncred commented Apr 18, 2024

find

Ya, there are things I'd like to see changed too. Highlighting is not one of them though. By that I mean, I agree with your suggestion with having table handle the highlighting. I just don't want to lose the highlighting hits ability altogether. Also, the --regex does not highlight at all. It would be nice to have some consistency with --regex and highlight how the string search one does.

image

But, at this point, I still like having the regular string search and a separate regex search. I'm not sure I'd want to simplify and remove the regular string search. Having said that, if something like that was done, and the functionality was still there but under-the-hood it only used regex, I'd probably be ok with that.

It also has a bunch of command line parameters. I wonder if separate sub-commands are needed? 🤔 I'm not sure if there's value in doing it though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-system Related to commands and core nushell behavior around the file system pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ls/rm/cd strip ansi escapes Unable to remove file with tabulation symbol in its name
2 participants