tomnomnom / gron

Make JSON greppable!
MIT License
13.87k stars 328 forks source link

gron should ignore ANSI colors in it's input #47

Open rjmunro opened 6 years ago

rjmunro commented 6 years ago

I was forcing colorize mode so that I would still see the color when I piped the output to grep:

$ gron -c "https://api.github.com/repos/tomnomnom/gron/commits?per_page=1" | grep name
json[0].commit.author.name = "Tom Hudson";
json[0].commit.committer.name = "GitHub";

I then tried to pipe the output to ungron, and got a really unhelpful error:

$ gron -c "https://api.github.com/repos/tomnomnom/gron/commits?per_page=1" | grep name | ungron
ungron failed for ``: invalid statement

gron / ungron should ignore any ANSI formatting codes that it or another tool might have generated, as long as they don't occur inside JSON strings (in which case they should probably be preserved and converted to \u00xx format for readability).

It would also be good if the error message could be improved, by giving a line number and the error character in \u00xx format if it is non printable.

tomnomnom commented 6 years ago

Hi @rjmunro! Thanks for reporting this :)

I'll have a think about the best way to approach this.

It would also be good if the error message could be improved, by giving a line number and the error character in \u00xx format if it is non printable.

That sounds like a good idea; I'll add that to the list of things to implement :)

bAndie91 commented 5 years ago

hi guys. i'm strongly against the idea that ungron would have the responsibility to ignore escape codes. it's a good practice to let the output producer component to check who is on the other end of the pipe (usually by isatty()) and colorize accordingly. I suggest to add the isatty()-like condition in gron to solve this issue. cheers.

rjmunro commented 5 years ago

@bAndie91 gron already has this, but this means you never any syntax colouring if you want to use gron with grep, which is kind of the main point. So it has an option to force colouring to be on (-c), which is great.

What's the problem with ignoring escape codes? It should only be a small adjustment to the parser, but it's a huge saving when you need it.

bAndie91 commented 5 years ago

@rjmunro ye sorry i did not compare the current codebase to the version of the age of this ticket, just checked that the ticket is open. i'm glad that gron tests for isatty and stuff. i'm also ok with force coloring switch, grep and ls have it too.

stripping escape sequence (as ungron would do) is an other topic. i just have not seen this behaviour in any let's say standard unix tool. and again, stripping by ungron and auto-no-coloring by gron solves the same problem. my other thought on this topic is that auto-no-coloring by gron is more deterministic since it knows what is data and what is styling, while on the other side of the pipe it's not so unambiguous to tell what was intended to be the part of the data and what not.

if I designed someting where terminal escape sequences were flowing though the pipe I'd put an additional component in the pipe whiches only responsibility is to strip the codes (like i sometimes sentenced do it with programms which misregard isatty, eg. colorful-tool | sed -e 's/\x1B...something...//g' | egrep ^xyz).

thanks for your attention.

rjmunro commented 5 years ago

This started because the error message you get for passing gron -c to ungron was really unhelpful (ungron failed for ``: invalid statement), and stumped me for several minutes. I don't mind that much about auto-stripping colour if we can fix that to be more explanative - e.g. by having it escape the characters with the error in the error message.

bAndie91 commented 5 years ago

i agree ungron may indicate where the parse error happened by a snippet with escaped control chars in it - if you mean it.