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

Add --color flag to disable ANSI sequences #30

Merged

Conversation

bennetthardwick
Copy link
Contributor

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.

@bennetthardwick bennetthardwick force-pushed the feature/exclude-ansi-when-not-tty branch from 8eaa9cb to 723db2d Compare January 11, 2019 12:31
@kilobyte
Copy link

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
Copy link

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
Copy link
Owner

sharkdp commented Jan 13, 2019

@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
Copy link

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
Copy link
Contributor

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

@sharkdp
Copy link
Owner

sharkdp commented Jan 13, 2019

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
Copy link
Contributor

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
Copy link
Owner

sharkdp commented Jan 13, 2019

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

@kilobyte
Copy link

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
Copy link
Contributor

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

@bennetthardwick
Copy link
Contributor Author

bennetthardwick commented Jan 13, 2019

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.

@bennetthardwick bennetthardwick force-pushed the feature/exclude-ansi-when-not-tty branch from 723db2d to 43b20cd Compare January 13, 2019 21:17
@sharkdp
Copy link
Owner

sharkdp commented Jan 13, 2019

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 bennetthardwick force-pushed the feature/exclude-ansi-when-not-tty branch from 43b20cd to 0c232b5 Compare January 13, 2019 21:30
@bennetthardwick
Copy link
Contributor Author

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

@bennetthardwick bennetthardwick changed the title Remove ANSI sequences when using shell redirect Add --no-color flag to disable ANSI sequences Jan 13, 2019
@sharkdp
Copy link
Owner

sharkdp commented Jan 14, 2019

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.

@sharkdp sharkdp changed the title Add --no-color flag to disable ANSI sequences Add --color flag to disable ANSI sequences Jan 14, 2019
@sharkdp sharkdp merged commit f6f41e7 into sharkdp:master Jan 14, 2019
@sharkdp sharkdp mentioned this pull request Apr 27, 2019
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.

5 participants