If pygments is installed, produce colorized json output by default. #10
Conversation
|
Slight update to ensure pygments has json support before using it. Older versions of pygments didn't have json support until https://github.com/orb/pygments-json was merged. |
| #Also ensure the JSON lexer is installed. | ||
| #Old versions of pygments don't have this installed, | ||
| #it's available separately as pygments-json | ||
| lexer = pygments.lexers.get_lexer_by_name('json') |
There was a problem hiding this comment.
What happens if the JSON lexer is not installed? Does get_lexer_by_name throw an ImportError?
There was a problem hiding this comment.
Good point, indeed you are right. get_lexer_by_name throws a pygments.util.ClassNotFound when the given lexer is not found (see http://pygments.org/docs/api/).
I updated the PR to catch any exception when trying to use pygments, falling back to the straight JSONFormatter if anything exception arise.
|
Updated PR to catch all errors when trying to use the pygments JSON lexer, as throws its own exceptions rather than ImportError if it can't find a specific lexer. |
|
Updated PR to fix compatibility with Python 3 - Travis build now passes. Also rebased on top of latest commit to develop branch. |
|
It would be nice to have Pygments be automatically installed when i |
|
It would be easy to make pygments a dependency for aws-cli. But it's also a judgement call by the maintainers, it makes the install a bit heavier and isn't a requirement. @garnaat : Do you think this PR is useful, and what are your thoughts on a pygments dependency? |
|
Tough call, I agree. But I never would have heard of pygments had i not come here to file a different bug. Not sure what the right way to deal with it is; can pip ask if you want to install optional dependencies? That would probably be ideal. |
|
No, pip doesn't provide user prompted optional dependencies. Prompting the user would only confuse them anyway - the library should make a reasonable choice for devs - whether to provide it or not. The docs can state they can get colored output merely by doing a |
|
I'd like this feature but my 2 cents would be to leave it optional. I typically have pygments installed on development workstations (for Sphinx documentation among other things), but could do without the extra download/package on servers. Speaking of servers, for scripting I humbly request that this patch be enhanced to not colorize if I'm piping, redirecting output to a file, etc. (check |
Fall back to standard uncolorized output if pygments is not installed. Only output colorized json if output is a tty
|
I rebased the PR to the head of the develop branch, and changed it so that colorized output is only produced when writing to a stream that .isatty(). Thanks for the suggestion, that should have been the correct behavior the whole time. Pygments support is indeed optional. If you don't have pygments installed with this PR, it will fall back to straight uncolorized output. |
|
👍 |
|
👍 |
JordonPhillips
added
pr:needs-review
|
Closing this out since this isn't the way we want to go with this feature. Feature request located at #2123. |
Implement copy method for transfer manager
Fall back to standard uncolorized output if pygments is not installed.