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

[REQUEST] Respect PY_COLORS environment variable #343

Closed
ssbarnea opened this issue Oct 6, 2020 · 11 comments
Closed

[REQUEST] Respect PY_COLORS environment variable #343

ssbarnea opened this issue Oct 6, 2020 · 11 comments
Labels
accepted Task was accepted

Comments

@ssbarnea
Copy link

ssbarnea commented Oct 6, 2020

PY_COLORS is a very popular method for forcing ANSI mode on or off in a command line application.

Basically undefined means keeping application default behavior, likely auto-detection. Having it PY_COLORS=0 means application will not use any ANSI codes and PY_COLORS=1 will force ANSI mode regardless if terminal reports it or not.

For me personally is not a big deal as I already have code in applications I work with, but it would prove useful to have this directly into rich, so any user of rich would get this for free.

An implementation example can be seen at https://github.com/ansible-community/molecule/blob/20e1fa8a23308a9103e2dbdb4d3c4ae1ffe59c9d/molecule/logger.py#L42-L48

That is very useful feature to have when running on CI as you do not want to end-up defining 50-100 environment variables only to control ANSI behavior.

@willmcgugan
Copy link
Collaborator

It would be useful to force color on. The only related standard I know is NO_COLOR which disables color, which Rich supports.

I gather this is from PyTest originally? Which I see also supports a FORCE_COLOR env var.

FORCE_COLOR and NO_COLOR compliment each other and give you complete control over ansi output. I suspect PyTest added PY_COLOR as an override when the other env vars were in use by test code, hence the specific prefix of PY. Popular or not, it's may be a mistake to view it as some kind of standard beyond PyTest.

That is very useful feature to have when running on CI as you do not want to end-up defining 50-100 environment variables only to control ANSI behavior.

Let's not exaggerate.

@ssbarnea
Copy link
Author

ssbarnea commented Oct 6, 2020

I think it comes from python itself, but I do not remember exactly. I do agree that the PY_ prefix it does not make much sense as this behaviour could apply to any tools.

I am aware of existence of https://no-color.org/ movement but sadly it lacked no color but sadly it started with a broken design and lacks any drivers for fixing these. See https://github.com/jcs/no_color/issues/29 (and linked ticket on how others reacted, xkcd reference as bonus)

You can see my old ticket from https://github.com/jcs/no_color/issues/79 which didn't get any feedback.

@willmcgugan
Copy link
Collaborator

It's definitely not a Python thing or it would be documented here.

I'll give it some thought, but with the lack of a real standard, it's hard to know what the "right thing" is to do.

@willmcgugan willmcgugan added accepted Task was accepted and removed Needs triage labels Oct 6, 2020
@Edward-Knight
Copy link
Contributor

I was having a look at implementing this. What colour mode should PY_COLORS turn on?
https://github.com/willmcgugan/rich/blob/c3db5a8c0d48a2a1e27a2accde155bedd879181a/rich/console.py#L279-L284
I assume ColorSystem.WINDOWS on Windows, but I'm unsure for other platforms.

Or will it be enough to just turn off this clause?
https://github.com/willmcgugan/rich/blob/c3db5a8c0d48a2a1e27a2accde155bedd879181a/rich/console.py#L465-L466

@willmcgugan
Copy link
Collaborator

It should probably set the force_terminal attribute if not explicitly set.

However, I've pretty much rejected PY_COLOR as its not really an established standard.

FORCE_COLOR seems better, but is still problematic. In some apps FORCE_COLOR set to anything enables color, but I've also see FORCE_COLOR=1 enable color and FORCE_COLOR=0 disable it.

And I've also see CLICOLOR used.

Since its not clear exactly what is the right thing to do, I'd prefer to leave it up to the developer to implement.

@ssbarnea
Copy link
Author

I agree with you that at this point, due to divergence in the wild, it does not make sense to make a change. I will close the ticket with the note that we should reopen it once we have an emerging winner in that area.

@ewels
Copy link
Contributor

ewels commented Nov 5, 2020

Is there any downside for Rich supporting all of these environment variables? It sounds to me like they are all established standards, I'm not sure why there has to be a single winner..

@ewels
Copy link
Contributor

ewels commented Nov 5, 2020

(You could even add the GITHUB_ACTIONS env var to the list, which is how I ended up here 👀 - x-ref #52)

@willmcgugan
Copy link
Collaborator

They not really standards, since they're not documented by any kind of authority. And not all that established, since relatively few apps support them.

They're also implemented inconsistently, for instance on some apps FORCE_COLOR=0 would force color on because anything other than empty string is considered True, but some apps would treat "0" as false and disable color. And if I implement more than one, what if they conflict?

If I was to add support for these env vars, then I've baked that in to all apps using Rich, which means that if I change it in the future when / if a standard emerges @willmcgugan is going to get a lot of angry mentions.

So I'd rather to leave it up to the developer what env vars they should support. It's only a line or two to set force_color on the Console constructor.

I may yet relent on this. I try to be pragmatic!

@ssbarnea
Copy link
Author

ssbarnea commented Nov 5, 2020

If anyone is curious, I ended up with https://github.com/ansible-community/molecule/blob/master/lib/molecule/console.py#L40-L60 implementation, which is bit hilarious but should produce decent results.

Anything around terminal is lacking standards, because there is no authority and too many use-cases. Until major players would team-up to join some kind of movement we will still need to rely on guesstimates. My respect goes to those that tried to reuse existing approaches instead of creating MYAPP_WANTS_COLOR kind of approaches.

It took a good number of years to see the hyperlink ANSI extensions being adopted by various terminals and there is no official standard for it. Colorama project refused my patch to add support for links due to lack of standards. Attempts to do the same for defining collapsable sections sadly didn't materialize yet.

@ewels
Copy link
Contributor

ewels commented Nov 5, 2020

Yes, for future googlers wanting to copy and paste, here was my workaround to do the same with our codebase: https://github.com/nf-core/tools/pull/760/files (bit simpler / more crude, includes direct detection for GitHub Actions).

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

No branches or pull requests

4 participants