search

sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
9.07k stars 227 forks source link

Add --color flag to disable ANSI sequences #30

Closed bennetthardwick closed 5 years ago

bennetthardwick commented 5 years ago

When printing the output of hexyl to a file (hexyl file.exe > file.txt), the file.txt file gets polluted by the ANSI escape sequences. Checking whether the application has been launched from inside the terminal or not, allows you to decide whether or not to use them.

I don't have access to a Windows machine, but atty claims that it has been tested on Windows.

kilobyte commented 5 years ago

Note that this breaks a common use case, of redirecting |less -R. On the other hand, so do almost all similar programs, so this would be consistent. On the third hand, overriding isatty() is tedious without one of my, little known, tools (pipetty).

roryokane commented 5 years ago

Note that this breaks a common use case, of redirecting |less -R.

If color codes were not emitted through pipes by default, that use-case could be restored by requiring a --color flag that would force ANSI escape sequences to be emitted.

sharkdp commented 5 years ago

@bennetthardwick Thank you very much for your contribution!

Note that this breaks a common use case, of redirecting |less -R.

True. Adding a --color=auto/always/never flag could be one option. This is what I did with https://github.com/sharkdp/fd and https://github.com/sharkdp/bat. However, this would require users to type

hexyl --color=always … | less -R

if they wanted to use a pager. To solve this, we could think about adding a --pager[=CMD] flag, similar to how this is done in https://github.com/sharkdp/bat. I don't want to enable paging by default (even with the "quit if one screen" setting of less) because that has caused so much pain in bat. But I'd be fine with a opt-in feature.

For now, I'd say we merge this and add a --color flag afterwards.

kilobyte commented 5 years ago

There's also an env var named CLICOLOR_FORCE that a bunch of programs support. While I despise its name on aesthethic grounds, it might be good to have it behave same as --color=always.

lilyball commented 5 years ago

If you don't want color, why use hexyl? That's kind of its defining purpose.

sharkdp commented 5 years ago

If you don't want color, why use hexyl? That's kind of its defining purpose.

I fully agree. But what if somebody was looking at the colored output and now wants to write that to a file or use grep to filter the output? Yes, he could now switch to hd/xxd, but it's easier to just bring up the last command and pipe it somewhere else.

lilyball commented 5 years ago

The problem is piping to less -R is likely to be a very common use-case, so hurting that use-case in favor of the less common case of piping to a file is problematic.

sharkdp commented 5 years ago

Okay, I tend to agree. @bennetthardwick what do you think?

kilobyte commented 5 years ago

It depends on how far are you going to extend hexyl. Is it going to be just a simple colorized hd? If so, there's no reason to not use hd when colors are not needed (and the user can use ansi2txt if for some reason he/she wants hexyl).

But if there's a plan for anything that hd can't do, then piping definitely should be supported, and in such case you'd want consistency with other programs. I mean, if for example you'd implement #6, it'd be something useful even without color.

So it's about how many features you plan for.

lilyball commented 5 years ago

We could add a --no-color flag if someone wants to dump to a file without trying to auto-detect color support.

bennetthardwick commented 5 years ago

I agree with @kballard, if there's an existing use case that's quite popular it wouldn't be great to break it. hexyl could support the --color flag and default to always, which would allow anyone who doesn't use less -R to set --color=auto as an alias or something. But I think creating an argument that disables the colours is probably the best idea.

sharkdp commented 5 years ago

hexyl could support the --color flag and default to always, which would allow anyone who doesn't use less -R to set --color=auto as an alias or something. But I think creating an argument that disables the colours is probably the best idea.

That sounds very good to me. If you want to work on this, please amend this PR. If not, feel free to say so and somebody else can continue working on this. In any case, thank you very much!

bennetthardwick commented 5 years ago

Sweet! I've updated the PR to use the --no-color flag instead of detecting whether the output is to a terminal.

sharkdp commented 5 years ago

Thank you very much! I have updated the PR to use --color=always/auto/never (defaulting to always) instead of --no-color. This way, users can set

alias hexyl="hexyl --color=auto"

if they want the TTY-detection behavior.