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
Colours shouldn't depend on environment variables or stderr #4193
Comments
Yes, I think there were lots of tradeoffs here. We decided to go with the philosophy at https://docs.rs/anstream/0.6.11/anstream/ & rust-cli/concolor#47 (possibly there's a fuller description somewhere but I couldn't immediately find it) i.e.
Worth noting that
Lots of tradeoffs — the downside is that every function requires an additional param, and we need to thread those params through the every library call... Happy to leave this open to solicit and thoughts or questions; eventually we can mark as postponed and come back to it when libraries such as insta have added something to strip the codes |
I recently worked on the code in ClickHouse for its native PRQL support. I added something to strip color codes, since PRQL doesn't do that any longer. I worry that removing that option was too strict. While the ideology of So possibly we add back re-enable |
What happened?
Currently
prql
has the inadvisable behaviour that the presence of ANSI colour codes in the output for errors depends onNO_COLOR
and maybe if stderr is a tty (I didn't look into the details). This is obviously not the right thing to do, but it seems like this was a temporary backwards compatibility measure:So this is just a note to actually remove this "eventually" :-D
Tbh I don't really understand why you deprecated the
colors
option. Seems like a very useful thing?PRQL input
NA
SQL output
Expected SQL output
No response
MVCE confirmation
Anything else?
No response
The text was updated successfully, but these errors were encountered: