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

Adding support for colorized JSON output #2125

Closed
wants to merge 1 commit into from

Conversation

alexejk
Copy link

@alexejk alexejk commented Aug 22, 2016

As an attempt to provide support for #2123 this contains JSON colorization.
Basically the only difference is additional dependency and updates to the JsonFormatter to pipe through Pygments before writing out to the stream.

This PR also takes into consideration color flag for output and defaults to off for backwards compatibility.

There might be a more efficient way of doing this - to be frank, Python is not my strongest side.

@jamesls
Copy link
Member

jamesls commented Aug 23, 2016

Thanks for the pull request. There's a previous discussion about this feature: #10

Based on the feedback I've heard from people (including that thread):

  • People are (understandably) hesitant to add big dependencies such as pygments. Especially in this case because we're using such a small fraction of the available features needed from pygments
  • I prefer not to have conditional behavior based on the existence of a package being installed or not (one of the original ideas in If pygments is installed, produce colorized json output by default. #10).
  • If you do have pygments installed, you can already run aws ec2 describe-instances | pygments -l json.

Just to throw out another alternative, has anyone looked at how much work it is to write a colorized JSON lexer? I think the assumption has been that it's too much code to be worth the trouble, but perhaps that's a reasonable compromise. We already have colorama as a dep that we use to produce colorized output for --output table.

@JordonPhillips
Copy link
Member

JordonPhillips commented Jul 24, 2017

As James mentioned, this isn't how we'd like to go about supporting this feature. I'm closing this out, but feel free to open a new PR that goes along with James's suggestions. Feature request opened at #2123.

@aisipos
Copy link

aisipos commented Jul 24, 2017

Quote:
I prefer not to have conditional behavior based on the existence of a package being installed or not (one of the original ideas in #10).

How about if the dependency were conditional, but still required some command line flag to activate (eg, --colorize)? That way the output is predictable in the general case, but would show colorized output with the flag, or an error message indicating you to install pygments if it weren't installed.

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

Successfully merging this pull request may close these issues.

None yet

4 participants