Skip to content

Add colors to JSON printer #3343

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

Merged
merged 17 commits into from
Jul 15, 2023
Merged

Add colors to JSON printer #3343

merged 17 commits into from
Jul 15, 2023

Conversation

mavam
Copy link
Member

@mavam mavam commented Jul 11, 2023

This PR brings a few JSON printer changes:

  1. Make --pretty the default for write json
  2. Ditch --pretty in favor of the opposite -c|--compact-output (like jq)
  3. Add -C|--color-output and -M|--monochrome-output to explicitly set the color.

Unlike jq, there's currently no auto-detection of colors. You have to provide -C for now. While we could do TTY detection, it's a bit more complicated from the printer's perspective, because we need to know the corresponding saver in order to make a call. If it's stdout, we can add colors, if it's a file, we can't.

### Tasks
- [x] Implement styling logic
- [x] Support clear and colored styles
- [x] Add changelog entry
- [x] Update docs
- [x] Adapt unit tests
- [x] Adapt integration tests

@mavam mavam added the format Parser and printer label Jul 11, 2023
@mavam mavam force-pushed the topic/json-coloring branch from 30a809c to 6eb99d9 Compare July 11, 2023 08:07
@tobim
Copy link
Member

tobim commented Jul 11, 2023

I'm not sure I like these new defaults. I guess I don't have a strong opinion.

Can you please add support for NO_COLOR? An equivalent Tenzir specific config option should also be available.

@mavam
Copy link
Member Author

mavam commented Jul 11, 2023

I'm not sure I like these new defaults. I guess I don't have a strong opinion.

These are jq and that's the de-facto UX for JSON processing. If you don't have a strong opinion, I would strongly recommend to follow their defaults.

Can you please add support for NO_COLOR? An equivalent Tenzir specific config option should also be available.

I added support for NO_COLOR in the style detection, but don't know what you mean by "config option".

@tobim
Copy link
Member

tobim commented Jul 11, 2023

but don't know what you mean by "config option".

Something that I can add to my tenzir.yaml to make it explicit. @jachris please chime in if you already considered a generic solution for configuring operators.

@mavam
Copy link
Member Author

mavam commented Jul 11, 2023

Something that I can add to my tenzir.yaml to make it explicit.

This is out of scope for this PR. We don't have something like that yet.

I also don't think operators should be able to configured in an external file. Then you can no longer share pipelines and assume they do what you'd expect.

@jachris
Copy link
Contributor

jachris commented Jul 11, 2023

please chime in if you already considered a generic solution for configuring operators.

Not really, but since every operator is a plugin, you could use the plugin configuration for that.

I also don't think operators should be able to configured in an external file. Then you can no longer share pipelines and assume they do what you'd expect.

We have the same problem with environment variables like NO_COLOR though. But in general, I tend to agree that we want to avoid changing the observable behavior with config options (unless it leads to an error, like allow-unsafe-pipelines).

@tobim
Copy link
Member

tobim commented Jul 11, 2023

I guess not providing config options for operators is fine for most cases, purely cosmetic options like NO_COLOR should be exempt though.

@Dakostu Dakostu force-pushed the topic/json-coloring branch from a97747f to 46e3c66 Compare July 12, 2023 07:48
@Dakostu Dakostu marked this pull request as ready for review July 12, 2023 08:12
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working well in practice. Thanks!

@mavam
Copy link
Member Author

mavam commented Jul 13, 2023

@dominiklohmann I saw you sneaking in the --omit-* commit! Planned that for another PR. But oh well, @satta will be happy. 😉

@dominiklohmann
Copy link
Member

Hah, I just noticed that being documented but not implemented when reviewing this PR, so I asked Daniel whether it was okay to just push it on top. Felt quicker than writing a story for it.

@satta
Copy link
Contributor

satta commented Jul 13, 2023

I will be happy indeed :) Thanks!

BTW, are you using isatty() or something similar to detect whether you're printing to a terminal or not and deactivate colors in the latter case? I found that useful in order to avoid escape sequences in redirected JSON output.

@dominiklohmann
Copy link
Member

BTW, are you using isatty() or something similar to detect whether you're printing to a terminal or not and deactivate colors in the latter case? I found that useful in order to avoid escape sequences in redirected JSON output.

We discussed somewhere in a thread for this PR that we actually need to pass that information up from the last operator, and wrote a follow-up story for that. For now, colors are opt-in.

E.g., if you run this:

tenzir '… | write json | save file foo.json'

Then the information of whether or not we're printing to a terminal must be passed from save to write. We talked about the how, but it's for sometime in the future because write cannot reliably detect that on its own.

@satta
Copy link
Contributor

satta commented Jul 13, 2023

I see, true!

@dominiklohmann dominiklohmann enabled auto-merge July 14, 2023 11:53
@dominiklohmann dominiklohmann merged commit 89444d5 into main Jul 15, 2023
@dominiklohmann dominiklohmann deleted the topic/json-coloring branch July 15, 2023 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format Parser and printer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants